Date: Wed, 3 Apr 2024 23:01:58 +0200
From: Ferry Toth <fntoth@...il.com>
To: Ferry Toth <fntoth@...il.com>, Hardik Gajjar <hgajjar@...adit-jv.com>,
 Andy Shevchenko <andriy.shevchenko@...el.com>
Cc: gregkh@...uxfoundation.org, s.hauer@...gutronix.de, jonathanh@...dia.com,
 linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
 quic_linyyuan@...cinc.com, paul@...pouillou.net, quic_eserrao@...cinc.com,
 erosca@...adit-jv.com
Subject: Re: [PATCH v4] usb: gadget: u_ether: Replace netif_stop_queue with
 netif_device_detach

Hi,

Op 15-01-2024 om 21:10 schreef Ferry Toth:
> Hi,
>
> Op 15-01-2024 om 14:27 schreef Hardik Gajjar:
>> On Sun, Jan 14, 2024 at 06:59:19PM +0200, Andy Shevchenko wrote:
>>> +Cc: Ferry.
>>>
>>> On Fri, Oct 06, 2023 at 05:56:46PM +0200, Hardik Gajjar wrote:
>>>> This patch replaces the usage of netif_stop_queue with 
>>>> netif_device_detach
>>>> in the u_ether driver. The netif_device_detach function not only 
>>>> stops all
>>>> tx queues by calling netif_tx_stop_all_queues but also marks the 
>>>> device as
>>>> removed by clearing the __LINK_STATE_PRESENT bit.
>>>>
>>>> This change helps notify user space about the disconnection of the 
>>>> device
>>>> more effectively, compared to netif_stop_queue, which only stops a 
>>>> single
>>>> transmit queue.
>>>
>>> This change effectively broke my USB ether setup.
>>>
>>> git bisect start
>>> # status: waiting for both good and bad commits
>>> # good: [1f24458a1071f006e3f7449c08ae0f12af493923] Merge tag 
>>> 'tty-6.7-rc1' of 
>>> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty
>>> git bisect good 1f24458a1071f006e3f7449c08ae0f12af493923
>>> # status: waiting for bad commit, 1 good commit known
>>> # bad: [2c40c1c6adab90ee4660caf03722b3a3ec67767b] Merge tag 
>>> 'usb-6.7-rc1' of 
>>> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb
>>> git bisect bad 2c40c1c6adab90ee4660caf03722b3a3ec67767b
>>> # bad: [17d6b82d2d6d467149874b883cdba844844b996d] usb/usbip: fix 
>>> wrong data added to platform device
>>> git bisect bad 17d6b82d2d6d467149874b883cdba844844b996d
>>> # good: [ba6b83a910b6d8a9379bda55cbf06cb945473a96] usb: xhci-mtk: 
>>> add a bandwidth budget table
>>> git bisect good ba6b83a910b6d8a9379bda55cbf06cb945473a96
>>> # good: [dddc00f255415b826190cfbaa5d6dbc87cd9ded1] Revert "usb: 
>>> gadget: uvc: cleanup request when not in correct state"
>>> git bisect good dddc00f255415b826190cfbaa5d6dbc87cd9ded1
>>> # bad: [8f999ce60ea3d47886b042ef1f22bb184b6e9c59] USB: typec: 
>>> tps6598x: Refactor tps6598x port registration
>>> git bisect bad 8f999ce60ea3d47886b042ef1f22bb184b6e9c59
>>> # bad: [f49449fbc21e7e9550a5203902d69c8ae7dfd918] usb: gadget: 
>>> u_ether: Replace netif_stop_queue with netif_device_detach
>>> git bisect bad f49449fbc21e7e9550a5203902d69c8ae7dfd918
>>> # good: [97475763484245916735a1aa9a3310a01d46b008] USB: usbip: fix 
>>> stub_dev hub disconnect
>>> git bisect good 97475763484245916735a1aa9a3310a01d46b008
>>> # good: [0f5aa1b01263b8b621bc4f031a1f2983ef8517b7] usb: usbtest: fix 
>>> a type promotion bug
>>> git bisect good 0f5aa1b01263b8b621bc4f031a1f2983ef8517b7
>>> # first bad commit: [f49449fbc21e7e9550a5203902d69c8ae7dfd918] usb: 
>>> gadget: u_ether: Replace netif_stop_queue with netif_device_detach
>>>
>>> Note, revert indeed helps. Should I send a revert?
>>>
>>> I use configfs to setup USB EEM function and it worked till this 
>>> commit.
>>> If needed, I can share my scripts, but I believe it's not needed as 
>>> here
>>> we see a clear regression.
>>>
>>> -- 
>>> With Best Regards,
>>> Andy Shevchenko
>>>
>>>
>>
>> Without this patch, there may be a potential crash in a race 
>> condition, as __LINK_STATE_PRESENT is monitored at many places in the 
>> Network stack to determine the status of the link.
>>
>> Could you please provide details on how this patch affects your 
>> functionality? Are you experiencing connection problems or data 
>> transfer interruptions?
>
> In my case on mrfld (Intel Edison Arduino) using configfs with this 
> patch no config from host through dhcp is received. Manual setting 
> correct ipv4 addr / mask / gw still no connection.
>
>> Instead of reverting this patch, consider trying the upcoming patch 
>> (soon to be available in the mainline) to see if it resolves your issue.
>>
>> https://lore.kernel.org/lkml/2023122900-commence-agenda-db2c@gregkh/T/#m36a812d3f1e5d744ee32381f6ae4185940b376de 
>>
>
> This patch works for me with v6.7.0.

I need to revisit this. The patch in this topic landed in v6.7.0-rc1 
(f49449fbc21e) and breaks the gadget mrfld (Intel Edison Arduino) and 
other platforms as well.

The mentioned fix "usb: gadget: u_ether: Re-attach netif device to 
mirror detachment*" * has landed in v6.8.0-rc1 (76c945730). What it does 
fix: I am able to make a USB EEM function again.

However, now a hidden issue appears. With mrfld there is an external 
switch to easily switch between host and device mode.

What is not fixed:

- when in device mode and unplugging/plugging the cable when using 
`ifconfig usb0` the line "usb0: 
flags=4163<UP,BROADCAST,RUNNING,MULTICAST>" changes to "usb0: 
flags=4099<UP,BROADCAST,MULTICAST>" as is supposed to, the route table 
is updated and the dir `/sys/class/net/usb0` exists and in the dir `cat 
carrier*` shows the carrier up and down counts. This is the expected 
behavior.

- when in device mode and switching to host mode `ifconfig usb0` 
continues to show "RUNNING", the route table is not modified and the dir 
`/sys/class/net/usb0` no longer exists.

- switching to device mode again, USB EEM works fine, no changes to 
RUNNING or the route table happen and the dir `/sys/class/net/usb0` 
still is non- existing.

- unplugging/plugging the cable in device mode after this does not 
restore the original situation.

This behavior I tested on v6.9.0-rc2 (with a few unrelated but essential 
patches on top) and bisected back to this patch in v6.70-rc1.

It seems `netif_device_detach` does not completely clean up as expected 
and `netif_device_attach` does not completely rebuild.

I am wondering if on other platforms this can be reproduced? If so, inho 
it would be best to revert the both patches until the issue is resolved.

Thanks,

Ferry

>> Thanks,
>> Hardik
>

Powered by blists - more mailing lists