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

public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed

* [PATCH 2/9] drivers/acpi/asus_acpi.c: Correct use of ! and &
@ 2008-02-26 20:42 Julia Lawall
  2008-02-27 17:03 ` Karol Kozimor
  0 siblings, 1 reply; 12+ messages in thread
From: Julia Lawall @ 2008-02-26 20:42 UTC (permalink / raw)
  To: corentincj, sziwan, acpi4asus-user, linux-kernel, kernel-janitors

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",

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/9] drivers/acpi/asus_acpi.c: Correct use of ! and &
  2008-02-26 20:42 [PATCH 2/9] drivers/acpi/asus_acpi.c: Correct use of ! and & Julia Lawall
@ 2008-02-27 17:03 ` Karol Kozimor
  2008-02-27 17:41   ` Julia Lawall
  2008-02-27 18:29   ` Mark Pearson
  0 siblings, 2 replies; 12+ messages in thread
From: Karol Kozimor @ 2008-02-27 17:03 UTC (permalink / raw)
  To: Julia Lawall
  Cc: corentincj, sziwan, acpi4asus-user, linux-kernel, kernel-janitors

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



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/9] drivers/acpi/asus_acpi.c: Correct use of ! and &
  2008-02-27 17:03 ` Karol Kozimor
@ 2008-02-27 17:41   ` Julia Lawall
  2008-02-27 18:29   ` Mark Pearson
  1 sibling, 0 replies; 12+ messages in thread
From: Julia Lawall @ 2008-02-27 17:41 UTC (permalink / raw)
  To: Karol Kozimor
  Cc: corentincj, sziwan, acpi4asus-user, linux-kernel, kernel-janitors

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/9] drivers/acpi/asus_acpi.c: Correct use of ! and &
  2008-02-27 17:03 ` Karol Kozimor
  2008-02-27 17:41   ` Julia Lawall
@ 2008-02-27 18:29   ` Mark Pearson
  2008-02-29  5:55     ` Andrew Morton
  1 sibling, 1 reply; 12+ messages in thread
From: Mark Pearson @ 2008-02-27 18:29 UTC (permalink / raw)
  To: Karol Kozimor
  Cc: Julia Lawall, corentincj, sziwan, acpi4asus-user, linux-kernel,
	kernel-janitors

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/9] drivers/acpi/asus_acpi.c: Correct use of ! and &
  2008-02-27 18:29   ` Mark Pearson
@ 2008-02-29  5:55     ` Andrew Morton
  2008-02-29  6:10       ` Matthew Wilcox
                         ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Andrew Morton @ 2008-02-29  5:55 UTC (permalink / raw)
  To: Mark Pearson
  Cc: Karol Kozimor, Julia Lawall, corentincj, sziwan, acpi4asus-user,
	linux-kernel, kernel-janitors, Len Brown

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",
_


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/9] drivers/acpi/asus_acpi.c: Correct use of ! and &
  2008-02-29  5:55     ` Andrew Morton
@ 2008-02-29  6:10       ` Matthew Wilcox
  2008-02-29  6:14         ` Matthew Wilcox
  2008-02-29  6:19         ` Andrew Morton
  2008-02-29 11:01       ` Julia Lawall
  2008-02-29 18:06       ` Mark Pearson
  2 siblings, 2 replies; 12+ messages in thread
From: Matthew Wilcox @ 2008-02-29  6:10 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Mark Pearson, Karol Kozimor, Julia Lawall, corentincj, sziwan,
	acpi4asus-user, linux-kernel, kernel-janitors, Len Brown

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."

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/9] drivers/acpi/asus_acpi.c: Correct use of ! and &
  2008-02-29  6:10       ` Matthew Wilcox
@ 2008-02-29  6:14         ` Matthew Wilcox
  2008-02-29  6:19         ` Andrew Morton
  1 sibling, 0 replies; 12+ messages in thread
From: Matthew Wilcox @ 2008-02-29  6:14 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Mark Pearson, Karol Kozimor, Julia Lawall, corentincj, sziwan,
	acpi4asus-user, linux-kernel, kernel-janitors, Len Brown

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."

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/9] drivers/acpi/asus_acpi.c: Correct use of ! and &
  2008-02-29  6:10       ` Matthew Wilcox
  2008-02-29  6:14         ` Matthew Wilcox
@ 2008-02-29  6:19         ` Andrew Morton
  1 sibling, 0 replies; 12+ messages in thread
From: Andrew Morton @ 2008-02-29  6:19 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Mark Pearson, Karol Kozimor, Julia Lawall, corentincj, sziwan,
	acpi4asus-user, linux-kernel, kernel-janitors, Len Brown

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, ! != ~ ;)



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/9] drivers/acpi/asus_acpi.c: Correct use of ! and &
  2008-02-29  5:55     ` Andrew Morton
  2008-02-29  6:10       ` Matthew Wilcox
@ 2008-02-29 11:01       ` Julia Lawall
  2008-02-29 11:08         ` Andrew Morton
  2008-02-29 18:06       ` Mark Pearson
  2 siblings, 1 reply; 12+ messages in thread
From: Julia Lawall @ 2008-02-29 11:01 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Mark Pearson, Karol Kozimor, corentincj, sziwan, acpi4asus-user,
	linux-kernel, kernel-janitors, Len Brown

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/9] drivers/acpi/asus_acpi.c: Correct use of ! and &
  2008-02-29 11:01       ` Julia Lawall
@ 2008-02-29 11:08         ` Andrew Morton
  0 siblings, 0 replies; 12+ messages in thread
From: Andrew Morton @ 2008-02-29 11:08 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Mark Pearson, Karol Kozimor, corentincj, sziwan, acpi4asus-user,
	linux-kernel, kernel-janitors, Len Brown

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/9] drivers/acpi/asus_acpi.c: Correct use of ! and &
  2008-02-29  5:55     ` Andrew Morton
  2008-02-29  6:10       ` Matthew Wilcox
  2008-02-29 11:01       ` Julia Lawall
@ 2008-02-29 18:06       ` Mark Pearson
  2008-02-29 21:33         ` Andrew Morton
  2 siblings, 1 reply; 12+ messages in thread
From: Mark Pearson @ 2008-02-29 18:06 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Karol Kozimor, Julia Lawall, corentincj, sziwan, acpi4asus-user,
	linux-kernel, kernel-janitors, Len Brown

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/9] drivers/acpi/asus_acpi.c: Correct use of ! and &
  2008-02-29 18:06       ` Mark Pearson
@ 2008-02-29 21:33         ` Andrew Morton
  0 siblings, 0 replies; 12+ messages in thread
From: Andrew Morton @ 2008-02-29 21:33 UTC (permalink / raw)
  To: Mark Pearson
  Cc: sziwan, julia, corentincj, sziwan, acpi4asus-user, linux-kernel,
	kernel-janitors, lenb

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2008-02-29 21:49 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-02-26 20:42 [PATCH 2/9] drivers/acpi/asus_acpi.c: Correct use of ! and & Julia Lawall
2008-02-27 17:03 ` Karol Kozimor
2008-02-27 17:41   ` Julia Lawall
2008-02-27 18:29   ` Mark Pearson
2008-02-29  5:55     ` Andrew Morton
2008-02-29  6:10       ` Matthew Wilcox
2008-02-29  6:14         ` Matthew Wilcox
2008-02-29  6:19         ` Andrew Morton
2008-02-29 11:01       ` Julia Lawall
2008-02-29 11:08         ` Andrew Morton
2008-02-29 18:06       ` Mark Pearson
2008-02-29 21:33         ` Andrew Morton

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox