On 12/5/22 16:33, Niel Fourie wrote:
Hi Marek,
Hi,
On 05/12/2022 13:06, Niel Fourie wrote:
[...]
It does however show that this patch introduces a bug -- this patch
changes the order in which priv->state = ETH_STATE_PASSIVE; is
assigned from _after_ the ->stop callback to _before_ the -> stop
callback. This breaks drivers/net/ldpaa_eth/ldpaa_eth.c which checks
the priv->state in its ->stop callback, either on its own in non-DM
case, or in eth_is_active() implementation in DM case. With this
patch, the interface would never be stopped in the ->stop callback,
because the condition (net_dev->state == ETH_STATE_PASSIVE) test in
the ldpaa stop callback implementation would always be true.
In drivers/net/ldpaa_eth/ldpaa_eth.c:ldpaa_eth_stop(), priv is of type
struct ldpaa_eth_priv*, defined in drivers/net/ldpaa_eth/ldpaa_eth.h
and is accessed using dev_get_priv().
In net/eth-uclass.c:eht_halt(), priv is of type struct
eth_device_priv* and defined in the same .c file, and is accessed
using dev_get_uclass_priv(). As the structure is local to this file,
nothing outside of this file should have any knowledge of its
contents, and changing of the order of the calls should only impact
this file.
I sincerely hope that these two are not interfering with each other,
otherwise we have much bigger problems...
Shucks, I was thrown off by the the fact that net_dev is of type struct
eth_device, and its member state is separate from struct
eth_device_state and its member state, that I missed the implication of
eth_is_active() *setting* the value of struct eth_device_priv's state
not *reading* it.
Well spotted, you are correct. The patch in its current form would
introduce that bug. Thank you for finding that.
Good, so we agree this patch introduces a bug.
Adding back the call to dev_get_uclass_priv() to get priv and validating
it again *after* stop() as it was done before commit fa795f45254 ("net:
eth-uclass: avoid running start() twice without stop()") would fix this,
and perhaps also make the issue with stop() and Ethernet gadget more
obvious. A comment on why it needs to be repeated would also be useful.
Would this be an acceptable improvement?
I agree that fixing the USB ethernet gadget is still the best solution,
but until that happens, we could at least limit everyone's pain.
See my reply to the previous email.
Keep the usb gadget device around, that should not be hard to implement
and that should fix this problem once and for all, and for the future too.
