Re: [PATCH] csiostor: Fix backwards locking in the function __csio_unreg_rnode

[James Bottomley <jejb@linux.vnet.ibm.com>]

On Wed, 2016-04-06 at 13:23 -0400, Bastien Philbert wrote:
> 
> On 2016-04-06 01:14 PM, James Bottomley wrote:
> > 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
> > > > > > > > <bastienphilbert@gmail.com> 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
> > 
> No I am not Nick Krause. I am just aware of how he got banned a few 
> years ago. That email was a mistake by typo and was hoping nobody 
> picked it up as they would then believe I was Nick Krause.

Hm, OK, but currently you are repeating his behaviour ... please don't
send any more patches until they're about real problems backed by
actual data.

Thanks,

James