diff mbox series

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

Message ID 20231006155646.12938-1-hgajjar@de.adit-jv.com
State New
Headers
Series [v4] usb: gadget: u_ether: Replace netif_stop_queue with netif_device_detach |

Commit Message

Hardik Gajjar Oct. 6, 2023, 3:56 p.m. UTC 
  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.

Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com>
---
Changes since version 1:
	- Correct Singed-off user name and e-mail

Changes since version 2:
	- Move change history below signed-off-by

Changes since version 3:
	- move netif_device_detach from eth_stop to gether_disconnect.
---
 drivers/usb/gadget/function/u_ether.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andy Shevchenko Jan. 14, 2024, 4:59 p.m. UTC | #1 
+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.
Hardik Gajjar Jan. 15, 2024, 1:27 p.m. UTC | #2 
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?
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

Thanks,
Hardik
Ferry Toth Jan. 15, 2024, 8:10 p.m. UTC | #3 
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.

> Thanks,
> Hardik
diff mbox series

Patch

diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
index 4bb0553da658..9d1c40c152d8 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -1200,7 +1200,7 @@  void gether_disconnect(struct gether *link)
 
 	DBG(dev, "%s\n", __func__);
 
-	netif_stop_queue(dev->net);
+	netif_device_detach(dev->net);
 	netif_carrier_off(dev->net);
 
 	/* disable endpoints, forcing (synchronous) completion