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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Sun, Apr 07, 2024 at 10:51:51PM +0200, Ferry Toth wrote:
> Op 05-04-2024 om 13:38 schreef Hardik Gajjar:
> > On Wed, Apr 03, 2024 at 11:01:58PM +0200, Ferry Toth wrote:
> > > Op 15-01-2024 om 21:10 schreef Ferry Toth:
> > > > Op 15-01-2024 om 14:27 schreef Hardik Gajjar:
> > > > > On Sun, Jan 14, 2024 at 06:59:19PM +0200, Andy Shevchenko wrote:
> > > > > > 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.
> > > > > > 
> > > > > 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://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_lkml_2023122900-2Dcommence-2Dagenda-2Ddb2c-40gregkh_T_-23m36a812d3f1e5d744ee32381f6ae4185940b376de&d=DwICaQ&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=SAhjP5GOmrADp1v_EE5jWoSuMlYCIt9gKduw-DCBPLs&m=4g6EtvkKKfp8YYHpU196b2HzQxCMgsAhuo8pFng3X4TCWdcOVEUCug2-l2hRfLyV&s=t82wZAFwm2FTSaacgsmSpZWvWEa89jN8GX-okIJrwFw&e=
> > > > > 
> > > > 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
> > I'm wondering why the /sys/class/net/usb0 directory is being removed when the network interface is still available.
> > This behavior seems not correct.
> 
> Exactly. And this didn't happen before the 2 patches.
> 
> To be precise: /sys/class/net/usb0 is not removed and it is a link, the link
> target /sys/devices/pci0000:00/0000:00:11.0/dwc3.0.auto/gadget.0/net/usb0 no
> longer exists
> 
> > The gether_cleanup function should remove the interface along with the associated kobject,
> > and this function should only be called during the unloading of the driver or deleting the gadget.
> > Could you please confirm whether any of your custom modifications are removing the net interface kobject?
> 
> As far as know not. I have one tusb1210 (usb phy) and 2 dwc3 patches,
> nothing related to gadgets or net.
> 
> For reference, patches are here: https://github.com/htot/meta-intel-edison/tree/nanbield/meta-intel-edison-bsp/recipes-kernel/linux/files
> 
> 0001-phy-ti-tusb1210-write-to-scratch-on-power-on.patch
> 
> 0001a-usb-dwc3-core-Fix-dwc3_core_soft_reset-before-anythi.patch
> 
> 044-REVERTME-usb-dwc3-gadget-skip-endpoints-ep-18-in-out.patch
> 
> Seems (just guessing) gether_cleanup is being called due to the switch to
> host mode, but cleanup only partly succeeds. Now I'm finding I can make the
> net interface disappear with `ip link set dev usb0 down` and the broken link
> goes away by destroying the gadget in configfs.
> 
> Is that intentional? Do I need to tear down the gadgets in reverse order as
> on create when switching to host mode? That would be new.
> 
> What happens when you trigger a switch to host mode without actually
> unplugging your cable?

Since there is no response, should we actually prepare revert next week?

-- 
With Best Regards,
Andy Shevchenko