From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755220AbbCEKt5 (ORCPT ); Thu, 5 Mar 2015 05:49:57 -0500 Received: from galahad.ideasonboard.com ([185.26.127.97]:60781 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754945AbbCEKty (ORCPT ); Thu, 5 Mar 2015 05:49:54 -0500 From: Laurent Pinchart To: Sudeep Holla Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, Felipe Balbi , Greg Kroah-Hartman Subject: Re: [PATCH] usb: isp1760: fix possible deadlock in isp1760_udc_irq Date: Thu, 05 Mar 2015 12:49:58 +0200 Message-ID: <3620278.kWq4f83uKd@avalon> User-Agent: KMail/4.14.3 (Linux/3.18.7-gentoo; KDE/4.14.3; x86_64; ; ) In-Reply-To: <1425488877-1596-1-git-send-email-sudeep.holla@arm.com> References: <1425488877-1596-1-git-send-email-sudeep.holla@arm.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Sudeep, Thank you for the patch. On Wednesday 04 March 2015 17:07:57 Sudeep Holla wrote: > Use spin_{un,}lock_irq{save,restore} in isp1760_udc_{start,stop} to > prevent following potentially deadlock scenario between > isp1760_udc_{start,stop} and isp1760_udc_irq : > > ================================= > [ INFO: inconsistent lock state ] > 4.0.0-rc2-00004-gf7bb2ef60173 #51 Not tainted > --------------------------------- > inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage. > in:imklog/2118 [HC1[1]:SC0[0]:HE0:SE1] takes: > (&(&udc->lock)->rlock){?.+...}, at: [] > isp1760_udc_irq+0x367/0x9dc {HARDIRQ-ON-W} state was registered at: > [] _raw_spin_lock+0x23/0x30 > [] isp1760_udc_start+0x23/0xf8 > [] udc_bind_to_driver+0x71/0xb0 > [] usb_gadget_probe_driver+0x53/0x9c > [] usb_composite_probe+0x8a/0xa4 [libcomposite] > [] 0xbf8311a7 > [] do_one_initcall+0x8d/0x17c > [] do_init_module+0x49/0x148 > [] load_module+0xb7f/0xbc4 > [] SyS_finit_module+0x51/0x74 > [] ret_fast_syscall+0x1/0x68 > irq event stamp: 4966 > hardirqs last enabled at (4965): [] > _raw_spin_unlock_irq+0x1f/0x24 hardirqs last disabled at (4966): > [] __irq_svc+0x33/0x64 softirqs last enabled at (4458): > [] __do_softirq+0x23d/0x2d0 softirqs last disabled at (4389): > [] irq_exit+0xef/0x15c > > other info that might help us debug this: > Possible unsafe locking scenario: > > CPU0 > ---- > lock(&(&udc->lock)->rlock); > > lock(&(&udc->lock)->rlock); > > *** DEADLOCK *** > > 1 lock held by in:imklog/2118: > #0: (&f->f_pos_lock){+.+.+.}, at: [] __fdget_pos+0x31/0x34 > > Signed-off-by: Sudeep Holla > Cc: Laurent Pinchart > Cc: Greg Kroah-Hartman > Cc: Felipe Balbi > --- > drivers/usb/isp1760/isp1760-udc.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/isp1760/isp1760-udc.c > b/drivers/usb/isp1760/isp1760-udc.c index 6d618b3fab07..fbfbd59aae64 100644 > --- a/drivers/usb/isp1760/isp1760-udc.c > +++ b/drivers/usb/isp1760/isp1760-udc.c > @@ -1191,6 +1191,7 @@ static int isp1760_udc_start(struct usb_gadget > *gadget, struct usb_gadget_driver *driver) > { > struct isp1760_udc *udc = gadget_to_udc(gadget); > + unsigned long flags; > > /* The hardware doesn't support low speed. */ > if (driver->max_speed < USB_SPEED_FULL) { > @@ -1198,7 +1199,7 @@ static int isp1760_udc_start(struct usb_gadget > *gadget, return -EINVAL; > } > > - spin_lock(&udc->lock); > + spin_lock_irqsave(&udc->lock, flags); Strictly speaking spin_lock_irq() should be enough given that udc_start and udc_stop are called with interrupts enabled, but I suppose it doesn't hurt to be safe. I'll let you go with your preference. For both options, Acked-by: Laurent Pinchart > > if (udc->driver) { > dev_err(udc->isp->dev, "UDC already has a gadget driver\n"); > @@ -1208,7 +1209,7 @@ static int isp1760_udc_start(struct usb_gadget > *gadget, > > udc->driver = driver; > > - spin_unlock(&udc->lock); > + spin_unlock_irqrestore(&udc->lock, flags); > > dev_dbg(udc->isp->dev, "starting UDC with driver %s\n", > driver->function); > @@ -1232,6 +1233,7 @@ static int isp1760_udc_start(struct usb_gadget > *gadget, static int isp1760_udc_stop(struct usb_gadget *gadget) > { > struct isp1760_udc *udc = gadget_to_udc(gadget); > + unsigned long flags; > > dev_dbg(udc->isp->dev, "%s\n", __func__); > > @@ -1239,9 +1241,9 @@ static int isp1760_udc_stop(struct usb_gadget *gadget) > > isp1760_udc_write(udc, DC_MODE, 0); > > - spin_lock(&udc->lock); > + spin_lock_irqsave(&udc->lock, flags); > udc->driver = NULL; > - spin_unlock(&udc->lock); > + spin_unlock_irqrestore(&udc->lock, flags); > > return 0; > } -- Regards, Laurent Pinchart