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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
