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

[Marek Vasut]

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.