[PATCH] net: eth-uclass: change state before stop() in eth_halt()

[Niel Fourie]

Hi Marek,

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.

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.

Best regards,
Niel Fourie

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-21 Fax: +49-8142-66989-80  Email: lusus at denx.de