net/usbnet: avoid recursive locking in usbnet_stop()
Subsystems (5)
PATCH 1/1 net/usbnet: avoid recursive locking in usbnet_stop()
|kernel BUG at kernel/rtmutex.c:724! |[<c029599c>] (rt_spin_lock_slowlock+0x108/0x2bc) from [<c01c2330>] (defer_bh+0x1c/0xb4) |[<c01c2330>] (defer_bh+0x1c/0xb4) from [<c01c3afc>] (rx_complete+0x14c/0x194) |[<c01c3afc>] (rx_complete+0x14c/0x194) from [<c01cac88>] (usb_hcd_giveback_urb+0xa0/0xf0) |[<c01cac88>] (usb_hcd_giveback_urb+0xa0/0xf0) from [<c01e1ff4>] (musb_giveback+0x34/0x40) |[<c01e1ff4>] (musb_giveback+0x34/0x40) from [<c01e2b1c>] (musb_advance_schedule+0xb4/0x1c0) |[<c01e2b1c>] (musb_advance_schedule+0xb4/0x1c0) from [<c01e2ca8>] (musb_cleanup_urb.isra.9+0x80/0x8c) |[<c01e2ca8>] (musb_cleanup_urb.isra.9+0x80/0x8c) from [<c01e2ed0>] (musb_urb_dequeue+0xec/0x108) |[<c01e2ed0>] (musb_urb_dequeue+0xec/0x108) from [<c01cbb90>] (unlink1+0xbc/0xcc) |[<c01cbb90>] (unlink1+0xbc/0xcc) from [<c01cc2ec>] (usb_hcd_unlink_urb+0x54/0xa8) |[<c01cc2ec>] (usb_hcd_unlink_urb+0x54/0xa8) from [<c01c2a84>] (unlink_urbs.isra.17+0x2c/0x58) |[<c01c2a84>] (unlink_urbs.isra.17+0x2c/0x58) from [<c01c2b44>] (usbnet_terminate_urbs+0x94/0x10c) |[<c01c2b44>] (usbnet_terminate_urbs+0x94/0x10c) from [<c01c2d68>] (usbnet_stop+0x100/0x15c) |[<c01c2d68>] (usbnet_stop+0x100/0x15c) from [<c020f718>] (__dev_close_many+0x94/0xc8)
defer_bh() takes the lok which is hold during unlink_urbs(). The safe walk suggest that the skb will be removed from the list and this is done by defer_bh() so it seems to be okay to drop the lock here.
--- drivers/net/usb/usbnet.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/net/usb/usbnet.c +2 -0
@@ -589,6 +589,7 @@ static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q) entry = (struct skb_data *) skb->cb; urb = entry->urb; +spin_unlock_irqrestore (&q->lock, flags);// during some PM-driven resume scenarios, // these (async) unlinks complete immediately retval =usb_unlink_urb (urb);
@@ -596,6 +597,7 @@ static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q)netdev_dbg (dev->net,"unlink urb err, %d\n" , retval); else count++; +spin_lock_irqsave (&q->lock, flags); }spin_unlock_irqrestore (&q->lock, flags);return count;
Signature · 5 lines
1.7.9 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Montag, 20. Februar 2012, 17:01:48 schrieb Sebastian Andrzej Siewior:
Quoted 15 lines
|kernel BUG at kernel/rtmutex.c:724! |[<c029599c>] (rt_spin_lock_slowlock+0x108/0x2bc) from [<c01c2330>] (defer_bh+0x1c/0xb4) |[<c01c2330>] (defer_bh+0x1c/0xb4) from [<c01c3afc>] (rx_complete+0x14c/0x194) |[<c01c3afc>] (rx_complete+0x14c/0x194) from [<c01cac88>] (usb_hcd_giveback_urb+0xa0/0xf0) |[<c01cac88>] (usb_hcd_giveback_urb+0xa0/0xf0) from [<c01e1ff4>] (musb_giveback+0x34/0x40) |[<c01e1ff4>] (musb_giveback+0x34/0x40) from [<c01e2b1c>] (musb_advance_schedule+0xb4/0x1c0) |[<c01e2b1c>] (musb_advance_schedule+0xb4/0x1c0) from [<c01e2ca8>] (musb_cleanup_urb.isra.9+0x80/0x8c) |[<c01e2ca8>] (musb_cleanup_urb.isra.9+0x80/0x8c) from [<c01e2ed0>] (musb_urb_dequeue+0xec/0x108) |[<c01e2ed0>] (musb_urb_dequeue+0xec/0x108) from [<c01cbb90>] (unlink1+0xbc/0xcc) |[<c01cbb90>] (unlink1+0xbc/0xcc) from [<c01cc2ec>] (usb_hcd_unlink_urb+0x54/0xa8) |[<c01cc2ec>] (usb_hcd_unlink_urb+0x54/0xa8) from [<c01c2a84>] (unlink_urbs.isra.17+0x2c/0x58) |[<c01c2a84>] (unlink_urbs.isra.17+0x2c/0x58) from [<c01c2b44>] (usbnet_terminate_urbs+0x94/0x10c) |[<c01c2b44>] (usbnet_terminate_urbs+0x94/0x10c) from [<c01c2d68>] (usbnet_stop+0x100/0x15c) |[<c01c2d68>] (usbnet_stop+0x100/0x15c) from [<c020f718>] (__dev_close_many+0x94/0xc8)
Quoted 3 lines
defer_bh()takes the lok which is hold duringunlink_urbs(). The safe walk suggest that the skb will be removed from the list and this is done bydefer_bh()so it seems to be okay to drop the lock here.
I am afraid there's something wrong in the hcd driver. Async unlink must be possible with a lock held. I cannot approve this patch.
Regards
Oliver
A message that is not in the archive (sent off-list or not yet received): 201202201721.38136.oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org
Quoted 6 lines · Oliver Neukum
Quoted 3 lines
defer_bh()takes the lok which is hold duringunlink_urbs(). The safe walk suggest that the skb will be removed from the list and this is done bydefer_bh()so it seems to be okay to drop the lock here.I am afraid there's something wrong in the hcd driver. Async unlink must be possible with a lock held. I cannot approve this patch.
Hmmm. The comment above unlink() says that. Looking through other hcds it seems that musb is not the only one doing it wrong. Oh well...
Quoted 3 lines
Regards
Oliver
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 10 lines · Sebastian Andrzej Siewior
Quoted 6 lines · Oliver Neukum
Quoted 3 lines
defer_bh()takes the lok which is hold duringunlink_urbs(). The safe walk suggest that the skb will be removed from the list and this is done bydefer_bh()so it seems to be okay to drop the lock here.I am afraid there's something wrong in the hcd driver. Async unlink must be possible with a lock held. I cannot approve this patch.
Hmmm. The comment above
unlink()says that. Looking through other hcds it seems that musb is not the only one doing it wrong. Oh well...
What's the issue here?
If a driver calls usb_unlink_urb() while holding a lock, and the completion routine tries to acquire the same lock, then deadlock is possible. The fact that usb_unlink_urb() is asynchronous is not a guarantee of anything; the HCD is allowed to call the completion handler from within usb_unlink_urb().
It's true that the kerneldoc for usb_unlink_urb() says "This request is always asynchronous". It might be a good idea to remove the word "always", because it seems to give people the wrong idea.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): Pine.LNX.4.44L0.1202201524290.11040-100000-pYrvlCTfrz9XsRXLowluHWD2FQJk+8+b@public.gmane.org
Quoted 21 lines · Alan Stern
Quoted 10 lines · Sebastian Andrzej Siewior
Quoted 6 lines · Oliver Neukum
Quoted 3 lines
defer_bh()takes the lok which is hold duringunlink_urbs(). The safe walk suggest that the skb will be removed from the list and this is done bydefer_bh()so it seems to be okay to drop the lock here.I am afraid there's something wrong in the hcd driver. Async unlink must be possible with a lock held. I cannot approve this patch.
Hmmm. The comment above
unlink()says that. Looking through other hcds it seems that musb is not the only one doing it wrong. Oh well...What's the issue here?
If a driver calls
usb_unlink_urb()while holding a lock, and the completion routine tries to acquire the same lock, then deadlock is possible. The fact thatusb_unlink_urb()is asynchronous is not a guarantee of anything; the HCD is allowed to call the completion handler from withinusb_unlink_urb().
Quoted 3 lines
It's true that the kerneldoc for
usb_unlink_urb()says "This request is always asynchronous". It might be a good idea to remove the word "always", because it seems to give people the wrong idea.
I see. So you approve that patch and suggest to remove the "always" wording plus adding something like "the hcd might call complete routine during unlink" ?
Quoted 3 lines
Alan Stern
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 8 lines · Sebastian Andrzej Siewior
Quoted 8 lines
What's the issue here?
If a driver calls
usb_unlink_urb()while holding a lock, and the completion routine tries to acquire the same lock, then deadlock is possible. The fact thatusb_unlink_urb()is asynchronous is not a guarantee of anything; the HCD is allowed to call the completion handler from withinusb_unlink_urb().
Quoted 7 lines
Quoted 3 lines
It's true that the kerneldoc for
usb_unlink_urb()says "This request is always asynchronous". It might be a good idea to remove the word "always", because it seems to give people the wrong idea.I see. So you approve that patch and suggest to remove the "always" wording plus adding something like "the hcd might call complete routine during unlink" ?
Well, I haven't read the patch and don't really understand what issue it tries to solve. But if that issue is the one I talked about above then yes, it makes sense. And changing the documentation as you suggest would be a good thing to do in any case.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): Pine.LNX.4.44L0.1202201638330.13111-100000-pYrvlCTfrz9XsRXLowluHWD2FQJk+8+b@public.gmane.org
PATCH 1/1 usb/core: remove "always" from usb_unlink_urb() kernel doc entry
The kernel doc entry for usb_unlink_urb() contains the phrase "This request is always asynchronous.". The "always" leads to the assumption that the ->complete() callback is not called from within usb_unlink_urb(). This is not true. The HCD is allowed to call the ->complete() from within ->urb_dequeue() if it is appropriate for the hardware.
This patch updates the kernel doc so usb-device driver authors make sure to drop all locks (and make sure it is okay to drop them) which are acquired by the complete callback before calling usb_unlink_urb().
--- drivers/usb/core/urb.c | 9 +++++---- 1 files changed, 5 insertions(+), 4 deletions(-)
drivers/usb/core/urb.c +5 -4
@@ -527,10 +527,11 @@ EXPORT_SYMBOL_GPL(usb_submit_urb);* a driver's I/O routines to insure that all URB-related activity has * completed before it returns. * -* This request is always asynchronous. Success is indicated by -* returning -EINPROGRESS, at which time the URB will probably not yet -* have been given back to the device driver. When it is eventually -* called, the completion function will see @urb->status == -ECONNRESET. +* This request is asynchronous, however the hcd might call the ->complete() +* callback during unlink. Success is indicated by returning -EINPROGRESS, at +* which time the URB will probably not yet have been given back to the device +* driver. When it is eventually called, the completion function will see +* @urb->status == -ECONNRESET. * Failure is indicated by usb_unlink_urb() returning any other value. * Unlinking will fail when @urb is not currently "linked" (i.e., it was * never submitted, or it was unlinked before, or the hardware is already
Signature · 6 lines
1.7.9 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH] usb/core: remove "always" from usb_unlink_urb() kernel doc entry
Quoted 20 lines · Sebastian Andrzej Siewior
The kernel doc entry for
usb_unlink_urb()contains the phrase "This request is always asynchronous.". The "always" leads to the assumption that the ->complete() callback is not called from withinusb_unlink_urb(). This is not true. The HCD is allowed to call the ->complete() from within ->urb_dequeue() if it is appropriate for the hardware.
This patch updates the kernel doc so usb-device driver authors make sure to drop all locks (and make sure it is okay to drop them) which are acquired by the complete callback before callingusb_unlink_urb().Signed-off-by Sebastian Andrzej Siewior <[email protected]>--- drivers/usb/core/urb.c | 9 +++++---- 1 files changed, 5 insertions(+), 4 deletions(-)drivers/usb/core/urb.c +0
-0
drivers/usb/core/urb.c EXPORT_SYMBOL_GPL(usb_submit_urb); · line 531
Quoted 9 lines
drivers/usb/core/urb.c +2
-4@@ -527,10 +527,11 @@ EXPORT_SYMBOL_GPL(usb_submit_urb);* a driver's I/O routines to insure that all URB-related activity has * completed before it returns. * -* This request is always asynchronous. Success is indicated by -* returning -EINPROGRESS, at which time the URB will probably not yet -* have been given back to the device driver. When it is eventually -* called, the completion function will see @urb->status == -ECONNRESET. +* This request is asynchronous, however the hcd might call the ->complete() +* callback during unlink.
"Therefore when drivers call usb_unlink_urb(), they must not hold any locks that may be taken by the completion function."
Also, we write "HCD" in capital letters.
Quoted 7 lines
drivers/usb/core/urb.c +3
-0Success is indicated by returning -EINPROGRESS, at + * which time the URB will probably not yet have been given back to the device + * driver. When it is eventually called, the completion function will see + * @urb->status == -ECONNRESET. * Failure is indicated by usb_unlink_urb() returning any other value. * Unlinking will fail when @urb is not currently "linked" (i.e., it was* never submitted, or it was unlinked before, or the hardware is already
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): Pine.LNX.4.44L0.1202221525210.1311-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org
PATCH v2 1/1 usb/core: remove "always" from usb_unlink_urb() kernel doc entry
The kernel doc entry for usb_unlink_urb() contains the phrase "This request is always asynchronous.". The "always" leads to the assumption that the ->complete() callback is not called from within usb_unlink_urb(). This is not true. The HCD is allowed to call the ->complete() from within ->urb_dequeue() if it is appropriate for the hardware.
This patch updates the kernel doc so usb-device driver authors make sure to drop all locks (and make sure it is okay to drop them) which are acquired by the complete callback before calling usb_unlink_urb().
---
v1..v2:
- add Alan's suggestion
- s/hcd/HCD
drivers/usb/core/urb.c | 11 +++++++---- 1 files changed, 7 insertions(+), 4 deletions(-)
drivers/usb/core/urb.c +7 -4
@@ -527,10 +527,13 @@ EXPORT_SYMBOL_GPL(usb_submit_urb);* a driver's I/O routines to insure that all URB-related activity has * completed before it returns. * -* This request is always asynchronous. Success is indicated by -* returning -EINPROGRESS, at which time the URB will probably not yet -* have been given back to the device driver. When it is eventually -* called, the completion function will see @urb->status == -ECONNRESET. +* This request is asynchronous, however the HCD might call the ->complete() +* callback during unlink. Therefore when drivers call usb_unlink_urb(), they +* must not hold any locks that may be taken by the completion function. +* Success is indicated by returning -EINPROGRESS, at which time the URB will +* probably not yet have been given back to the device driver. When it is +* eventually called, the completion function will see @urb->status == +* -ECONNRESET. * Failure is indicated by usb_unlink_urb() returning any other value. * Unlinking will fail when @urb is not currently "linked" (i.e., it was * never submitted, or it was unlinked before, or the hardware is already
Signature · 6 lines
1.7.9 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 16 lines · * Oliver Neukum | 2012-02-20 17:21:38 [+0100]:
Am Montag, 20. Februar 2012, 17:01:48 schrieb Sebastian Andrzej Siewior:
Quoted 15 lines
|kernel BUG at kernel/rtmutex.c:724! |[<c029599c>] (rt_spin_lock_slowlock+0x108/0x2bc) from [<c01c2330>] (defer_bh+0x1c/0xb4) |[<c01c2330>] (defer_bh+0x1c/0xb4) from [<c01c3afc>] (rx_complete+0x14c/0x194) |[<c01c3afc>] (rx_complete+0x14c/0x194) from [<c01cac88>] (usb_hcd_giveback_urb+0xa0/0xf0) |[<c01cac88>] (usb_hcd_giveback_urb+0xa0/0xf0) from [<c01e1ff4>] (musb_giveback+0x34/0x40) |[<c01e1ff4>] (musb_giveback+0x34/0x40) from [<c01e2b1c>] (musb_advance_schedule+0xb4/0x1c0) |[<c01e2b1c>] (musb_advance_schedule+0xb4/0x1c0) from [<c01e2ca8>] (musb_cleanup_urb.isra.9+0x80/0x8c) |[<c01e2ca8>] (musb_cleanup_urb.isra.9+0x80/0x8c) from [<c01e2ed0>] (musb_urb_dequeue+0xec/0x108) |[<c01e2ed0>] (musb_urb_dequeue+0xec/0x108) from [<c01cbb90>] (unlink1+0xbc/0xcc) |[<c01cbb90>] (unlink1+0xbc/0xcc) from [<c01cc2ec>] (usb_hcd_unlink_urb+0x54/0xa8) |[<c01cc2ec>] (usb_hcd_unlink_urb+0x54/0xa8) from [<c01c2a84>] (unlink_urbs.isra.17+0x2c/0x58) |[<c01c2a84>] (unlink_urbs.isra.17+0x2c/0x58) from [<c01c2b44>] (usbnet_terminate_urbs+0x94/0x10c) |[<c01c2b44>] (usbnet_terminate_urbs+0x94/0x10c) from [<c01c2d68>] (usbnet_stop+0x100/0x15c) |[<c01c2d68>] (usbnet_stop+0x100/0x15c) from [<c020f718>] (__dev_close_many+0x94/0xc8)
Quoted 6 lines
Quoted 3 lines
defer_bh()takes the lok which is hold duringunlink_urbs(). The safe walk suggest that the skb will be removed from the list and this is done bydefer_bh()so it seems to be okay to drop the lock here.I am afraid there's something wrong in the hcd driver. Async unlink must be possible with a lock held. I cannot approve this patch.
This argument is no longer valid, the documentation has been updated [0].
Is it okay for the patch to be merged?
Quoted 2 lines
Regards
Oliver
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller
maintainerFrom: Sebastian Andrzej Siewior <[email protected]>
Date: Mon, 5 Mar 2012 22:25:00 +0100
Quoted 1 line
Is it okay for the patch to be merged?
Just simply make a fresh submission of it.