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