From: Julia Lawall <[email protected]>
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 <[email protected]>
---
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",
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
[email protected]
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
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.
On Wed, 27 Feb 2008 19:29:15 +0100 Mark Pearson <[email protected]> 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",
_
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."
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."
On Thu, 28 Feb 2008 23:10:23 -0700 Matthew Wilcox <[email protected]> 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, ! != ~ ;)
On Thu, 28 Feb 2008, Andrew Morton wrote:
> On Wed, 27 Feb 2008 19:29:15 +0100 Mark Pearson <[email protected]> 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
On Fri, 29 Feb 2008 12:01:26 +0100 (CET) Julia Lawall <[email protected]> wrote:
> On Thu, 28 Feb 2008, Andrew Morton wrote:
>
> > On Wed, 27 Feb 2008 19:29:15 +0100 Mark Pearson <[email protected]> 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.
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.
On Fri, 29 Feb 2008 19:06:48 +0100
Mark Pearson <[email protected]> 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.