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

[James Bottomley]

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@xxxxxxxxx> 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@xxxxxxxxx>
> > > > > > > ---
> > > > > > >  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