Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752298AbcDFROy (ORCPT ); Wed, 6 Apr 2016 13:14:54 -0400 Received: from e35.co.us.ibm.com ([32.97.110.153]:46485 "EHLO e35.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751016AbcDFROw (ORCPT ); Wed, 6 Apr 2016 13:14:52 -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: <1459962881.2372.40.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 13:14:41 -0400 In-Reply-To: <57051EF3.2070508@gmail.com> References: <1459891143-20451-1-git-send-email-bastienphilbert@gmail.com> <57050D62.3090802@gmail.com> <1459949907.2372.13.camel@linux.vnet.ibm.com> <57051913.4050304@gmail.com> <1459952680.2372.25.camel@linux.vnet.ibm.com> <57051EF3.2070508@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: 16040617-0013-0000-0000-000025152C5F Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4286 Lines: 116 On Wed, 2016-04-06 at 10:36 -0400, Bastien Philbert wrote: > > On 2016-04-06 10:24 AM, James Bottomley wrote: > > On Wed, 2016-04-06 at 10:11 -0400, Bastien Philbert wrote: > > > > > > On 2016-04-06 09:38 AM, James Bottomley wrote: > > > > 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 < > > > > > > > bastienphilbert@gmail.com> > > > > > > > --- > > > > > > > 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 > > > > > > > > > > > Yes I do. > > > > You mean you don't see the lockdep assert, since you're asking to > > drop the patch? > > > > > For now just drop the patch but I am still concerned that we are > > > double unlocking here. > > > > Really, no. The pattern in the code indicates the lock is expected > > to be held. This can be wrong (sometimes code moves or people > > forget), but if it is wrong we'll get an assert about unlock of an > > already unlocked lock. If there's no assert, the lock is held on > > entry and the code is correct. > > > > You're proposing patches based on misunderstandings of the code > > which aren't backed up by actual issues and wasting everyone's time > > to look at them. Please begin with the hard evidence of a problem > > first, so post the lockdep assert in the changelog so we know > > there's a real problem. > > > > James > > > Certainly James. I think I just got carried away with the last few > patches :(. Is this Nick Krause? An email reply that Martin forwarded but the list didn't pick up (because it had a html part) suggests this. What you're doing is what got you banned from LKML the last time: sending patches without evidence there's a problem or understanding the code you're patching. Repeating the behaviour under a new identity isn't going to help improve your standing. James