drivers: ethernet: eth_stm32_hal: fix API_V2 multicast

[kevinior]

After API_V2 auto-negotiation support was added by #86621 setup_mac_filter() was called before HAL_ETH_Init(), which resulted in received multicast packets being discarded.

This removes that call to setup_mac_filter() when API_V2 in enabled and calls it from eth_init_api_v2() instead, after HAL_ETH_Init().

I've verified the problem and tested the fix on a Nucleo-H563ZI with a simple application that just starts up and makes an mDNS query (which depends on working multicast).

[ghost]

I experience the multicast problem on main on an stm32h573i_dk.

I have the intention to test this patch. Thank you for it.

[ghost]

Will this not result in calling it twice on api v1 devices?

[kevinior]

eth_init_api_v2() is only called on API_V2 devices.

[marwaiehm-st]

LGTM

[maass-hamburg]

how about moving setup_mac_filter() into the iface init right before net_if_carrier_off(iface); for both apis?

[kevinior]

@maass-hamburg

how about moving setup_mac_filter() into the iface init right before net_if_carrier_off(iface); for both apis?

Seems like it would make it a bit neater, yes.

[kevinior]

@maass-hamburg

how about moving setup_mac_filter() into the iface init right before net_if_carrier_off(iface); for both apis?

Except eth_init_api_v2() is called from eth_iface_init(), which is an API function. eth_initialize() is called as the driver initialization function. So I'm not really clear if that's a safe change.

[ghost]

how about moving setup_mac_filter() into the iface init right before net_if_carrier_off(iface); for both apis?

Except eth_init_api_v2() is called from eth_iface_init(), which is an API function. eth_initialize() is called as the driver initialization function. So I'm not really clear if that's a safe change.

I had the same thought. It is not clear to me, why HAL_ETH_Init is called from the API function for V2 and from the driver initialization for V1.

[ghost]

how about moving setup_mac_filter() into the iface init right before net_if_carrier_off(iface); for both apis?

Except eth_init_api_v2() is called from eth_iface_init(), which is an API function. eth_initialize() is called as the driver initialization function. So I'm not really clear if that's a safe change.

I had the same thought. It is not clear to me, why HAL_ETH_Init is called from the API function for V2 and from the driver initialization for V1.

I traced the call to the api init.

Its here

zephyr/subsys/net/ip/net_core.c

Line 647 in d0fdd38

net_if_init();

Calls go through net_if_init -> init_iface -> api->init(.... with prio POST_KERNEL at CONFIG_NET_INIT_PRIO.

[ghost]

I tested the patch in this state including:

diff --git a/drivers/ethernet/eth_stm32_hal.c b/drivers/ethernet/eth_stm32_hal.c
index 1ec5bb1f05a..bec5dae50ac 100644
--- a/drivers/ethernet/eth_stm32_hal.c
+++ b/drivers/ethernet/eth_stm32_hal.c
@@ -1192,6 +1192,7 @@ static void eth_iface_init(struct net_if *iface)
        const struct device *dev;
        struct eth_stm32_hal_dev_data *dev_data;
        bool is_first_init = false;
+       ETH_HandleTypeDef *heth;

        __ASSERT_NO_MSG(iface != NULL);

@@ -1201,6 +1202,9 @@ static void eth_iface_init(struct net_if *iface)
        dev_data = dev->data;
        __ASSERT_NO_MSG(dev_data != NULL);

+       heth = &dev_data->heth;
+       __ASSERT_NO_MSG(heth != NULL);
+
        if (dev_data->iface == NULL) {
                dev_data->iface = iface;
                is_first_init = true;

Which solved the problem for me.

[ghost]

I tested the patch in this state including:

FYI @kevinior : undefined ETH_HandleTypeDef *heth; is the cause of the test failure.

[kevinior]

@clamattia

I tested the patch in this state including:

FYI @kevinior : undefined ETH_HandleTypeDef *heth; is the cause of the test failure.

Thanks, I'll fix it in the morning.

[kevinior]

Rebased onto latest main to force a CI re-run, since this unrelated test had failed.

[ghost]

Could we get another (relevant, mine unfortunately not :) ) review on this?
@marwaiehm-st maybe?

[pdgendt]

Could we get another (relevant, mine unfortunately not :) ) review on this?

It needs assignee approval.

[marwaiehm-st]

You need to update the commit message
Otherwise LGTM

[kevinior]

@marwaiehm-st

You need to update the commit message Otherwise LGTM

Is the commit message OK now?