From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755237AbbCEKwv (ORCPT ); Thu, 5 Mar 2015 05:52:51 -0500 Received: from service87.mimecast.com ([91.220.42.44]:35148 "EHLO service87.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754854AbbCEKwt convert rfc822-to-8bit (ORCPT ); Thu, 5 Mar 2015 05:52:49 -0500 Message-ID: <54F83587.2030407@arm.com> Date: Thu, 05 Mar 2015 10:52:55 +0000 From: Sudeep Holla User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Laurent Pinchart CC: Sudeep Holla , "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 References: <1425488877-1596-1-git-send-email-sudeep.holla@arm.com> <3620278.kWq4f83uKd@avalon> In-Reply-To: <3620278.kWq4f83uKd@avalon> X-OriginalArrivalTime: 05 Mar 2015 10:52:46.0716 (UTC) FILETIME=[83EE53C0:01D05732] X-MC-Unique: 115030510524704101 Content-Type: text/plain; charset=WINDOWS-1252; format=flowed Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/03/15 10:49, Laurent Pinchart wrote: > 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, > I agree, even I had similar thoughts but just played safe :) > Acked-by: Laurent Pinchart > Thanks. -- Regars, Sudeep