Hello,
On Mon, Oct 10, 2011 at 06:38:07PM +0200, Marc Kleine-Budde wrote:
> The inline documentation of _gadget_stop_activity() states that the
> function should be called holding the udc->lock. This however will
> result in a deadlock, because _gadget_stop_activity() takes the udc->lock.
>
> During normal operation _gadget_stop_activity() is always called unlocked,
> but in ci13xxx_stop() it's called locked, this results in the following
> deadlock during rmmod of a gadget driver.
>
> This patch fixes the deadlock by calling _gadget_stop_activity() always
> unlocked, the inline documentation is adjusted accordingly.
>
> =============================================
> [ INFO: possible recursive locking detected ]
> 3.1.0-rc6+ #159
> ---------------------------------------------
> rmmod/121 is trying to acquire lock:
> (udc_lock){-.-...}, at: [<c0229048>] _gadget_stop_activity+0x18/0x154
>
> but task is already holding lock:
> (udc_lock){-.-...}, at: [<c02291e0>] ci13xxx_stop+0x5c/0x164
>
> other info that might help us debug this:
> Possible unsafe locking scenario:
>
> CPU0
> ----
> lock(udc_lock);
> lock(udc_lock);
>
> *** DEADLOCK ***
>
> May be due to missing lock nesting notation
>
> 2 locks held by rmmod/121:
> #0: (udc_lock#2){+.+.+.}, at: [<c02286c0>] usb_gadget_unregister_driver+0x34/0x88
> #1: (udc_lock){-.-...}, at: [<c02291e0>] ci13xxx_stop+0x5c/0x164
>
> stack backtrace:
> [<c000d41c>] (unwind_backtrace+0x0/0xf0) from [<c0056f94>] (check_deadlock.clone.24+0x284/0x2c4)
> [<c0056f94>] (check_deadlock.clone.24+0x284/0x2c4) from [<c00589ac>] (validate_chain.clone.25+0x430/0x6fc)
> [<c00589ac>] (validate_chain.clone.25+0x430/0x6fc) from [<c0059bac>] (__lock_acquire+0x494/0x8f0)
> [<c0059bac>] (__lock_acquire+0x494/0x8f0) from [<c005a698>] (lock_acquire+0x98/0x1a8)
> [<c005a698>] (lock_acquire+0x98/0x1a8) from [<c02f12a4>] (_raw_spin_lock_irqsave+0x64/0xa0)
> [<c02f12a4>] (_raw_spin_lock_irqsave+0x64/0xa0) from [<c0229048>] (_gadget_stop_activity+0x18/0x154)
> [<c0229048>] (_gadget_stop_activity+0x18/0x154) from [<c0229234>] (ci13xxx_stop+0xb0/0x164)
> [<c0229234>] (ci13xxx_stop+0xb0/0x164) from [<c022867c>] (usb_gadget_remove_driver+0x88/0x98)
> [<c022867c>] (usb_gadget_remove_driver+0x88/0x98) from [<c02286f4>] (usb_gadget_unregister_driver+0x68/0x88)
> [<c02286f4>] (usb_gadget_unregister_driver+0x68/0x88) from [<c0065f2c>] (sys_delete_module+0x1fc/0x26c)
> [<c0065f2c>] (sys_delete_module+0x1fc/0x26c) from [<c00092a0>] (ret_fast_syscall+0x0/0x38)
> BUG: spinlock lockup on CPU#0, rmmod/121, c05b1644
> [<c000d41c>] (unwind_backtrace+0x0/0xf0) from [<c01da000>] (do_raw_spin_lock+0x128/0x144)
> [<c01da000>] (do_raw_spin_lock+0x128/0x144) from [<c02f12c8>] (_raw_spin_lock_irqsave+0x88/0xa0)
> [<c02f12c8>] (_raw_spin_lock_irqsave+0x88/0xa0) from [<c0229048>] (_gadget_stop_activity+0x18/0x154)
> [<c0229048>] (_gadget_stop_activity+0x18/0x154) from [<c0229234>] (ci13xxx_stop+0xb0/0x164)
> [<c0229234>] (ci13xxx_stop+0xb0/0x164) from [<c022867c>] (usb_gadget_remove_driver+0x88/0x98)
> [<c022867c>] (usb_gadget_remove_driver+0x88/0x98) from [<c02286f4>] (usb_gadget_unregister_driver+0x68/0x88)
> [<c02286f4>] (usb_gadget_unregister_driver+0x68/0x88) from [<c0065f2c>] (sys_delete_module+0x1fc/0x26c)
> [<c0065f2c>] (sys_delete_module+0x1fc/0x26c) from [<c00092a0>] (ret_fast_syscall+0x0/0x38)
>
> Signed-off-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
> ---
> drivers/usb/gadget/ci13xxx_udc.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/usb/gadget/ci13xxx_udc.c b/drivers/usb/gadget/ci13xxx_udc.c
> index ae6c001..4241241 100644
> --- a/drivers/usb/gadget/ci13xxx_udc.c
> +++ b/drivers/usb/gadget/ci13xxx_udc.c
> @@ -1613,7 +1613,6 @@ __acquires(mEp->lock)
> * @gadget: gadget
> *
> * This function returns an error code
> - * Caller must hold lock
+ * Caller must not hold lock
?
> */
> static int _gadget_stop_activity(struct usb_gadget *gadget)
> {
> @@ -2707,7 +2706,9 @@ static int ci13xxx_stop(struct usb_gadget_driver *driver)
> if (udc->udc_driver->notify_event)
> udc->udc_driver->notify_event(udc,
> CI13XXX_CONTROLLER_STOPPED_EVENT);
> + spin_unlock_irqrestore(udc->lock, flags);
> _gadget_stop_activity(&udc->gadget);
> + spin_lock_irqsave(udc->lock, flags);
> pm_runtime_put(&udc->gadget.dev);
> }
These constructs tend to be not avery robust. Did you check that nothing
relevant can happen while you released the lock?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
