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

[Ferry Toth <fntoth@gmail.com>]

Hi

Op 05-04-2024 om 13:38 schreef Hardik Gajjar:
> On Wed, Apr 03, 2024 at 11:01:58PM +0200, Ferry Toth wrote:
>> 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://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?

>>>> Thanks,
>>>> Hardik