Skip to content

shims: report SPI init failures instead of ignoring them #5

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ericrwx wants to merge 1 commit into
base: main
Choose a base branch
from
+49 −9
Open

shims: report SPI init failures instead of ignoring them #5

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 49 additions & 9 deletions halow/components/shims/mmhal_wlan.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,13 @@ static void wlan_hal_gpio_init(void)
gpio_config(&io_conf);
}

static void wlan_hal_spi_init(void)
/**
* Bring up the SPI bus and add the WLAN transceiver device.
*
* @returns ESP_OK on success, otherwise the first error encountered. On failure the transceiver is
* unreachable, so the caller must not treat it as recoverable.
*/
static esp_err_t wlan_hal_spi_init(void)
{
esp_err_t ret;

Expand All @@ -89,9 +95,24 @@ static void wlan_hal_spi_init(void)
.flags = SPICOMMON_BUSFLAG_MASTER,
};
ret = spi_bus_initialize(SPI2_HOST, &buscfg, SPI_DMA_CH_AUTO);
if (ret != ESP_OK)
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

@morse-arienjudge morse-arienjudge Jul 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* after a failed attempt and the failing path does not run wlan_hal_spi_deinit(), so reuse
* the bus rather than failing. Drop any device the previous attempt added: otherwise its
* handle is leaked and a second device is stacked on the same bus. */
ESP_LOGW(TAG, "SPI host already initialized, reusing it");
if (spi_handle != NULL)
{
(void)spi_bus_remove_device(spi_handle);

@morse-cmistry morse-cmistry Jul 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for void-casting the return here?

spi_handle = NULL;
}
}
else if (ret != ESP_OK)
{
ESP_LOGE(TAG, "spi_bus_initialize failed: %s", esp_err_to_name(ret));
spi_handle = NULL;
return ret;
}

/* Selected the highest available SPI clock speed that is still below the MM6108's maximum of
Expand All @@ -105,7 +126,11 @@ static void wlan_hal_spi_init(void)
ret = spi_bus_add_device(SPI2_HOST, &dev_cfg, &spi_handle);
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

@morse-arienjudge morse-arienjudge Jul 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is documenting the change.
Personally I think the error handling you've implemented documents itself.

* passing it to spi_device_get_actual_freq() below. */
ESP_LOGE(TAG, "spi_bus_add_device failed: %s", esp_err_to_name(ret));
spi_handle = NULL;
return ret;
}

/* The actual clock frequency may not be the one that was set as it is re-calculated by the
Expand All @@ -114,20 +139,29 @@ static void wlan_hal_spi_init(void)
int actual_freq_khz = 0;
spi_device_get_actual_freq(spi_handle, &actual_freq_khz);
ESP_LOGD(TAG, "Actual SPI CLK %dkHz\n", actual_freq_khz);

return ESP_OK;
}

static void wlan_hal_spi_deinit(void)
{
esp_err_t ret = spi_bus_remove_device(spi_handle);
if (ret != ESP_OK)
esp_err_t ret;

/* spi_handle is NULL when wlan_hal_spi_init() failed before adding the device. */
if (spi_handle != NULL)
{
ESP_LOGE(TAG, "spi_bus_remove_device failed\n");
ret = spi_bus_remove_device(spi_handle);
if (ret != ESP_OK)
{
ESP_LOGE(TAG, "spi_bus_remove_device failed: %s", esp_err_to_name(ret));
}
spi_handle = NULL;
}

ret = spi_bus_free(SPI2_HOST);
if (ret != ESP_OK)
{
ESP_LOGE(TAG, "spi_bus_initialize failed\n");
ESP_LOGE(TAG, "spi_bus_free failed: %s", esp_err_to_name(ret));
}
}

Expand Down Expand Up @@ -245,7 +279,13 @@ void mmhal_wlan_set_spi_irq_enabled(bool enabled)
void mmhal_wlan_init(void)
{
wlan_hal_gpio_init();
wlan_hal_spi_init();
esp_err_t ret = wlan_hal_spi_init();
if (ret != ESP_OK)
{
/* This HAL entry point cannot report failure upward, so record it here. Without this the
* only symptom is a later, causeless "Transport init failed" from mmdrv_init(). */
ESP_LOGE(TAG, "SPI init failed (%s), transceiver will be unreachable", esp_err_to_name(ret));
}
/* Raise the RESET_N line to enable the WLAN transceiver. */
gpio_set_level(CONFIG_MM_RESET_N, 1);
}
Expand Down