[PATCH] uas: replace WARN_ON_ONCE() with assert_spin_locked().

[PATCH] uas: replace WARN_ON_ONCE() with assert_spin_locked().

[Sanjeev Sharma]

spin_is_locked() always return false in uniprocessor configuration and therefore it
would be advise to repalce with assert_spin_locked().

Signed-off-by: Sanjeev Sharma <Sanjeev_Sharma@mentor.com>
---
 drivers/usb/storage/uas.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 3f42785..8e5877d 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -154,7 +154,7 @@ static void uas_mark_cmd_dead(struct uas_dev_info *devinfo,
 	struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp);
 
 	uas_log_cmd_state(cmnd, caller);
-	WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
+	assert_spin_locked(&devinfo->lock);
 	WARN_ON_ONCE(cmdinfo->state & COMMAND_ABORTED);
 	cmdinfo->state |= COMMAND_ABORTED;
 	cmdinfo->state &= ~IS_IN_WORK_LIST;
@@ -181,7 +181,7 @@ static void uas_add_work(struct uas_cmd_info *cmdinfo)
 	struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp);
 	struct uas_dev_info *devinfo = cmnd->device->hostdata;
 
-	WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
+	assert_spin_locked(&devinfo->lock);
 	cmdinfo->state |= IS_IN_WORK_LIST;
 	schedule_work(&devinfo->work);
 }
@@ -283,7 +283,7 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller)
 	struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
 	struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
 
-	WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
+	assert_spin_locked(&devinfo->lock);
 	if (cmdinfo->state & (COMMAND_INFLIGHT |
 			      DATA_IN_URB_INFLIGHT |
 			      DATA_OUT_URB_INFLIGHT |
@@ -622,7 +622,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 	struct urb *urb;
 	int err;
 
-	WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
+	assert_spin_locked(&devinfo->lock);
 	if (cmdinfo->state & SUBMIT_STATUS_URB) {
 		urb = uas_submit_sense_urb(cmnd, gfp, cmdinfo->stream);
 		if (!urb)
-- 
1.7.11.7

Re: [PATCH] uas: replace WARN_ON_ONCE() with assert_spin_locked().

[Greg KH]

On Mon, Aug 11, 2014 at 01:29:26PM +0530, Sanjeev Sharma wrote:
> spin_is_locked() always return false in uniprocessor configuration

Really?  On what processor type?  I don't see that being the case on a
few processors, but I didn't check them all, or I might be totally
confused with all of the arch_spin_is_locked() definitions in the tree.

> and therefore it
> would be advise to repalce with assert_spin_locked().

This implies that all uses of this function is wrong and should be
replaced and removed, right?

thanks,

greg k-h

RE: [PATCH] uas: replace WARN_ON_ONCE() with assert_spin_locked().

[Sharma, Sanjeev]

Hi Greg,

Please find my comment in line.

-----Original Message-----
From: Greg KH [mailto:gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org] 
Sent: Monday, August 11, 2014 1:58 PM
To: Sharma, Sanjeev
Cc: hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org; kraxel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org; mdharm-usb-JGfshJpz5UybPZpvUQj5UqxOck334EZe@public.gmane.org; linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: Re: [PATCH] uas: replace WARN_ON_ONCE() with assert_spin_locked().

On Mon, Aug 11, 2014 at 01:29:26PM +0530, Sanjeev Sharma wrote:
> spin_is_locked() always return false in uniprocessor configuration

Really?  On what processor type?  I don't see that being the case on a few processors, but I didn't check them all, or I might be totally confused with all of the arch_spin_is_locked() definitions in the tree.

specially in case of mips and powerpc but we can configure our kernel to uniprocessor configuration for specific processor and in that case we should be extra cautious. Please check spinlock_up.h

> and therefore it
> would be advise to repalce with assert_spin_locked().

This implies that all uses of this function is wrong and should be replaced and removed, right?

Yes and there are only few places in driver for which I am submitting patches which need to be change and at other places it has been already taken care. Please have an look into the
Old discussion for more details.

http://linux-kernel.2935.n7.nabble.com/Is-spin-is-locked-safe-to-use-with-BUG-ON-WARN-ON-td654800.html#a921802

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] uas: replace WARN_ON_ONCE() with assert_spin_locked().

[Guenter Roeck]

On Mon, Aug 11, 2014 at 01:29:26PM +0530, Sanjeev Sharma wrote:
> spin_is_locked() always return false in uniprocessor configuration and therefore it
> would be advise to repalce with assert_spin_locked().
> 
> Signed-off-by: Sanjeev Sharma <Sanjeev_Sharma@mentor.com>
> ---
>  drivers/usb/storage/uas.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 3f42785..8e5877d 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -154,7 +154,7 @@ static void uas_mark_cmd_dead(struct uas_dev_info *devinfo,
>  	struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp);
>  
>  	uas_log_cmd_state(cmnd, caller);
> -	WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
> +	assert_spin_locked(&devinfo->lock);

Seems to me that replacing WARN_ON_ONCE (which may be annoying but only
creates a traceback, and only once) with assert_spin_locked (which
crashes the kernel) is a bit drastic.

Guenter

Re: [PATCH] uas: replace WARN_ON_ONCE() with assert_spin_locked().

[Hans de Goede]

Hi,

On 08/11/2014 08:19 PM, Guenter Roeck wrote:
> On Mon, Aug 11, 2014 at 01:29:26PM +0530, Sanjeev Sharma wrote:
>> spin_is_locked() always return false in uniprocessor configuration and therefore it
>> would be advise to repalce with assert_spin_locked().
>>
>> Signed-off-by: Sanjeev Sharma <Sanjeev_Sharma@mentor.com>
>> ---
>>  drivers/usb/storage/uas.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
>> index 3f42785..8e5877d 100644
>> --- a/drivers/usb/storage/uas.c
>> +++ b/drivers/usb/storage/uas.c
>> @@ -154,7 +154,7 @@ static void uas_mark_cmd_dead(struct uas_dev_info *devinfo,
>>  	struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp);
>>  
>>  	uas_log_cmd_state(cmnd, caller);
>> -	WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
>> +	assert_spin_locked(&devinfo->lock);
> 
> Seems to me that replacing WARN_ON_ONCE (which may be annoying but only
> creates a traceback, and only once) with assert_spin_locked (which
> crashes the kernel) is a bit drastic.

I can see your point, but so far these paranoia checks have never triggered,
and having them trigger _always_ one some uni-processor (which is the reason
for this patch) to me seems the worse problem of the 2.

Ideally we would have a warn_spin_not_locked or such ...

Regards,

Hans

Re: [PATCH] uas: replace WARN_ON_ONCE() with assert_spin_locked().

[Hans de Goede]

Hi,

On 08/11/2014 09:59 AM, Sanjeev Sharma wrote:
> spin_is_locked() always return false in uniprocessor configuration and therefore it
> would be advise to repalce with assert_spin_locked().
> 
> Signed-off-by: Sanjeev Sharma <Sanjeev_Sharma@mentor.com>

Looks sane / good to me:

Acked-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
>  drivers/usb/storage/uas.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 3f42785..8e5877d 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -154,7 +154,7 @@ static void uas_mark_cmd_dead(struct uas_dev_info *devinfo,
>  	struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp);
>  
>  	uas_log_cmd_state(cmnd, caller);
> -	WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
> +	assert_spin_locked(&devinfo->lock);
>  	WARN_ON_ONCE(cmdinfo->state & COMMAND_ABORTED);
>  	cmdinfo->state |= COMMAND_ABORTED;
>  	cmdinfo->state &= ~IS_IN_WORK_LIST;
> @@ -181,7 +181,7 @@ static void uas_add_work(struct uas_cmd_info *cmdinfo)
>  	struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp);
>  	struct uas_dev_info *devinfo = cmnd->device->hostdata;
>  
> -	WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
> +	assert_spin_locked(&devinfo->lock);
>  	cmdinfo->state |= IS_IN_WORK_LIST;
>  	schedule_work(&devinfo->work);
>  }
> @@ -283,7 +283,7 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller)
>  	struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
>  	struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
>  
> -	WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
> +	assert_spin_locked(&devinfo->lock);
>  	if (cmdinfo->state & (COMMAND_INFLIGHT |
>  			      DATA_IN_URB_INFLIGHT |
>  			      DATA_OUT_URB_INFLIGHT |
> @@ -622,7 +622,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
>  	struct urb *urb;
>  	int err;
>  
> -	WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
> +	assert_spin_locked(&devinfo->lock);
>  	if (cmdinfo->state & SUBMIT_STATUS_URB) {
>  		urb = uas_submit_sense_urb(cmnd, gfp, cmdinfo->stream);
>  		if (!urb)
> 

Re: [PATCH] uas: replace WARN_ON_ONCE() with assert_spin_locked().

[Guenter Roeck]

On Mon, Aug 11, 2014 at 08:55:07PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 08/11/2014 08:19 PM, Guenter Roeck wrote:
> > On Mon, Aug 11, 2014 at 01:29:26PM +0530, Sanjeev Sharma wrote:
> >> spin_is_locked() always return false in uniprocessor configuration and therefore it
> >> would be advise to repalce with assert_spin_locked().
> >>
> >> Signed-off-by: Sanjeev Sharma <Sanjeev_Sharma@mentor.com>
> >> ---
> >>  drivers/usb/storage/uas.c | 8 ++++----
> >>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> >> index 3f42785..8e5877d 100644
> >> --- a/drivers/usb/storage/uas.c
> >> +++ b/drivers/usb/storage/uas.c
> >> @@ -154,7 +154,7 @@ static void uas_mark_cmd_dead(struct uas_dev_info *devinfo,
> >>  	struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp);
> >>  
> >>  	uas_log_cmd_state(cmnd, caller);
> >> -	WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
> >> +	assert_spin_locked(&devinfo->lock);
> > 
> > Seems to me that replacing WARN_ON_ONCE (which may be annoying but only
> > creates a traceback, and only once) with assert_spin_locked (which
> > crashes the kernel) is a bit drastic.
> 
> I can see your point, but so far these paranoia checks have never triggered,
> and having them trigger _always_ one some uni-processor (which is the reason
> for this patch) to me seems the worse problem of the 2.
> 
If those are just paranoia checks, it might make sense to use
lockdep_assert_held() to reduce runtime overhead if lockdep
debugging is disabled.

> Ideally we would have a warn_spin_not_locked or such ...
> 
There is WARN_ON_SMP, which might have been a better choice if the _ONCE isn't
that important (which one should think if it is ok to crash the kernel if the
problem is seen).

Guenter

Re: [PATCH] uas: replace WARN_ON_ONCE() with assert_spin_locked().

[Hans de Goede]

Hi,

On 08/11/2014 09:08 PM, Guenter Roeck wrote:
> On Mon, Aug 11, 2014 at 08:55:07PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 08/11/2014 08:19 PM, Guenter Roeck wrote:
>>> On Mon, Aug 11, 2014 at 01:29:26PM +0530, Sanjeev Sharma wrote:
>>>> spin_is_locked() always return false in uniprocessor configuration and therefore it
>>>> would be advise to repalce with assert_spin_locked().
>>>>
>>>> Signed-off-by: Sanjeev Sharma <Sanjeev_Sharma@mentor.com>
>>>> ---
>>>>  drivers/usb/storage/uas.c | 8 ++++----
>>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
>>>> index 3f42785..8e5877d 100644
>>>> --- a/drivers/usb/storage/uas.c
>>>> +++ b/drivers/usb/storage/uas.c
>>>> @@ -154,7 +154,7 @@ static void uas_mark_cmd_dead(struct uas_dev_info *devinfo,
>>>>  	struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp);
>>>>  
>>>>  	uas_log_cmd_state(cmnd, caller);
>>>> -	WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
>>>> +	assert_spin_locked(&devinfo->lock);
>>>
>>> Seems to me that replacing WARN_ON_ONCE (which may be annoying but only
>>> creates a traceback, and only once) with assert_spin_locked (which
>>> crashes the kernel) is a bit drastic.
>>
>> I can see your point, but so far these paranoia checks have never triggered,
>> and having them trigger _always_ one some uni-processor (which is the reason
>> for this patch) to me seems the worse problem of the 2.
>>
> If those are just paranoia checks, it might make sense to use
> lockdep_assert_held() to reduce runtime overhead if lockdep
> debugging is disabled.

Ah yes, that is a good idea.

Sanjeev, can you please send a v2 using lockdep_assert_held() ?

Thanks & Regards,

Hans (the uas driver maintainer)

Re: [PATCH] uas: replace WARN_ON_ONCE() with assert_spin_locked().

[Greg KH]

On Mon, Aug 11, 2014 at 11:56:17AM +0000, Sharma, Sanjeev wrote:
> Hi Greg,
> 
> Please find my comment in line.

As it should, but where is it, your quoting is all messed up...

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org] 
> Sent: Monday, August 11, 2014 1:58 PM
> To: Sharma, Sanjeev
> Cc: hdegoede@redhat.com; kraxel@redhat.com; mdharm-usb@one-eyed-alien.net; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; linux-scsi@vger.kernel.org
> Subject: Re: [PATCH] uas: replace WARN_ON_ONCE() with assert_spin_locked().
> 
> On Mon, Aug 11, 2014 at 01:29:26PM +0530, Sanjeev Sharma wrote:
> > spin_is_locked() always return false in uniprocessor configuration
> 
> Really?  On what processor type?  I don't see that being the case on a few processors, but I didn't check them all, or I might be totally confused with all of the arch_spin_is_locked() definitions in the tree.
> 
> specially in case of mips and powerpc but we can configure our kernel to uniprocessor configuration for specific processor and in that case we should be extra cautious. Please check spinlock_up.h

Shouldn't you just fix your .h file to not do this?  x86 is not this
way, nor is ARM, right?

> > and therefore it
> > would be advise to repalce with assert_spin_locked().
> 
> This implies that all uses of this function is wrong and should be replaced and removed, right?
> 
> Yes and there are only few places in driver for which I am submitting patches which need to be change and at other places it has been already taken care. Please have an look into the
> Old discussion for more details.
> 
> http://linux-kernel.2935.n7.nabble.com/Is-spin-is-locked-safe-to-use-with-BUG-ON-WARN-ON-td654800.html#a921802

Ok, just remove all the tests, it doesn't help anyone :)

thanks,

greg k-h