shims: report SPI init failures instead of ignoring them - #5
Conversation
Problem: wlan_hal_spi_init() logged errors without their code and then
continued regardless. A failed spi_bus_initialize() was still followed by
spi_bus_add_device() on an unusable bus; a failed add_device left
spi_handle indeterminate yet still passed it to
spi_device_get_actual_freq(). wlan_hal_spi_deinit() reported a
spi_bus_free() error as "spi_bus_initialize failed".
Reproduce:
1. Claim SPI2_HOST before mmhalow_init() (or let mmwlan_boot() retry
after a failure -- the failing path never runs
wlan_hal_spi_deinit(), so the host stays initialized).
2. Boot.
Observed: "spi_bus_initialize failed" with no error code, then a
causeless "Transport init failed" from mmdrv_init().
Expected: the actual esp_err_t, and no work done on a dead bus.
Fix: return esp_err_t and stop at the first error, logging
esp_err_to_name(). ESP_ERR_INVALID_STATE is benign (host already up --
the retry case) so it reuses the bus, but now drops any device the
previous attempt added instead of leaking its handle and stacking a
second device. spi_handle is cleared on every failure path so it is never
used indeterminate. mmhal_wlan_init() logs the failure, since the HAL
signature is void and cannot report upward.
Verified: no behaviour change on the success path; the retry path no
longer accumulates devices. wlan_hal_spi_init() is static, so this is not
an API change.
Note: this makes the failure attributable. It does not change how often
it occurs.
Signed-off-by: Eric Wang <eric@rwx.one>
| if (ret == ESP_ERR_INVALID_STATE) | ||
| { | ||
| ESP_LOGE(TAG, "spi_bus_initialize failed\n"); | ||
| /* The host is already initialized. This is reachable because mmwlan_boot() may be retried |
There was a problem hiding this comment.
I'm not seeing any instances where mmwlan_boot is retried after a failed attempt. There's potentially a few places where wlan_hal_init is retried, but the error paths appear to tear things down correctly.
If you found an instance where this holds can you share it? I'd like to better understand the circumstances in which the problem this change is fixing actually occurs.
If you're guarding against the possibility of this happening, I think I would prefer to "fix" the API contract so that wlan_hal_spi_deinit is actually called or any subsequent entry to wlan_hal_spi_init on an already initialised bus is ignored.
| if (ret != ESP_OK) | ||
| { | ||
| ESP_LOGE(TAG, "spi_bus_add_device failed\n"); | ||
| /* A failed add_device leaves spi_handle indeterminate, so clear it and return rather than |
There was a problem hiding this comment.
This comment is documenting the change.
Personally I think the error handling you've implemented documents itself.
| ESP_LOGW(TAG, "SPI host already initialized, reusing it"); | ||
| if (spi_handle != NULL) | ||
| { | ||
| (void)spi_bus_remove_device(spi_handle); |
There was a problem hiding this comment.
Is there a reason for void-casting the return here?
3 participants
Problem:
wlan_hal_spi_init() logged errors without their code and then continued regardless. A failed spi_bus_initialize() was still followed by spi_bus_add_device() on an unusable bus; a failed add_device left spi_handle indeterminate yet still passed it to
spi_device_get_actual_freq(). wlan_hal_spi_deinit() reported a spi_bus_free() error as "spi_bus_initialize failed".
Steps to Reproduce:
Fix:
Return esp_err_t and stop at the first error, logging esp_err_to_name(). ESP_ERR_INVALID_STATE is benign (host already up -- the retry case) so it reuses the bus, but now drops any device the previous attempt added instead of leaking its handle and stacking a second device. spi_handle is cleared on every failure path so it is never used indeterminate. mmhal_wlan_init() logs the failure, since the HAL signature is void and cannot report upward.
Test Plan:
NOTE: This makes the failure attributable. It does not change how often it occurs.