Hi Rogier,
here's a patch to fix a typo in rio_linux which affects both
kernel 2.4 and 2.6. It's not big deal it seems as it only
affects the irq-less path.
I found this one like that :
$ grep -r '[^&]&[^&]*![^=]' drivers/char/
I'm sure others will find more efficient rules to catch such
errors.
Regards,
Willy
>From 4fb85842b76ad28893ea2aeaeb6dbc4e3f5a2dee Mon Sep 17 00:00:00 2001
From: Willy Tarreau <[email protected]>
Date: Wed, 22 Nov 2006 23:54:48 +0100
Subject: [PATCH] rio: typo in bitwise AND expression.
The line :
hp->Mode &= !RIO_PCI_INT_ENABLE;
is obviously wrong as RIO_PCI_INT_ENABLE=0x04 and is used as a bitmask
2 lines before. Getting no IRQ would not disable RIO_PCI_INT_ENABLE
but rather RIO_PCI_BOOT_FROM_RAM which equals 0x01.
Obvious fix is to change ! for ~.
Signed-off-by: Willy Tarreau <[email protected]>
---
drivers/char/rio/rio_linux.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/char/rio/rio_linux.c b/drivers/char/rio/rio_linux.c
index 7ac68cb..3228fad 100644
--- a/drivers/char/rio/rio_linux.c
+++ b/drivers/char/rio/rio_linux.c
@@ -1143,7 +1143,7 @@ #endif /* PCI */
rio_dprintk(RIO_DEBUG_INIT, "Enabling interrupts on rio card.\n");
hp->Mode |= RIO_PCI_INT_ENABLE;
} else
- hp->Mode &= !RIO_PCI_INT_ENABLE;
+ hp->Mode &= ~RIO_PCI_INT_ENABLE;
rio_dprintk(RIO_DEBUG_INIT, "New Mode: %x\n", hp->Mode);
rio_start_card_running(hp);
}
--
1.4.2.4
On Wed, Nov 22, 2006 at 11:58:56PM +0100, Willy Tarreau wrote:
Seems ok.
Signed-off-by: Patrick vd Lageweg <[email protected]>
Patrick
> Hi Rogier,
>
> here's a patch to fix a typo in rio_linux which affects both
> kernel 2.4 and 2.6. It's not big deal it seems as it only
> affects the irq-less path.
>
> I found this one like that :
>
> $ grep -r '[^&]&[^&]*![^=]' drivers/char/
>
> I'm sure others will find more efficient rules to catch such
> errors.
>
> Regards,
> Willy
>
> From 4fb85842b76ad28893ea2aeaeb6dbc4e3f5a2dee Mon Sep 17 00:00:00 2001
> From: Willy Tarreau <[email protected]>
> Date: Wed, 22 Nov 2006 23:54:48 +0100
> Subject: [PATCH] rio: typo in bitwise AND expression.
>
> The line :
>
> hp->Mode &= !RIO_PCI_INT_ENABLE;
>
> is obviously wrong as RIO_PCI_INT_ENABLE=0x04 and is used as a bitmask
> 2 lines before. Getting no IRQ would not disable RIO_PCI_INT_ENABLE
> but rather RIO_PCI_BOOT_FROM_RAM which equals 0x01.
>
> Obvious fix is to change ! for ~.
>
> Signed-off-by: Willy Tarreau <[email protected]>
> ---
> drivers/char/rio/rio_linux.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/char/rio/rio_linux.c b/drivers/char/rio/rio_linux.c
> index 7ac68cb..3228fad 100644
> --- a/drivers/char/rio/rio_linux.c
> +++ b/drivers/char/rio/rio_linux.c
> @@ -1143,7 +1143,7 @@ #endif /* PCI */
> rio_dprintk(RIO_DEBUG_INIT, "Enabling interrupts on rio card.\n");
> hp->Mode |= RIO_PCI_INT_ENABLE;
> } else
> - hp->Mode &= !RIO_PCI_INT_ENABLE;
> + hp->Mode &= ~RIO_PCI_INT_ENABLE;
> rio_dprintk(RIO_DEBUG_INIT, "New Mode: %x\n", hp->Mode);
> rio_start_card_running(hp);
> }
> --
> 1.4.2.4
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
Applied to 2.4, thanks Patrick.
On Thu, Nov 23, 2006 at 03:11:06PM +0100, Patrick vd Lageweg wrote:
> On Wed, Nov 22, 2006 at 11:58:56PM +0100, Willy Tarreau wrote:
>
> Seems ok.
>
> Signed-off-by: Patrick vd Lageweg <[email protected]>
>
> Patrick
>
> > Hi Rogier,
> >
> > here's a patch to fix a typo in rio_linux which affects both
> > kernel 2.4 and 2.6. It's not big deal it seems as it only
> > affects the irq-less path.
> >
> > I found this one like that :
> >
> > $ grep -r '[^&]&[^&]*![^=]' drivers/char/
> >
> > I'm sure others will find more efficient rules to catch such
> > errors.
> >
> > Regards,
> > Willy
> >
> > From 4fb85842b76ad28893ea2aeaeb6dbc4e3f5a2dee Mon Sep 17 00:00:00 2001
> > From: Willy Tarreau <[email protected]>
> > Date: Wed, 22 Nov 2006 23:54:48 +0100
> > Subject: [PATCH] rio: typo in bitwise AND expression.
> >
> > The line :
> >
> > hp->Mode &= !RIO_PCI_INT_ENABLE;
> >
> > is obviously wrong as RIO_PCI_INT_ENABLE=0x04 and is used as a bitmask
> > 2 lines before. Getting no IRQ would not disable RIO_PCI_INT_ENABLE
> > but rather RIO_PCI_BOOT_FROM_RAM which equals 0x01.
> >
> > Obvious fix is to change ! for ~.
> >
> > Signed-off-by: Willy Tarreau <[email protected]>
> > ---
> > drivers/char/rio/rio_linux.c | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/char/rio/rio_linux.c b/drivers/char/rio/rio_linux.c
> > index 7ac68cb..3228fad 100644
> > --- a/drivers/char/rio/rio_linux.c
> > +++ b/drivers/char/rio/rio_linux.c
> > @@ -1143,7 +1143,7 @@ #endif /* PCI */
> > rio_dprintk(RIO_DEBUG_INIT, "Enabling interrupts on rio card.\n");
> > hp->Mode |= RIO_PCI_INT_ENABLE;
> > } else
> > - hp->Mode &= !RIO_PCI_INT_ENABLE;
> > + hp->Mode &= ~RIO_PCI_INT_ENABLE;
> > rio_dprintk(RIO_DEBUG_INIT, "New Mode: %x\n", hp->Mode);
> > rio_start_card_running(hp);
> > }
> > --
> > 1.4.2.4
> >
> > -
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
> >
Andreas, is this correct?
---------------------------------
SIXP_RX_DCD_MASK is 0x18, so the command below will make cmd 0 always.
This is likely wrong.
Signed-off-by: Alexey Dobriyan <[email protected]>
---
drivers/net/hamradio/6pack.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/drivers/net/hamradio/6pack.c
+++ b/drivers/net/hamradio/6pack.c
@@ -914,7 +914,7 @@ static void decode_prio_command(struct s
printk(KERN_DEBUG "6pack: protocol violation\n");
else
sp->status = 0;
- cmd &= !SIXP_RX_DCD_MASK;
+ cmd &= ~SIXP_RX_DCD_MASK;
}
sp->status = cmd & SIXP_PRIO_DATA_MASK;
} else { /* output watchdog char if idle */
On Fri, Nov 24, 2006 at 09:58:16PM +0300, Alexey Dobriyan wrote:
> Andreas, is this correct?
> ---------------------------------
> SIXP_RX_DCD_MASK is 0x18, so the command below will make cmd 0 always.
> This is likely wrong.
>
> Signed-off-by: Alexey Dobriyan <[email protected]>
This one is already merged.
It's funny how long this bug survived - it's in the kernel since the 6pack
driver was first merged that is 2.1 or 2.2 ...
Ralf
On Fri, Nov 24, 2006 at 08:21:53PM +0000, Ralf Baechle wrote:
> On Fri, Nov 24, 2006 at 09:58:16PM +0300, Alexey Dobriyan wrote:
>
> > Andreas, is this correct?
> > ---------------------------------
> > SIXP_RX_DCD_MASK is 0x18, so the command below will make cmd 0 always.
> > This is likely wrong.
> >
> > Signed-off-by: Alexey Dobriyan <[email protected]>
>
> This one is already merged.
>
> It's funny how long this bug survived - it's in the kernel since the 6pack
> driver was first merged that is 2.1 or 2.2 ...
One more reason to perform more code reviews helped with automated tools.
We found this one and the rio's one while discussing with Jean Delvare
about such bugs, and firing a random grep to illustrate how easy it could
be to spot bugs similar to Alexey's "&&" instead of "&" ...
I think that we should at least take a look at all lines in the pre-processed
code having both '!' and '&' on the same line. There are a lot of them, but
divided by a sufficient number of volunteers, we might catch a bunch of them.
BTW, has anyone a good idea on how to make gcc dump the preprocessed files
for everything it builds ? I mean, just by changing some variables in the
Makefile.
> Ralf
Cheers,
Willy
On Fri, Nov 24, 2006 at 10:23:06PM +0100, Willy Tarreau wrote:
> One more reason to perform more code reviews helped with automated tools.
> We found this one and the rio's one while discussing with Jean Delvare
> about such bugs, and firing a random grep to illustrate how easy it could
> be to spot bugs similar to Alexey's "&&" instead of "&" ...
>
> I think that we should at least take a look at all lines in the pre-processed
> code having both '!' and '&' on the same line. There are a lot of them, but
> divided by a sufficient number of volunteers, we might catch a bunch of them.
>
> BTW, has anyone a good idea on how to make gcc dump the preprocessed files
> for everything it builds ? I mean, just by changing some variables in the
> Makefile.
It seems to might be about to re-invent sparse which is probably the
right framework for such semantic tests.
Ralf