Report errors returned by _qspi_configure_format #11603

kyle-cypress · 2019-09-30T22:00:12Z

Description

The function returns a qspi_status_t but most usages in QSPIFBlockDevice assume that it always succeeds.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[x] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

Release Notes

ciarmcom · 2019-09-30T23:00:24Z

@kyle-cypress, thank you for your changes.
@ARMmbed/mbed-os-storage @ARMmbed/mbed-os-maintainers please review.

SeppoTakalo · 2019-10-01T07:34:27Z

I like the fixes, but I don't like to extend more error codes into the system.
It is a pain to maintain and document.

To me the _qspi_configure_format() sounds like it will always pass or always fails on my device, depending whether I'm passing right parameters. Therefore QSPIF_BD_ERROR_DEVICE_ERROR should be sufficient error code.

The driver can produce more info by doing tr_error() call, so that while you are debugging the device, you will see that actual problem. But once running in the field, more error codes don't make sense.

0xc0170

LGTM , just a question about error numbering

Edit: rethinking my review after the above comment, missed it prior reviewing

0xc0170 · 2019-10-01T10:38:25Z

components/storage/blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.h

-    QSPIF_BD_ERROR_INVALID_ERASE_PARAMS  = -4005, /* Erase command not on sector aligned addresses or exceeds device size */
-    QSPIF_BD_ERROR_DEVICE_NOT_UNIQE      = -4006, /* Only one instance per csel is allowed */
-    QSPIF_BD_ERROR_DEVICE_MAX_EXCEED     = -4007 /* Max active QSPIF devices exceeded */
+    QSPIF_BD_ERROR_CONF_FORMAT_FAILED    = -4005, /* Configure format failed */


QSPIF_BD_ERROR_CONF_FORMAT_FAILED shall this be 4008? Or this follows _FAILED thus placed irght after the latest failure status and the rest is shifted.

This is made moot by 5b3b147 which removes the new error code entirely.

0xc0170 · 2019-10-01T10:40:38Z

@SeppoTakalo Acceptable patch would be to revert adding new error code and use QSPIF_BD_ERROR_DEVICE_ERROR instead ?

SeppoTakalo · 2019-10-01T12:53:28Z

Yes.

When no new error codes are added, it does not change the API and is acceptable as a "fix" into the next patch release.

bulislaw · 2019-10-01T15:50:44Z

When no new error codes are added, it does not change the API and is acceptable as a "fix" into the next patch release.

Agreed

SeppoTakalo

LGTM

VeijoPesonen · 2019-10-10T07:09:53Z

components/storage/blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.cpp

@@ -302,17 +306,23 @@ int QSPIFBlockDevice::read(void *buffer, bd_addr_t addr, bd_size_t size)
    _mutex.lock();


Remember to release this. ~~Could you also check all the places where the code decides to jump to goto exit_point-label as the the same issue seems to be found also from other functions inside this file.~~ Scratch that, other places seem to be fine.

Updated to follow the goto exit_point pattern used everywhere else.

adbridge · 2019-10-11T11:20:46Z

CI started

mbed-ci · 2019-10-11T13:02:43Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

adbridge · 2019-10-11T13:35:05Z

@kyle-cypress looks like this needs a rebase

kyle-cypress · 2019-10-11T18:51:35Z

Rebased


        Report errors returned by _qspi_configure_format

The function returns a qspi_status_t but most usages in QSPIFBlockDevice assume that it always succeeds.


        Reuse existing error for _qspi_configure_format

Use QSPIF_BD_ERROR_DEVICE_ERROR instead of introducing a new error code. Add tr_error calls whenever _qspi_configure_format fails to aid in debugging.

VeijoPesonen

Please see my previous comment.


        Avoid stale mutex in QSPIFBlockDevice::read

Update to follow the same `goto exit_point` pattern that is used by the rest of the functions to avoid leaving the mutex locked when errors are detected and require the function to abort.

kyle-cypress · 2019-10-14T21:07:47Z

Please see my previous comment.

I missed the non-struck-out text at the beginning before. Should be fixed now.

VeijoPesonen

Looks good to me

jeromecoutant · 2019-10-15T14:33:05Z

Hi
Quick test with N25Q128A and MX25R6435F seems OK,

0xc0170 · 2019-10-17T13:31:16Z

CI restarted

mbed-ci · 2019-10-17T13:44:57Z

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-IAR

0xc0170 · 2019-10-17T13:45:42Z

There's internal CI license issue, we are investigating.

0xc0170 · 2019-10-18T09:02:54Z

CI restarted

mbed-ci · 2019-10-18T10:02:54Z

Test run: FAILED

Summary: 7 of 11 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_cloud-client-test
jenkins-ci/mbed-os-ci_mbed2-build-ARM
jenkins-ci/mbed-os-ci_dynamic-memory-usage
jenkins-ci/mbed-os-ci_greentea-test
jenkins-ci/mbed-os-ci_mbed2-build-IAR
jenkins-ci/mbed-os-ci_exporter
jenkins-ci/mbed-os-ci_mbed2-build-GCC_ARM

0xc0170 · 2019-10-18T11:05:31Z

CI restarted

mbed-ci · 2019-10-18T13:58:16Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 4
Build artifacts

kyle-cypress mentioned this pull request Sep 30, 2019

Differentiate alt and dummy cycles in QSPIF #11297

Merged

ciarmcom requested review from ARMmbed/mbed-os-maintainers Sep 30, 2019

ciarmcom added the needs: review label Sep 30, 2019

0xc0170 approved these changes Oct 1, 2019

View changes

0xc0170 self-requested a review Oct 1, 2019

0xc0170 added needs: work and removed needs: review labels Oct 1, 2019

SeppoTakalo approved these changes Oct 2, 2019

View changes

kyle-cypress mentioned this pull request Oct 4, 2019

Improve QSPIFBlockDevice conformance to SFDP #11531

Merged

VeijoPesonen suggested changes Oct 10, 2019

View changes

adbridge added needs: CI release-version: 5.14.1 and removed needs: work labels Oct 11, 2019

adbridge added needs: work release-version: 5.14.2 and removed needs: CI release-version: 5.14.1 labels Oct 11, 2019

kyle-cypress force-pushed the kyle-cypress:pr/qspi-bd-format-error branch from 5b3b147 to 9730ee2 Oct 11, 2019

Kyle Kearney added 2 commits Sep 30, 2019

Report errors returned by _qspi_configure_format

726a73c

The function returns a qspi_status_t but most usages in QSPIFBlockDevice assume that it always succeeds.

Reuse existing error for _qspi_configure_format

Loading status checks…

3f20b80

Use QSPIF_BD_ERROR_DEVICE_ERROR instead of introducing a new error code. Add tr_error calls whenever _qspi_configure_format fails to aid in debugging.

kyle-cypress force-pushed the kyle-cypress:pr/qspi-bd-format-error branch from 9730ee2 to 3f20b80 Oct 11, 2019

VeijoPesonen suggested changes Oct 14, 2019

View changes

Avoid stale mutex in QSPIFBlockDevice::read

Loading status checks…

52cb2c2

Update to follow the same `goto exit_point` pattern that is used by the rest of the functions to avoid leaving the mutex locked when errors are detected and require the function to abort.

VeijoPesonen approved these changes Oct 15, 2019

View changes

0xc0170 added needs: CI and removed needs: work labels Oct 17, 2019

0xc0170 added ready for merge and removed needs: CI labels Oct 18, 2019

0xc0170 removed the ready for merge label Oct 18, 2019

ARMmbed/mbed-os

Join GitHub today

Report errors returned by _qspi_configure_format #11603

Report errors returned by _qspi_configure_format #11603

Conversation

kyle-cypress commented Sep 30, 2019

Description

Pull request type

Reviewers

Release Notes

This comment has been minimized.

ciarmcom commented Sep 30, 2019

This comment has been minimized.

SeppoTakalo commented Oct 1, 2019

0xc0170 left a comment • edited

This comment has been minimized.

0xc0170 Oct 1, 2019

This comment has been minimized.

kyle-cypress Oct 1, 2019

This comment has been minimized.

0xc0170 commented Oct 1, 2019

This comment has been minimized.

SeppoTakalo commented Oct 1, 2019

This comment has been minimized.

bulislaw commented Oct 1, 2019

SeppoTakalo left a comment

This comment has been minimized.

VeijoPesonen Oct 10, 2019 • edited

This comment has been minimized.

kyle-cypress Oct 14, 2019

This comment has been minimized.

adbridge commented Oct 11, 2019

This comment has been minimized.

mbed-ci commented Oct 11, 2019

Test run: SUCCESS

This comment has been minimized.

adbridge commented Oct 11, 2019

This comment has been minimized.

kyle-cypress commented Oct 11, 2019

VeijoPesonen left a comment

This comment has been minimized.

kyle-cypress commented Oct 14, 2019

VeijoPesonen left a comment

This comment has been minimized.

jeromecoutant commented Oct 15, 2019

This comment has been minimized.

0xc0170 commented Oct 17, 2019

This comment has been minimized.

mbed-ci commented Oct 17, 2019

Test run: FAILED

This comment has been minimized.

0xc0170 commented Oct 17, 2019

This comment has been minimized.

0xc0170 commented Oct 18, 2019

This comment has been minimized.

mbed-ci commented Oct 18, 2019

Test run: FAILED

This comment has been minimized.

0xc0170 commented Oct 18, 2019

This comment has been minimized.

mbed-ci commented Oct 18, 2019

Test run: SUCCESS

0xc0170 left a comment •

edited

VeijoPesonen Oct 10, 2019 •

edited