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 <[email protected]>
---
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);
/* Cleanup I/Os that were waiting for rnode to unregister */
if (cmpl)
--
2.5.0
Hi Bastien,
On Wed, Apr 6, 2016 at 7:19 AM, Bastien Philbert
<[email protected]> 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 <[email protected]>
> ---
> 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,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
On 2016-04-06 03:48 AM, Julian Calaby wrote:
> Hi Bastien,
>
> On Wed, Apr 6, 2016 at 7:19 AM, Bastien Philbert
> <[email protected]> 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 <[email protected]>
>> ---
>> 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.
Cheers,
Bastien
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
> > <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
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
>>> <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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. For now just drop the patch but I am still concerned that we are double unlocking here.
Bastien
>
>
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
> > > > <[email protected]> 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 <[email protected]>
> > > > > ---
> > > > > 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
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
>>>>> <[email protected]> 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 <[email protected]>
>>>>>> ---
>>>>>> 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 :(.
Bastien
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
> > > > > > <[email protected]> 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 <
> > > > > > > [email protected]>
> > > > > > > ---
> > > > > > > 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
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
>>>>>>> <[email protected]> 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 <
>>>>>>>> [email protected]>
>>>>>>>> ---
>>>>>>>> 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.
Bastien
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
> > > > > > > > <[email protected]> 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 <
> > > > > > > > > [email protected]>
> > > > > > > > > ---
> > > > > > > > > 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
On 2016-04-06 01:28 PM, James Bottomley wrote:
> 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
>>>>>>>>> <[email protected]> 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 <
>>>>>>>>>> [email protected]>
>>>>>>>>>> ---
>>>>>>>>>> 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
>
>
Ok sure I do have one patch that I tested and it worked for me but wasn't sure if I am just
trampling over the actual bug. If you would like me to send the patch and you can tell me
if I am right please let me known.
Sorry about the other patches,
Bastien
On Wed, Apr 06, 2016 at 01:28:24PM -0400, James Bottomley wrote:
> 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
> > > > > > > > > <[email protected]> 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 <
> > > > > > > > > > [email protected]>
> > > > > > > > > > ---
> > > > > > > > > > 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.
He's Nick, look at the email headers for proof.
James, and everyone else, please drop his patches. I'll go get him
banned from vger again.
Nick, please stop, you have violated the DCO now, a much worse thing
than before.
greg k-h
On Wed, Apr 06, 2016 at 11:41:49AM -0700, Greg KH wrote:
> > > > 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.
> > > >
> > > 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.
>
> He's Nick, look at the email headers for proof.
>
> James, and everyone else, please drop his patches. I'll go get him
> banned from vger again.
>
> Nick, please stop, you have violated the DCO now, a much worse thing
> than before.
Even if Bastien is going to try to claim that he happens to live in
the same house as Nick, the following two patches clearly shows that
even if Bastien is violating the DCO by using a fake sock puppet, he's
violated the DCO by failing to give Nick credit for writing the
identical patch.
- Ted