Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752263AbcDFNin (ORCPT ); Wed, 6 Apr 2016 09:38:43 -0400 Received: from e32.co.us.ibm.com ([32.97.110.150]:41381 "EHLO e32.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751742AbcDFNil (ORCPT ); Wed, 6 Apr 2016 09:38:41 -0400 X-IBM-Helo: d03dlp03.boulder.ibm.com X-IBM-MailFrom: jejb@linux.vnet.ibm.com X-IBM-RcptTo: linux-kernel@vger.kernel.org;linux-scsi@vger.kernel.org Message-ID: <1459949907.2372.13.camel@linux.vnet.ibm.com> Subject: Re: [PATCH] csiostor: Fix backwards locking in the function __csio_unreg_rnode From: James Bottomley To: Bastien Philbert , Julian Calaby Cc: "Martin K. Petersen" , linux-scsi , "linux-kernel@vger.kernel.org" Date: Wed, 06 Apr 2016 09:38:27 -0400 In-Reply-To: <57050D62.3090802@gmail.com> References: <1459891143-20451-1-git-send-email-bastienphilbert@gmail.com> <57050D62.3090802@gmail.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.16.5 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16040613-0005-0000-0000-000023908A29 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2012 Lines: 58 On Wed, 2016-04-06 at 09:21 -0400, Bastien Philbert wrote: > > On 2016-04-06 03:48 AM, Julian Calaby wrote: > > Hi Bastien, > > > > On Wed, Apr 6, 2016 at 7:19 AM, Bastien Philbert > > wrote: > > > This fixes backwards locking in the function __csio_unreg_rnode > > > to > > > properly lock before the call to the function csio_unreg_rnode > > > and > > > not unlock with spin_unlock_irq as this would not allow the > > > proper > > > protection for concurrent access on the shared csio_hw structure > > > pointer hw. In addition switch the locking after the critical > > > region > > > function call to properly unlock instead with spin_unlock_irq on > > > > > > Signed-off-by: Bastien Philbert > > > --- > > > drivers/scsi/csiostor/csio_rnode.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/scsi/csiostor/csio_rnode.c > > > b/drivers/scsi/csiostor/csio_rnode.c > > > index e9c3b04..029a09e 100644 > > > --- a/drivers/scsi/csiostor/csio_rnode.c > > > +++ b/drivers/scsi/csiostor/csio_rnode.c > > > @@ -580,9 +580,9 @@ __csio_unreg_rnode(struct csio_rnode *rn) > > > ln->last_scan_ntgts--; > > > } > > > > > > - spin_unlock_irq(&hw->lock); > > > - csio_unreg_rnode(rn); > > > spin_lock_irq(&hw->lock); > > > + csio_unreg_rnode(rn); > > > + spin_unlock_irq(&hw->lock); > > > > Are you _certain_ this is correct? This construct usually appears > > when > > a function has a particular lock held, then needs to unlock it to > > call > > some other function. Are you _certain_ that this isn't the case? > > > > Thanks, > > > Yes I am pretty certain this is correct. I checked the paths that > called this function > and it was weired that none of them gradded the spinlock before hand. That's not good enough. If your theory is correct, lockdep should be dropping an already unlocked assertion in this codepath ... do you see this? James