[PATCH 2/9] drivers/acpi/asus_acpi.c: Correct use of ! and &

[PATCH 2/9] drivers/acpi/asus_acpi.c: Correct use of ! and &

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

In commit e6bafba5b4765a5a252f1b8d31cbf6d2459da337, a bug was fixed that
involved converting !x & y to !(x & y).  The code below shows the same
pattern, and thus should perhaps be fixed in the same way.

This is not tested and clearly changes the semantics, so it is only
something to consider.

The semantic patch that makes this change is as follows:
(http://www.emn.fr/x-info/coccinelle/)

// <smpl>
@@ expression E1,E2; @@
(
  !E1 & !E2
|
- !E1 & E2
+ !(E1 & E2)
)
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---

diff -u -p a/drivers/acpi/asus_acpi.c b/drivers/acpi/asus_acpi.c
--- a/drivers/acpi/asus_acpi.c 2008-02-10 22:34:05.000000000 +0100
+++ b/drivers/acpi/asus_acpi.c 2008-02-26 08:00:42.000000000 +0100
@@ -610,7 +610,7 @@ write_led(const char __user * buffer, un
 	    (led_out) ? (hotk->status | ledmask) : (hotk->status & ~ledmask);
 
 	if (invert)		/* invert target value */
-		led_out = !led_out & 0x1;
+		led_out = !(led_out & 0x1);
 
 	if (!write_acpi_int(hotk->handle, ledname, led_out, NULL))
 		printk(KERN_WARNING "Asus ACPI: LED (%s) write failed\n",

Re: [PATCH 2/9] drivers/acpi/asus_acpi.c: Correct use of ! and &

[Karol Kozimor]

On 26-02-2008, at 21:42, Julia Lawall wrote:
>  	if (invert)		/* invert target value */
> -		led_out = !led_out & 0x1;
> +		led_out = !(led_out & 0x1);
>
>  	if (!write_acpi_int(hotk->handle, ledname, led_out, NULL))
>  		printk(KERN_WARNING "Asus ACPI: LED (%s) write failed\n",


IIRC we're just supposed to flip the last bit here, so the original  
code is correct.
Best regards,

-- 
Karol Kozimor
sziwan@hell.org.pl

Re: [PATCH 2/9] drivers/acpi/asus_acpi.c: Correct use of ! and &

[Julia Lawall]

On Wed, 27 Feb 2008, Karol Kozimor wrote:

> On 26-02-2008, at 21:42, Julia Lawall wrote:
> >  	if (invert)		/* invert target value */
> > -		led_out = !led_out & 0x1;
> > +		led_out = !(led_out & 0x1);
> >
> >  	if (!write_acpi_int(hotk->handle, ledname, led_out, NULL))
> >  		printk(KERN_WARNING "Asus ACPI: LED (%s) write failed\n",
>
>
> IIRC we're just supposed to flip the last bit here, so the original
> code is correct.

I spent some time thinking about this one.  The original code is ok if
led_out is always either 0x01 or 0x00.  But what if it is eg 0xc0?  Then
the negation amounts to the negation of a nonzero number, so the result is
0.  So the result of the bit and is 0.  So the last bit is not flipped.
But I don't know what is the range of led_out.  If it is always 1 or 0,
then why bother with the bit and?

julia

Re: [PATCH 2/9] drivers/acpi/asus_acpi.c: Correct use of ! and &

[Mark Pearson]

Karol Kozimor wrote:
> On 26-02-2008, at 21:42, Julia Lawall wrote:
>>      if (invert)        /* invert target value */
>> -        led_out = !led_out & 0x1;
>> +        led_out = !(led_out & 0x1);
>>
>>      if (!write_acpi_int(hotk->handle, ledname, led_out, NULL))
>>          printk(KERN_WARNING "Asus ACPI: LED (%s) write failed\n",
> 
> 
> IIRC we're just supposed to flip the last bit here, so the original code
> is correct.
> Best regards,
> 

Seems an odd way of doing:

	led_out ^= 0x01;

It this due to some optimisation?

Cheers, Mark.

Re: [PATCH 2/9] drivers/acpi/asus_acpi.c: Correct use of ! and &

[Andrew Morton]

On Wed, 27 Feb 2008 19:29:15 +0100 Mark Pearson <devnull.port@googlemail.com> wrote:

> Karol Kozimor wrote:
> > On 26-02-2008, at 21:42, Julia Lawall wrote:
> >>      if (invert)        /* invert target value */
> >> -        led_out = !led_out & 0x1;
> >> +        led_out = !(led_out & 0x1);
> >>
> >>      if (!write_acpi_int(hotk->handle, ledname, led_out, NULL))
> >>          printk(KERN_WARNING "Asus ACPI: LED (%s) write failed\n",
> > 
> > 
> > IIRC we're just supposed to flip the last bit here, so the original code
> > is correct.
> > Best regards,
> > 
> 
> Seems an odd way of doing:
> 
> 	led_out ^= 0x01;

It does.

> It this due to some optimisation?

Surely not ;)

That code has been there for many years.

I changed the patch to this:

--- a/drivers/acpi/asus_acpi.c~drivers-acpi-asus_acpic-correct-use-of-and
+++ a/drivers/acpi/asus_acpi.c
@@ -610,7 +610,7 @@ write_led(const char __user * buffer, un
 	    (led_out) ? (hotk->status | ledmask) : (hotk->status & ~ledmask);
 
 	if (invert)		/* invert target value */
-		led_out = !led_out & 0x1;
+		led_out = !led_out;
 
 	if (!write_acpi_int(hotk->handle, ledname, led_out, NULL))
 		printk(KERN_WARNING "Asus ACPI: LED (%s) write failed\n",
_

Re: [PATCH 2/9] drivers/acpi/asus_acpi.c: Correct use of ! and &

[Matthew Wilcox]

On Thu, Feb 28, 2008 at 09:55:03PM -0800, Andrew Morton wrote:
>  	if (invert)		/* invert target value */
> -		led_out = !led_out & 0x1;
> +		led_out = !led_out;
>  
>  	if (!write_acpi_int(hotk->handle, ledname, led_out, NULL))

But now you're writing 0xffffffff instead of 1.  I think the suggestion
of led_out ^= 1 was the correct one.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 2/9] drivers/acpi/asus_acpi.c: Correct use of ! and &

[Matthew Wilcox]

On Thu, Feb 28, 2008 at 11:10:23PM -0700, Matthew Wilcox wrote:
> On Thu, Feb 28, 2008 at 09:55:03PM -0800, Andrew Morton wrote:
> >  	if (invert)		/* invert target value */
> > -		led_out = !led_out & 0x1;
> > +		led_out = !led_out;
> >  
> >  	if (!write_acpi_int(hotk->handle, ledname, led_out, NULL))
> 
> But now you're writing 0xffffffff instead of 1.  I think the suggestion
> of led_out ^= 1 was the correct one.

! is not ~
! is not ~
! is not ~
....

I'll go to sleep now.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 2/9] drivers/acpi/asus_acpi.c: Correct use of ! and &

[Andrew Morton]

On Thu, 28 Feb 2008 23:10:23 -0700 Matthew Wilcox <matthew@wil.cx> wrote:

> On Thu, Feb 28, 2008 at 09:55:03PM -0800, Andrew Morton wrote:
> >  	if (invert)		/* invert target value */
> > -		led_out = !led_out & 0x1;
> > +		led_out = !led_out;
> >  
> >  	if (!write_acpi_int(hotk->handle, ledname, led_out, NULL))
> 
> But now you're writing 0xffffffff instead of 1.  I think the suggestion
> of led_out ^= 1 was the correct one.
> 

!0 is 1, not 0xffffffff.

IOW, ! != ~ ;)

Re: [PATCH 2/9] drivers/acpi/asus_acpi.c: Correct use of ! and &

[Julia Lawall]

On Thu, 28 Feb 2008, Andrew Morton wrote:

> On Wed, 27 Feb 2008 19:29:15 +0100 Mark Pearson <devnull.port@googlemail.com> wrote:
> 
> > Karol Kozimor wrote:
> > > On 26-02-2008, at 21:42, Julia Lawall wrote:
> > >>      if (invert)        /* invert target value */
> > >> -        led_out = !led_out & 0x1;
> > >> +        led_out = !(led_out & 0x1);
> > >>
> > >>      if (!write_acpi_int(hotk->handle, ledname, led_out, NULL))
> > >>          printk(KERN_WARNING "Asus ACPI: LED (%s) write failed\n",
> > > 
> > > 
> > > IIRC we're just supposed to flip the last bit here, so the original code
> > > is correct.
> > > Best regards,
> > > 
> > 
> > Seems an odd way of doing:
> > 
> > 	led_out ^= 0x01;
> 
> It does.
> 
> > It this due to some optimisation?
> 
> Surely not ;)
> 
> That code has been there for many years.
> 
> I changed the patch to this:
> 
> --- a/drivers/acpi/asus_acpi.c~drivers-acpi-asus_acpic-correct-use-of-and
> +++ a/drivers/acpi/asus_acpi.c
> @@ -610,7 +610,7 @@ write_led(const char __user * buffer, un
>  	    (led_out) ? (hotk->status | ledmask) : (hotk->status & ~ledmask);
>  
>  	if (invert)		/* invert target value */
> -		led_out = !led_out & 0x1;
> +		led_out = !led_out;

I don't think this is the same:

!(0110 & 0x01) = !0 = 1
!0110 = 0

led_out ^= 0x01;

is also not the same:

0110 ^ 0x01 = 0111

Is it desired to keep the value and flip the last bit or just obtain the 
opposite of the last bit?

julia

Re: [PATCH 2/9] drivers/acpi/asus_acpi.c: Correct use of ! and &

[Andrew Morton]

On Fri, 29 Feb 2008 12:01:26 +0100 (CET) Julia Lawall <julia@diku.dk> wrote:

> On Thu, 28 Feb 2008, Andrew Morton wrote:
> 
> > On Wed, 27 Feb 2008 19:29:15 +0100 Mark Pearson <devnull.port@googlemail.com> wrote:
> > 
> > > Karol Kozimor wrote:
> > > > On 26-02-2008, at 21:42, Julia Lawall wrote:
> > > >>      if (invert)        /* invert target value */
> > > >> -        led_out = !led_out & 0x1;
> > > >> +        led_out = !(led_out & 0x1);
> > > >>
> > > >>      if (!write_acpi_int(hotk->handle, ledname, led_out, NULL))
> > > >>          printk(KERN_WARNING "Asus ACPI: LED (%s) write failed\n",
> > > > 
> > > > 
> > > > IIRC we're just supposed to flip the last bit here, so the original code
> > > > is correct.
> > > > Best regards,
> > > > 
> > > 
> > > Seems an odd way of doing:
> > > 
> > > 	led_out ^= 0x01;
> > 
> > It does.
> > 
> > > It this due to some optimisation?
> > 
> > Surely not ;)
> > 
> > That code has been there for many years.
> > 
> > I changed the patch to this:
> > 
> > --- a/drivers/acpi/asus_acpi.c~drivers-acpi-asus_acpic-correct-use-of-and
> > +++ a/drivers/acpi/asus_acpi.c
> > @@ -610,7 +610,7 @@ write_led(const char __user * buffer, un
> >  	    (led_out) ? (hotk->status | ledmask) : (hotk->status & ~ledmask);
> >  
> >  	if (invert)		/* invert target value */
> > -		led_out = !led_out & 0x1;
> > +		led_out = !led_out;
> 
> I don't think this is the same:
> 
> !(0110 & 0x01) = !0 = 1
> !0110 = 0
> 
> led_out ^= 0x01;
> 
> is also not the same:
> 
> 0110 ^ 0x01 = 0111
> 
> Is it desired to keep the value and flip the last bit or just obtain the 
> opposite of the last bit?
> 

led_out can only take the values 0 or 1 here.

Re: [PATCH 2/9] drivers/acpi/asus_acpi.c: Correct use of ! and &

[Mark Pearson]

Andrew Morton wrote:
>> Seems an odd way of doing:
>>
>> 	led_out ^= 0x01;
> 
> It does.
> 
>> It this due to some optimisation?
> 
> Surely not ;)
>
;) Thought so - one doesn't like to be too presumptuous ;)

> That code has been there for many years.
> 
> I changed the patch to this:
> 
> --- a/drivers/acpi/asus_acpi.c~drivers-acpi-asus_acpic-correct-use-of-and
> +++ a/drivers/acpi/asus_acpi.c
> @@ -610,7 +610,7 @@ write_led(const char __user * buffer, un
>  	    (led_out) ? (hotk->status | ledmask) : (hotk->status & ~ledmask);
>  
>  	if (invert)		/* invert target value */
> -		led_out = !led_out & 0x1;
> +		led_out = !led_out;
>  
>  	if (!write_acpi_int(hotk->handle, ledname, led_out, NULL))
>  		printk(KERN_WARNING "Asus ACPI: LED (%s) write failed\n",
> _
> 
> 

Is the ! operator architecture/compiler dependent? or can one always say that
!NON_ZERO_VALUE == 0 and !0 == 1?

Cheers, Mark.

Re: [PATCH 2/9] drivers/acpi/asus_acpi.c: Correct use of ! and &

[Andrew Morton]

On Fri, 29 Feb 2008 19:06:48 +0100
Mark Pearson <devnull.port@googlemail.com> wrote:

> Andrew Morton wrote:
> >> Seems an odd way of doing:
> >>
> >> 	led_out ^= 0x01;
> > 
> > It does.
> > 
> >> It this due to some optimisation?
> > 
> > Surely not ;)
> >
> ;) Thought so - one doesn't like to be too presumptuous ;)
> 
> > That code has been there for many years.
> > 
> > I changed the patch to this:
> > 
> > --- a/drivers/acpi/asus_acpi.c~drivers-acpi-asus_acpic-correct-use-of-and
> > +++ a/drivers/acpi/asus_acpi.c
> > @@ -610,7 +610,7 @@ write_led(const char __user * buffer, un
> >  	    (led_out) ? (hotk->status | ledmask) : (hotk->status & ~ledmask);
> >  
> >  	if (invert)		/* invert target value */
> > -		led_out = !led_out & 0x1;
> > +		led_out = !led_out;
> >  
> >  	if (!write_acpi_int(hotk->handle, ledname, led_out, NULL))
> >  		printk(KERN_WARNING "Asus ACPI: LED (%s) write failed\n",
> > _
> > 
> > 
> 
> Is the ! operator architecture/compiler dependent?

It shouldn't be.

> or can one always say that
> !NON_ZERO_VALUE == 0 and !0 == 1?
> 

I always have ;)  I expect it's in the C standard somewhere.