Re: [PATCH v4] usb: gadget: u_ether: Replace netif_stop_queue with netif_device_detach

[Ferry Toth]

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