media: atomisp: silence "dubious: !x | !y" warning
diff mbox series

Message ID 20210417153627.GA50228@ashish-NUC8i5BEH
State New
Headers show
Series
  • media: atomisp: silence "dubious: !x | !y" warning
Related show

Commit Message

Ashish April 17, 2021, 3:36 p.m. UTC
Upon running sparse, "warning: dubious: !x | !y" is brought to notice
for this file.  Logical and bitwise OR are basically the same in this
context so it doesn't cause a runtime bug.  But let's change it to
logical OR to make it cleaner and silence the Sparse warning.

Signed-off-by: Ashish Kalra <eashishkalra@gmail.com>
---
 .../media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c    | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Mauro Carvalho Chehab April 17, 2021, 6:56 p.m. UTC | #1
Em Sat, 17 Apr 2021 21:06:27 +0530
Ashish Kalra <eashishkalra@gmail.com> escreveu:

> Upon running sparse, "warning: dubious: !x | !y" is brought to notice
> for this file.  Logical and bitwise OR are basically the same in this
> context so it doesn't cause a runtime bug.  But let's change it to
> logical OR to make it cleaner and silence the Sparse warning.
> 
> Signed-off-by: Ashish Kalra <eashishkalra@gmail.com>
> ---
>  .../media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c    | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c b/drivers/staging/media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c
> index 358cb7d2cd4c..3b850bb2d39d 100644
> --- a/drivers/staging/media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c
> +++ b/drivers/staging/media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c
> @@ -58,7 +58,7 @@ sh_css_vf_downscale_log2(
>  	unsigned int ds_log2 = 0;
>  	unsigned int out_width;
>  
> -	if ((!out_info) | (!vf_info))
> +	if ((!out_info) || (!vf_info))


While here, please get rid of the unneeded parenthesis:

	if (!out_info || !vf_info)


>  		return -EINVAL;
>  
>  	out_width = out_info->res.width;



Thanks,
Mauro
David Laight April 17, 2021, 9:31 p.m. UTC | #2
From: Mauro Carvalho Chehab
> Sent: 17 April 2021 19:56
> 
> Em Sat, 17 Apr 2021 21:06:27 +0530
> Ashish Kalra <eashishkalra@gmail.com> escreveu:
> 
> > Upon running sparse, "warning: dubious: !x | !y" is brought to notice
> > for this file.  Logical and bitwise OR are basically the same in this
> > context so it doesn't cause a runtime bug.  But let's change it to
> > logical OR to make it cleaner and silence the Sparse warning.

The old code is very likely to by slightly more efficient.

It may not matter here, but it might in a really hot path.

Since !x | !y and !x || !y always have the same value
why is sparse complaining at all.

	David

> >
> > Signed-off-by: Ashish Kalra <eashishkalra@gmail.com>
> > ---
> >  .../media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c    | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c
> b/drivers/staging/media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c
> > index 358cb7d2cd4c..3b850bb2d39d 100644
> > --- a/drivers/staging/media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c
> > +++ b/drivers/staging/media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c
> > @@ -58,7 +58,7 @@ sh_css_vf_downscale_log2(
> >  	unsigned int ds_log2 = 0;
> >  	unsigned int out_width;
> >
> > -	if ((!out_info) | (!vf_info))
> > +	if ((!out_info) || (!vf_info))
> 
> 
> While here, please get rid of the unneeded parenthesis:
> 
> 	if (!out_info || !vf_info)
> 
> 
> >  		return -EINVAL;
> >
> >  	out_width = out_info->res.width;
> 
> 
> 
> Thanks,
> Mauro

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Ashish April 18, 2021, 1:15 a.m. UTC | #3
On Sat, Apr 17, 2021 at 09:31:32PM +0000, David Laight wrote:
> From: Mauro Carvalho Chehab
> > Sent: 17 April 2021 19:56
> > 
> > Em Sat, 17 Apr 2021 21:06:27 +0530
> > Ashish Kalra <eashishkalra@gmail.com> escreveu:
> > 
> > > Upon running sparse, "warning: dubious: !x | !y" is brought to notice
> > > for this file.  Logical and bitwise OR are basically the same in this
> > > context so it doesn't cause a runtime bug.  But let's change it to
> > > logical OR to make it cleaner and silence the Sparse warning.
> 
> The old code is very likely to by slightly more efficient.
> 
> It may not matter here, but it might in a really hot path.
> 
> Since !x | !y and !x || !y always have the same value
> why is sparse complaining at all.
> 
> 	David
This warning is coming from sparse as per below explanation

As the name suggests, a "bitwise" expression is one that is restricted to
only a certain "bitwise" operations that make sense within that class. In
particular, you can't mix a "bitwise" class with a normal integer
expression
Warning:
int __bitwise i;
int __bitwise j;
the two variables "i" and "j" are _not_ compatible, simply because they
were declared separately, while in the case of
	int __bitwise i, j;
they _are_ compatible.

https://yarchive.net/comp/linux/sparse.html
> 
> > >
> > > Signed-off-by: Ashish Kalra <eashishkalra@gmail.com>
> > > ---
> > >  .../media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c    | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c
> > b/drivers/staging/media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c
> > > index 358cb7d2cd4c..3b850bb2d39d 100644
> > > --- a/drivers/staging/media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c
> > > +++ b/drivers/staging/media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c
> > > @@ -58,7 +58,7 @@ sh_css_vf_downscale_log2(
> > >  	unsigned int ds_log2 = 0;
> > >  	unsigned int out_width;
> > >
> > > -	if ((!out_info) | (!vf_info))
> > > +	if ((!out_info) || (!vf_info))
> > 
> > 
> > While here, please get rid of the unneeded parenthesis:
> > 
> > 	if (!out_info || !vf_info)
> > 
> > 
> > >  		return -EINVAL;
> > >
> > >  	out_width = out_info->res.width;
> > 
> > 
> > 
> > Thanks,
> > Mauro
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
Ashish April 18, 2021, 1:26 a.m. UTC | #4
On Sat, Apr 17, 2021 at 08:56:13PM +0200, Mauro Carvalho Chehab wrote:
> Em Sat, 17 Apr 2021 21:06:27 +0530
> Ashish Kalra <eashishkalra@gmail.com> escreveu:
> 
> > Upon running sparse, "warning: dubious: !x | !y" is brought to notice
> > for this file.  Logical and bitwise OR are basically the same in this
> > context so it doesn't cause a runtime bug.  But let's change it to
> > logical OR to make it cleaner and silence the Sparse warning.
> > 
> > Signed-off-by: Ashish Kalra <eashishkalra@gmail.com>
> > ---
> >  .../media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c    | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c b/drivers/staging/media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c
> > index 358cb7d2cd4c..3b850bb2d39d 100644
> > --- a/drivers/staging/media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c
> > +++ b/drivers/staging/media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c
> > @@ -58,7 +58,7 @@ sh_css_vf_downscale_log2(
> >  	unsigned int ds_log2 = 0;
> >  	unsigned int out_width;
> >  
> > -	if ((!out_info) | (!vf_info))
> > +	if ((!out_info) || (!vf_info))
> 
> 
> While here, please get rid of the unneeded parenthesis:
> 
> 	if (!out_info || !vf_info)
> 
> 
> >  		return -EINVAL;
> >  
> >  	out_width = out_info->res.width;
> 
> 
> 
> Thanks,
> Mauro
Updated Patch as per your feedback

Thanks
Ashish
Luc Van Oostenryck April 18, 2021, 9:59 p.m. UTC | #5
On Sat, Apr 17, 2021 at 09:31:32PM +0000, David Laight wrote:
> > > Upon running sparse, "warning: dubious: !x | !y" is brought to notice
> > > for this file.  Logical and bitwise OR are basically the same in this
> > > context so it doesn't cause a runtime bug.  But let's change it to
> > > logical OR to make it cleaner and silence the Sparse warning.
> 
> The old code is very likely to by slightly more efficient.
> 
> It may not matter here, but it might in a really hot path.
> 
> Since !x | !y and !x || !y always have the same value
> why is sparse complaining at all.

They both will have the same value here and any half-decent
compiler know that and thus generate the same code, so no
worries about efficiency.

Sparse complains because the programmer's intention is not clear.
Was a boolean context or a bitwise context that was meant?
Maybe '||' was meant and the RHS had to be short cut?
Maybe what was meant was '~x | ~y'?

-- Luc
Dan Carpenter April 20, 2021, 10:27 a.m. UTC | #6
On Sat, Apr 17, 2021 at 09:31:32PM +0000, David Laight wrote:
> From: Mauro Carvalho Chehab
> > Sent: 17 April 2021 19:56
> > 
> > Em Sat, 17 Apr 2021 21:06:27 +0530
> > Ashish Kalra <eashishkalra@gmail.com> escreveu:
> > 
> > > Upon running sparse, "warning: dubious: !x | !y" is brought to notice
> > > for this file.  Logical and bitwise OR are basically the same in this
> > > context so it doesn't cause a runtime bug.  But let's change it to
> > > logical OR to make it cleaner and silence the Sparse warning.
> 
> The old code is very likely to by slightly more efficient.
> 
> It may not matter here, but it might in a really hot path.
> 
> Since !x | !y and !x || !y always have the same value
> why is sparse complaining at all.
> 

Smatch doesn't warn about | vs || if both sides are true/false.  But
I've occasionally asked people if they were trying to do a fast path
optimization but it's always just a typo.

regards,
dan carpenter
David Laight April 20, 2021, 10:36 a.m. UTC | #7
From: Dan Carpenter
> Sent: 20 April 2021 11:28
> 
> On Sat, Apr 17, 2021 at 09:31:32PM +0000, David Laight wrote:
> > From: Mauro Carvalho Chehab
> > > Sent: 17 April 2021 19:56
> > >
> > > Em Sat, 17 Apr 2021 21:06:27 +0530
> > > Ashish Kalra <eashishkalra@gmail.com> escreveu:
> > >
> > > > Upon running sparse, "warning: dubious: !x | !y" is brought to notice
> > > > for this file.  Logical and bitwise OR are basically the same in this
> > > > context so it doesn't cause a runtime bug.  But let's change it to
> > > > logical OR to make it cleaner and silence the Sparse warning.
> >
> > The old code is very likely to by slightly more efficient.
> >
> > It may not matter here, but it might in a really hot path.
> >
> > Since !x | !y and !x || !y always have the same value
> > why is sparse complaining at all.
> >
> 
> Smatch doesn't warn about | vs || if both sides are true/false.  But
> I've occasionally asked people if they were trying to do a fast path
> optimization but it's always just a typo.

The problem is with people blindly patching code to 'fix'
these warnings.
It might just be a fast path optimisation - which they break.

Trying to beat the compiler into submission can be hard though.
Getting it to 'or' together the outputs from a series of x86
'setne' instructions isn't for the faint hearted.
Not helped by the instruction only setting %al.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Hans Verkuil April 20, 2021, 12:04 p.m. UTC | #8
Hi Ashish,

On 18/04/2021 03:26, Ashish Kalra wrote:
> On Sat, Apr 17, 2021 at 08:56:13PM +0200, Mauro Carvalho Chehab wrote:
>> Em Sat, 17 Apr 2021 21:06:27 +0530
>> Ashish Kalra <eashishkalra@gmail.com> escreveu:
>>
>>> Upon running sparse, "warning: dubious: !x | !y" is brought to notice
>>> for this file.  Logical and bitwise OR are basically the same in this
>>> context so it doesn't cause a runtime bug.  But let's change it to
>>> logical OR to make it cleaner and silence the Sparse warning.
>>>
>>> Signed-off-by: Ashish Kalra <eashishkalra@gmail.com>
>>> ---
>>>  .../media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c    | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c b/drivers/staging/media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c
>>> index 358cb7d2cd4c..3b850bb2d39d 100644
>>> --- a/drivers/staging/media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c
>>> +++ b/drivers/staging/media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c
>>> @@ -58,7 +58,7 @@ sh_css_vf_downscale_log2(
>>>  	unsigned int ds_log2 = 0;
>>>  	unsigned int out_width;
>>>  
>>> -	if ((!out_info) | (!vf_info))
>>> +	if ((!out_info) || (!vf_info))
>>
>>
>> While here, please get rid of the unneeded parenthesis:
>>
>> 	if (!out_info || !vf_info)
>>
>>
>>>  		return -EINVAL;
>>>  
>>>  	out_width = out_info->res.width;
>>
>>
>>
>> Thanks,
>> Mauro
> Updated Patch as per your feedback

Please don't post patches as an attachment. Just post it inline as you did the
first time, but with Subject prefix [PATCHv2].

Thanks!

	Hans

Patch
diff mbox series

diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c b/drivers/staging/media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c
index 358cb7d2cd4c..3b850bb2d39d 100644
--- a/drivers/staging/media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c
+++ b/drivers/staging/media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c
@@ -58,7 +58,7 @@  sh_css_vf_downscale_log2(
 	unsigned int ds_log2 = 0;
 	unsigned int out_width;
 
-	if ((!out_info) | (!vf_info))
+	if ((!out_info) || (!vf_info))
 		return -EINVAL;
 
 	out_width = out_info->res.width;