These patches relate to conditional tests where & is used with a constant
that is always 0 and where | is used with a constant that is always 0 or
always non-zero.
The following semantic match finds these problems
(http://coccinelle.lip6.fr/):
@and@
identifier i;
expression e;
position p;
@@
((e & i@p) && ...)
@iszera@
identifier and.i;
position p;
@@
#define i 0
@othera@
identifier and.i;
expression e!=0;
@@
#define i e
@script:python depends on iszera && !othera@
p << and.p;
@@
cocci.print_main("",p)
@or@
identifier i;
expression e;
position p;
@@
((e | i@p) && ...)
@iszero@
identifier or.i;
position p;
@@
#define i 0
@othero@
identifier or.i;
expression e!=0;
@@
#define i e
@script:python depends on (othero && !iszero) || (iszero && !othero)@
p << or.p;
@@
cocci.print_main("",p)
From: Julia Lawall <[email protected]>
TRIG_ROUND_NEAREST is 0, so a bit-and with it is always false. The
value TRIG_ROUND_MASK covers the bits of the TRIG_ROUND constants, so
first pick those bits and then make the test using ==.
The same is done for TRIG_ROUND_UP for symmetry, even though bit-and would
be sufficient in this case.
This problem was found using Coccinelle (http://coccinelle.lip6.fr/).
Signed-off-by: Julia Lawall <[email protected]>
---
The TRIG_ROUND_UP case could be left as is, if that is preferred. Or a
temporary variable could be introduced to avoid anding with the mask twice.
Actually, the following code suggests that the mask is not necessary, == would be sufficient:
/* Only rounding flags are implemented */
cmd->flags &= TRIG_ROUND_NEAREST | TRIG_ROUND_UP | TRIG_ROUND_DOWN;
But using the mask is safer, if there are future extensions.
drivers/staging/comedi/drivers/me4000.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/comedi/drivers/me4000.c b/drivers/staging/comedi/drivers/me4000.c
index 8ca1b54..fc60117 100644
--- a/drivers/staging/comedi/drivers/me4000.c
+++ b/drivers/staging/comedi/drivers/me4000.c
@@ -949,10 +949,10 @@ static int ai_round_cmd_args(struct comedi_device *dev,
*init_ticks = (cmd->start_arg * 33) / 1000;
rest = (cmd->start_arg * 33) % 1000;
- if (cmd->flags & TRIG_ROUND_NEAREST) {
+ if ((cmd->flags & TRIG_ROUND_MASK) == TRIG_ROUND_NEAREST) {
if (rest > 33)
(*init_ticks)++;
- } else if (cmd->flags & TRIG_ROUND_UP) {
+ } else if ((cmd->flags & TRIG_ROUND_MASK) == TRIG_ROUND_UP) {
if (rest)
(*init_ticks)++;
}
@@ -962,10 +962,10 @@ static int ai_round_cmd_args(struct comedi_device *dev,
*scan_ticks = (cmd->scan_begin_arg * 33) / 1000;
rest = (cmd->scan_begin_arg * 33) % 1000;
- if (cmd->flags & TRIG_ROUND_NEAREST) {
+ if ((cmd->flags & TRIG_ROUND_MASK) == TRIG_ROUND_NEAREST) {
if (rest > 33)
(*scan_ticks)++;
- } else if (cmd->flags & TRIG_ROUND_UP) {
+ } else if ((cmd->flags & TRIG_ROUND_MASK) == TRIG_ROUND_UP) {
if (rest)
(*scan_ticks)++;
}
@@ -975,10 +975,10 @@ static int ai_round_cmd_args(struct comedi_device *dev,
*chan_ticks = (cmd->convert_arg * 33) / 1000;
rest = (cmd->convert_arg * 33) % 1000;
- if (cmd->flags & TRIG_ROUND_NEAREST) {
+ if ((cmd->flags & TRIG_ROUND_MASK) == TRIG_ROUND_NEAREST) {
if (rest > 33)
(*chan_ticks)++;
- } else if (cmd->flags & TRIG_ROUND_UP) {
+ } else if ((cmd->flags & TRIG_ROUND_MASK) == TRIG_ROUND_UP) {
if (rest)
(*chan_ticks)++;
}
From: Julia Lawall <[email protected]>
IO_DATA_PATH_WIDTH_8 is 0, so a bit-and with it is always false. The
value IO_DATA_PATH_WIDTH covers the bits of the IO_DATA_PATH constants, so
first pick those bits and then make the test using !=.
This problem was found using Coccinelle (http://coccinelle.lip6.fr/).
Signed-off-by: Julia Lawall <[email protected]>
---
drivers/ata/pata_pcmcia.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/ata/pata_pcmcia.c b/drivers/ata/pata_pcmcia.c
index a808ba0..776e749 100644
--- a/drivers/ata/pata_pcmcia.c
+++ b/drivers/ata/pata_pcmcia.c
@@ -170,7 +170,8 @@ static int pcmcia_check_one_config(struct pcmcia_device *pdev, void *priv_data)
{
int *is_kme = priv_data;
- if (!(pdev->resource[0]->flags & IO_DATA_PATH_WIDTH_8)) {
+ if ((pdev->resource[0]->flags & IO_DATA_PATH_WIDTH)
+ != IO_DATA_PATH_WIDTH_8) {
pdev->resource[0]->flags &= ~IO_DATA_PATH_WIDTH;
pdev->resource[0]->flags |= IO_DATA_PATH_WIDTH_AUTO;
}
From: Julia Lawall <[email protected]>
CPUIF_DSC is 0x40, so it would seem that & is wanted rather than |.
This problem was found using Coccinelle (http://coccinelle.lip6.fr/).
Signed-off-by: Julia Lawall <[email protected]>
---
drivers/net/can/cc770/cc770_platform.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/can/cc770/cc770_platform.c b/drivers/net/can/cc770/cc770_platform.c
index 53115ee..688371c 100644
--- a/drivers/net/can/cc770/cc770_platform.c
+++ b/drivers/net/can/cc770/cc770_platform.c
@@ -154,7 +154,7 @@ static int __devinit cc770_get_platform_data(struct platform_device *pdev,
struct cc770_platform_data *pdata = pdev->dev.platform_data;
priv->can.clock.freq = pdata->osc_freq;
- if (priv->cpu_interface | CPUIF_DSC)
+ if (priv->cpu_interface & CPUIF_DSC)
priv->can.clock.freq /= 2;
priv->clkout = pdata->cor;
priv->bus_config = pdata->bcr;
From: Julia Lawall <[email protected]>
IRQF_TRIGGER_HIGH is 0x00000004, so it seems that & was intended rather than |.
This problem was found using Coccinelle (http://coccinelle.lip6.fr/).
Signed-off-by: Julia Lawall <[email protected]>
---
drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
index e2480d1..bed208f 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
@@ -91,7 +91,7 @@ int brcmf_sdio_intr_register(struct brcmf_sdio_dev *sdiodev)
/* redirect, configure ane enable io for interrupt signal */
data = SDIO_SEPINT_MASK | SDIO_SEPINT_OE;
- if (sdiodev->irq_flags | IRQF_TRIGGER_HIGH)
+ if (sdiodev->irq_flags & IRQF_TRIGGER_HIGH)
data |= SDIO_SEPINT_ACT_HI;
brcmf_sdio_regwb(sdiodev, SDIO_CCCR_BRCM_SEPINT, data, &ret);
From: Julia Lawall <[email protected]>
READ is 0, so the result of the bit-and operation is 0. Rewrite with == as
done elsewhere in the same file.
This problem was found using Coccinelle (http://coccinelle.lip6.fr/).
Signed-off-by: Julia Lawall <[email protected]>
---
fs/direct-io.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/direct-io.c b/fs/direct-io.c
index 0c85fae..1faf4cb 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -1258,7 +1258,7 @@ do_blockdev_direct_IO(int rw, struct kiocb *iocb, struct inode *inode,
*/
BUG_ON(retval == -EIOCBQUEUED);
if (dio->is_async && retval == 0 && dio->result &&
- ((rw & READ) || (dio->result == sdio.size)))
+ ((rw == READ) || (dio->result == sdio.size)))
retval = -EIOCBQUEUED;
if (retval != -EIOCBQUEUED)
From: Julia Lawall <[email protected]>
PCH_UDC_DMA_LAST is 0x08000000 so a bit-or with this value always gives a
nonzero result. The test is rewritten as done elsewhere in the same file.
This problem was found using Coccinelle (http://coccinelle.lip6.fr/).
Signed-off-by: Julia Lawall <[email protected]>
---
drivers/usb/gadget/pch_udc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/pch_udc.c b/drivers/usb/gadget/pch_udc.c
index 1cfcc9e..79f7a53 100644
--- a/drivers/usb/gadget/pch_udc.c
+++ b/drivers/usb/gadget/pch_udc.c
@@ -2208,7 +2208,8 @@ static void pch_udc_complete_receiver(struct pch_udc_ep *ep)
return;
}
if ((td->status & PCH_UDC_BUFF_STS) == PCH_UDC_BS_DMA_DONE)
- if (td->status | PCH_UDC_DMA_LAST) {
+ if ((td_data->status & PCH_UDC_DMA_LAST)
+ == PCH_UDC_DMA_LAST) {
count = td->status & PCH_UDC_RXTX_BYTES;
break;
}
From: Julia Lawall <[email protected]>
IO_DATA_PATH_WIDTH_8 is 0, so a bit-and with it is always false. The
value IO_DATA_PATH_WIDTH covers the bits of the IO_DATA_PATH constants, so
first pick those bits and then make the test using !=.
This problem was found using Coccinelle (http://coccinelle.lip6.fr/).
Signed-off-by: Julia Lawall <[email protected]>
---
drivers/ide/ide-cs.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/ide/ide-cs.c b/drivers/ide/ide-cs.c
index 28e344e..a5e57bf 100644
--- a/drivers/ide/ide-cs.c
+++ b/drivers/ide/ide-cs.c
@@ -167,7 +167,8 @@ static int pcmcia_check_one_config(struct pcmcia_device *pdev, void *priv_data)
{
int *is_kme = priv_data;
- if (!(pdev->resource[0]->flags & IO_DATA_PATH_WIDTH_8)) {
+ if ((p1dev->resource[0]->flags & IO_DATA_PATH_WIDTH)
+ != IO_DATA_PATH_WIDTH_8) {
pdev->resource[0]->flags &= ~IO_DATA_PATH_WIDTH;
pdev->resource[0]->flags |= IO_DATA_PATH_WIDTH_AUTO;
}
On 06/06/2012 11:41 PM, Julia Lawall wrote:
> From: Julia Lawall <[email protected]>
>
> CPUIF_DSC is 0x40, so it would seem that & is wanted rather than |.
>
> This problem was found using Coccinelle (http://coccinelle.lip6.fr/).
>
> Signed-off-by: Julia Lawall <[email protected]>
Good catch, Joe Perches has already found and fixed that problem. It has
been fixed in:
dc605db can: cc770: Fix likely misuse of | for &
The commit is currently in David's net/master, and scheduled for the
v3.5 release.
Regards, Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
On 06/06/2012 02:41 PM, Julia Lawall wrote:
> From: Julia Lawall<[email protected]>
>
> IRQF_TRIGGER_HIGH is 0x00000004, so it seems that& was intended rather than |.
>
> This problem was found using Coccinelle (http://coccinelle.lip6.fr/).
>
> Signed-off-by: Julia Lawall<[email protected]>
Thanks, Julia. But this has already been fixed by Joe Perches [1] and
the patch has arrived at Linux wireless tree.
Franky
[1] https://lkml.org/lkml/2012/5/30/482
Hello.
On 07-06-2012 1:41, Julia Lawall wrote:
> From: Julia Lawall <[email protected]>
> PCH_UDC_DMA_LAST is 0x08000000 so a bit-or with this value always gives a
> nonzero result. The test is rewritten as done elsewhere in the same file.
> This problem was found using Coccinelle (http://coccinelle.lip6.fr/).
> Signed-off-by: Julia Lawall<[email protected]>
> ---
> drivers/usb/gadget/pch_udc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/pch_udc.c b/drivers/usb/gadget/pch_udc.c
> index 1cfcc9e..79f7a53 100644
> --- a/drivers/usb/gadget/pch_udc.c
> +++ b/drivers/usb/gadget/pch_udc.c
> @@ -2208,7 +2208,8 @@ static void pch_udc_complete_receiver(struct pch_udc_ep *ep)
> return;
> }
> if ((td->status& PCH_UDC_BUFF_STS) == PCH_UDC_BS_DMA_DONE)
> - if (td->status | PCH_UDC_DMA_LAST) {
> + if ((td_data->status& PCH_UDC_DMA_LAST)
> + == PCH_UDC_DMA_LAST) {
But why not simply:
if (td_data->status & PCH_UDC_DMA_LAST)
if the constant is single bit anyway? And are you sure about s/td/td_data/?
WBR, Sergei
On Thu, 7 Jun 2012, Sergei Shtylyov wrote:
> Hello.
>
> On 07-06-2012 1:41, Julia Lawall wrote:
>
>> From: Julia Lawall <[email protected]>
>
>> PCH_UDC_DMA_LAST is 0x08000000 so a bit-or with this value always gives a
>> nonzero result. The test is rewritten as done elsewhere in the same file.
>
>> This problem was found using Coccinelle (http://coccinelle.lip6.fr/).
>
>> Signed-off-by: Julia Lawall<[email protected]>
>
>> ---
>> drivers/usb/gadget/pch_udc.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/pch_udc.c b/drivers/usb/gadget/pch_udc.c
>> index 1cfcc9e..79f7a53 100644
>> --- a/drivers/usb/gadget/pch_udc.c
>> +++ b/drivers/usb/gadget/pch_udc.c
>> @@ -2208,7 +2208,8 @@ static void pch_udc_complete_receiver(struct
>> pch_udc_ep *ep)
>> return;
>> }
>> if ((td->status& PCH_UDC_BUFF_STS) == PCH_UDC_BS_DMA_DONE)
>> - if (td->status | PCH_UDC_DMA_LAST) {
>> + if ((td_data->status& PCH_UDC_DMA_LAST)
>> + == PCH_UDC_DMA_LAST) {
>
> But why not simply:
>
> if (td_data->status & PCH_UDC_DMA_LAST)
>
> if the constant is single bit anyway? And are you sure about s/td/td_data/?
Oops, thanks for noticing that. I'll send a corrected version.
thanks,
julia
Julia Lawall <[email protected]> writes:
> From: Julia Lawall <[email protected]>
>
> READ is 0, so the result of the bit-and operation is 0. Rewrite with == as
> done elsewhere in the same file.
>
> This problem was found using Coccinelle (http://coccinelle.lip6.fr/).
Interesting catch, that one. Yeah, we don't expect any readahead I/O to
be done via O_DIRECT. The test for rw == READ all over that file seems
fine to me, given that.
Reviewed-by: Jeff Moyer <[email protected]>
On Thu, 2012-06-07 at 16:23 +0200, Julia Lawall wrote:
> Oops, thanks for noticing that. I'll send a corrected version.
Hi Julia.
I should have let you know that I already submitted
a patch series for a simple treewide
"s/if (value | UC_CONSTANT)/if (value & UC_CONSTANT)/"
https://lkml.org/lkml/2012/5/30/625
Felipe has already picked this patch up.
https://lkml.org/lkml/2012/6/1/83
From: Julia Lawall <[email protected]>
Date: Wed, 6 Jun 2012 23:41:35 +0200
> - if (!(pdev->resource[0]->flags & IO_DATA_PATH_WIDTH_8)) {
> + if ((p1dev->resource[0]->flags & IO_DATA_PATH_WIDTH)
I'm really surprised someone as thorough as yourself did not
compile test this.
This is just more proof that it's absolutely pointless to make any
changes at all to the old IDE layer. Nobody really cares, and the
risk %99.999 of the time is purely to introduce regressions rather
than make forward progress.
On Thu, 2012-06-07 at 14:46 -0700, David Miller wrote:
> the
> risk %99.999 of the time is purely to introduce regressions rather
> than make forward progress.
MAINTAINERS | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index b54f50c..41d4586 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3404,7 +3404,7 @@ M: "David S. Miller" <[email protected]>
L: [email protected]
Q: http://patchwork.ozlabs.org/project/linux-ide/list/
T: git git://git.kernel.org/pub/scm/linux/kernel/git/davem/ide.git
-S: Maintained
+S: Watchdogged - Patch at your own peril
F: Documentation/ide/
F: drivers/ide/
F: include/linux/ide.h
On Thu, 7 Jun 2012, David Miller wrote:
> From: Julia Lawall <[email protected]>
> Date: Wed, 6 Jun 2012 23:41:35 +0200
>
>> - if (!(pdev->resource[0]->flags & IO_DATA_PATH_WIDTH_8)) {
>> + if ((p1dev->resource[0]->flags & IO_DATA_PATH_WIDTH)
>
> I'm really surprised someone as thorough as yourself did not
> compile test this.
Sorry, I changed the patch at the last minute, and it was indeed not a
good idea not to have compiled again. I will send another version, in
case it is useful.
julia
>From nobody Wed Jun 6 21:48:37 CEST 2012
From: Julia Lawall <[email protected]>
To: "David S. Miller" <[email protected]>
Cc: [email protected],[email protected],[email protected]
Subject: [PATCH 1/7] drivers/ide/ide-cs.c: adjust suspicious bit operation
From: Julia Lawall <[email protected]>
IO_DATA_PATH_WIDTH_8 is 0, so a bit-and with it is always false. The
value IO_DATA_PATH_WIDTH covers the bits of the IO_DATA_PATH constants, so
first pick those bits and then make the test using !=.
This problem was found using Coccinelle (http://coccinelle.lip6.fr/).
Signed-off-by: Julia Lawall <[email protected]>
---
drivers/ide/ide-cs.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/ide/ide-cs.c b/drivers/ide/ide-cs.c
index 28e344e..f1e922e 100644
--- a/drivers/ide/ide-cs.c
+++ b/drivers/ide/ide-cs.c
@@ -167,7 +167,8 @@ static int pcmcia_check_one_config(struct pcmcia_device *pdev, void *priv_data)
{
int *is_kme = priv_data;
- if (!(pdev->resource[0]->flags & IO_DATA_PATH_WIDTH_8)) {
+ if ((pdev->resource[0]->flags & IO_DATA_PATH_WIDTH)
+ != IO_DATA_PATH_WIDTH_8) {
pdev->resource[0]->flags &= ~IO_DATA_PATH_WIDTH;
pdev->resource[0]->flags |= IO_DATA_PATH_WIDTH_AUTO;
}
On Thu, Jun 07, 2012 at 08:05:44AM -0700, Joe Perches wrote:
> On Thu, 2012-06-07 at 16:23 +0200, Julia Lawall wrote:
> > Oops, thanks for noticing that. I'll send a corrected version.
>
> Hi Julia.
>
> I should have let you know that I already submitted
> a patch series for a simple treewide
> "s/if (value | UC_CONSTANT)/if (value & UC_CONSTANT)/"
> https://lkml.org/lkml/2012/5/30/625
>
You could CC them to [email protected]. Julia and I
used to tread on each other's toes until we hijacked the k-j list.
regards,
dan carpenter
On Fri, 2012-06-08 at 15:58 +0300, Dan Carpenter wrote:
> On Thu, Jun 07, 2012 at 08:05:44AM -0700, Joe Perches wrote:
> > I should have let you know that I already submitted
> > a patch series for a simple treewide
> > "s/if (value | UC_CONSTANT)/if (value & UC_CONSTANT)/"
> > https://lkml.org/lkml/2012/5/30/625
> You could CC them to [email protected].
Hi Dan,
Generally I submit patches to maintainers.
All of these 4 patches were picked up by them.
cheers, Joe
From: Julia Lawall <[email protected]>
Date: Fri, 8 Jun 2012 07:14:03 +0200 (CEST)
> @@ -167,7 +167,8 @@ static int pcmcia_check_one_config(struct
> pcmcia_device *pdev, void *priv_data)
Patch mangled by your email client or similar.
From: Julia Lawall <[email protected]>
IO_DATA_PATH_WIDTH_8 is 0, so a bit-and with it is always false. The
value IO_DATA_PATH_WIDTH covers the bits of the IO_DATA_PATH constants, so
first pick those bits and then make the test using !=.
This problem was found using Coccinelle (http://coccinelle.lip6.fr/).
Signed-off-by: Julia Lawall <[email protected]>
---
I hope to have fixed the mail client problem.
diff --git a/drivers/ide/ide-cs.c b/drivers/ide/ide-cs.c
index 28e344e..f1e922e 100644
--- a/drivers/ide/ide-cs.c
+++ b/drivers/ide/ide-cs.c
@@ -167,7 +167,8 @@ static int pcmcia_check_one_config(struct pcmcia_device *pdev, void *priv_data)
{
int *is_kme = priv_data;
- if (!(pdev->resource[0]->flags & IO_DATA_PATH_WIDTH_8)) {
+ if ((pdev->resource[0]->flags & IO_DATA_PATH_WIDTH)
+ != IO_DATA_PATH_WIDTH_8) {
pdev->resource[0]->flags &= ~IO_DATA_PATH_WIDTH;
pdev->resource[0]->flags |= IO_DATA_PATH_WIDTH_AUTO;
}
From: Julia Lawall <[email protected]>
Date: Tue, 12 Jun 2012 10:17:25 -0400 (EDT)
> From: Julia Lawall <[email protected]>
>
> IO_DATA_PATH_WIDTH_8 is 0, so a bit-and with it is always false. The
> value IO_DATA_PATH_WIDTH covers the bits of the IO_DATA_PATH constants, so
> first pick those bits and then make the test using !=.
>
> This problem was found using Coccinelle (http://coccinelle.lip6.fr/).
>
> Signed-off-by: Julia Lawall <[email protected]>
Applied, thanks.