Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)
Alexey Dobriyan wrote: > On Wed, Oct 10, 2007 at 04:45:46AM -0700, Josh Triplett wrote: >> Alexey Dobriyan wrote: >>> ["if (!x & y)" patch from [EMAIL PROTECTED] >>> ["if (!x & y)" patch from [EMAIL PROTECTED] >>> ["if (!x & y)" patches from [EMAIL PROTECTED] >>> >>> While we're at it, below is somewhat ugly sparse patch for detecting >>> "&& 0x" typos. >> Excellent idea! I think it applies to || as well. > > Sadly, yes. > > [PATCH] smctr: fix "|| 0x" typo > > IBM_PASS_SOURCE_ADDR is 1, so logically ORing it with status bits is > pretty useless. Do bitwise OR, instead. > > Signed-off-by: Alexey Dobriyan <[EMAIL PROTECTED]> > --- > > drivers/net/tokenring/smctr.c |2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > --- a/drivers/net/tokenring/smctr.c > +++ b/drivers/net/tokenring/smctr.c > @@ -3413,7 +3413,7 @@ static int smctr_make_tx_status_code(struct net_device > *dev, > tsv->svi = TRANSMIT_STATUS_CODE; > tsv->svl = S_TRANSMIT_STATUS_CODE; > > -tsv->svv[0] = ((tx_fstatus & 0x0100 >> 6) || IBM_PASS_SOURCE_ADDR); > +tsv->svv[0] = ((tx_fstatus & 0x0100 >> 6) | IBM_PASS_SOURCE_ADDR); > > /* Stripped frame status of Transmitted Frame */ > tsv->svv[1] = tx_fstatus & 0xff; Nice catch. I agree with Kyle, though, that fixing it may require some care to make sure it doesn't break things relying on the bug. >> I'll most likely >> add a -Wboolean-logic-on-bit-constant to turn this warning on. > > Ewww, more options. :-( With heuristic warnings like this, people need a way to turn them off. >> Any reason why this wouldn't apply to octal constants or to GCC's new >> binary constants? I can trivially modify this patch to handle those >> as well, just by dropping the check for an 'x' or 'X', and renaming the >> flag. > > OK, let me try too. Go for it. I look forward to your next patch. Thanks, Josh Triplett - 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/
Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)
--- Pierre Ossman <[EMAIL PROTECTED]> wrote:
> On Wed, 10 Oct 2007 14:45:40 +0400
> Alexey Dobriyan <[EMAIL PROTECTED]> wrote:
>
> > ["if (!x & y)" patch from [EMAIL PROTECTED]
> > ["if (!x & y)" patch from [EMAIL PROTECTED]
> > ["if (!x & y)" patches from [EMAIL PROTECTED]
> >
> > While we're at it, below is somewhat ugly sparse patch for detecting
> > "&& 0x" typos.
> >
>
> The maintainer for tifm is Alex Dubov, so cc:ing him.
>
> > drivers/mmc/host/tifm_sd.c:183:9: warning: dubious && 0x
> >
> > if ((r_data->flags & MMC_DATA_WRITE)
> > && DATA_CARRY)
> > writel(host->bounce_buf_data[0],
> > host->dev->addr
> > + SOCK_MMCSD_DATA);
> >
> > given that DATA_CARRY is always used together with
> > ->cmd_flags, this place is asking for obvious fixlet:
> >
> >
> > [PATCH] tifm_sd.c: fix DATA_CARRY check
> >
> > Signed-off-by: Alexey Dobriyan <[EMAIL PROTECTED]>
> > ---
> >
> > drivers/mmc/host/tifm_sd.c |2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > --- a/drivers/mmc/host/tifm_sd.c
> > +++ b/drivers/mmc/host/tifm_sd.c
> > @@ -180,7 +180,7 @@ static void tifm_sd_transfer_data(struct tifm_sd
> > *host) host->sg_pos++;
> > if (host->sg_pos == host->sg_len) {
> > if ((r_data->flags & MMC_DATA_WRITE)
> > - && DATA_CARRY)
> > + && (host->cmd_flags &
> > DATA_CARRY)) writel(host->bounce_buf_data[0],
> >host->dev->addr
> >+ SOCK_MMCSD_DATA);
> >
> >
> >
>
> Rgds
> Pierre
>
Oops. I wonder why this was never triggered (some users are having problems
with dma, so they use
PIO rather extensively). The patch is probably correct, but I can't do any
testing because I'm
currently on vacation.
Shape Yahoo! in your own image. Join our Network Research Panel today!
http://surveylink.yahoo.com/gmrs/yahoo_panel_invite.asp?a=7
-
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/
Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)
On Oct 11, 2007, at 03:35:37, Alexey Dobriyan wrote: Sadly, yes. [PATCH] smctr: fix "|| 0x" typo IBM_PASS_SOURCE_ADDR is 1, so logically ORing it with status bits is pretty useless. Do bitwise OR, instead. Signed-off-by: Alexey Dobriyan <[EMAIL PROTECTED]> --- drivers/net/tokenring/smctr.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- a/drivers/net/tokenring/smctr.c +++ b/drivers/net/tokenring/smctr.c @@ -3413,7 +3413,7 @@ static int smctr_make_tx_status_code(struct net_device *dev, tsv->svi = TRANSMIT_STATUS_CODE; tsv->svl = S_TRANSMIT_STATUS_CODE; -tsv->svv[0] = ((tx_fstatus & 0x0100 >> 6) || IBM_PASS_SOURCE_ADDR); +tsv->svv[0] = ((tx_fstatus & 0x0100 >> 6) | IBM_PASS_SOURCE_ADDR); /* Stripped frame status of Transmitted Frame */ tsv->svv[1] = tx_fstatus & 0xff; Hmm, here's a question for you: The old code was equivalent to "tsv- >svv[0] = 1;", what's your proof that we don't rely on this "bug" elsewhere in the code? In other words, this is a significant behavior change (albeit fixing an apparent bug) from what we've done for a while. You might want to do a git-blame on this bit of code to see who the last person to modify it was and ask them to test or confirm the patch first. The same general questions apply to the other logical-op bugs. Cheers, Kyle Moffett - 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/
Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)
On Wed, Oct 10, 2007 at 04:45:46AM -0700, Josh Triplett wrote: > Alexey Dobriyan wrote: > > ["if (!x & y)" patch from [EMAIL PROTECTED] > > ["if (!x & y)" patch from [EMAIL PROTECTED] > > ["if (!x & y)" patches from [EMAIL PROTECTED] > > > > While we're at it, below is somewhat ugly sparse patch for detecting > > "&& 0x" typos. > > Excellent idea! I think it applies to || as well. Sadly, yes. [PATCH] smctr: fix "|| 0x" typo IBM_PASS_SOURCE_ADDR is 1, so logically ORing it with status bits is pretty useless. Do bitwise OR, instead. Signed-off-by: Alexey Dobriyan <[EMAIL PROTECTED]> --- drivers/net/tokenring/smctr.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- a/drivers/net/tokenring/smctr.c +++ b/drivers/net/tokenring/smctr.c @@ -3413,7 +3413,7 @@ static int smctr_make_tx_status_code(struct net_device *dev, tsv->svi = TRANSMIT_STATUS_CODE; tsv->svl = S_TRANSMIT_STATUS_CODE; -tsv->svv[0] = ((tx_fstatus & 0x0100 >> 6) || IBM_PASS_SOURCE_ADDR); +tsv->svv[0] = ((tx_fstatus & 0x0100 >> 6) | IBM_PASS_SOURCE_ADDR); /* Stripped frame status of Transmitted Frame */ tsv->svv[1] = tx_fstatus & 0xff; > I'll most likely > add a -Wboolean-logic-on-bit-constant to turn this warning on. Ewww, more options. :-( > Any reason why this wouldn't apply to octal constants or to GCC's new > binary constants? I can trivially modify this patch to handle those > as well, just by dropping the check for an 'x' or 'X', and renaming the > flag. OK, let me try too. - 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/
Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)
On Wed, Oct 10, 2007 at 04:45:46AM -0700, Josh Triplett wrote: Alexey Dobriyan wrote: [if (!x y) patch from [EMAIL PROTECTED] [if (!x y) patch from [EMAIL PROTECTED] [if (!x y) patches from [EMAIL PROTECTED] While we're at it, below is somewhat ugly sparse patch for detecting 0x typos. Excellent idea! I think it applies to || as well. Sadly, yes. [PATCH] smctr: fix || 0x typo IBM_PASS_SOURCE_ADDR is 1, so logically ORing it with status bits is pretty useless. Do bitwise OR, instead. Signed-off-by: Alexey Dobriyan [EMAIL PROTECTED] --- drivers/net/tokenring/smctr.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- a/drivers/net/tokenring/smctr.c +++ b/drivers/net/tokenring/smctr.c @@ -3413,7 +3413,7 @@ static int smctr_make_tx_status_code(struct net_device *dev, tsv-svi = TRANSMIT_STATUS_CODE; tsv-svl = S_TRANSMIT_STATUS_CODE; -tsv-svv[0] = ((tx_fstatus 0x0100 6) || IBM_PASS_SOURCE_ADDR); +tsv-svv[0] = ((tx_fstatus 0x0100 6) | IBM_PASS_SOURCE_ADDR); /* Stripped frame status of Transmitted Frame */ tsv-svv[1] = tx_fstatus 0xff; I'll most likely add a -Wboolean-logic-on-bit-constant to turn this warning on. Ewww, more options. :-( Any reason why this wouldn't apply to octal constants or to GCC's new binary constants? I can trivially modify this patch to handle those as well, just by dropping the check for an 'x' or 'X', and renaming the flag. OK, let me try too. - 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/
Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)
On Oct 11, 2007, at 03:35:37, Alexey Dobriyan wrote: Sadly, yes. [PATCH] smctr: fix || 0x typo IBM_PASS_SOURCE_ADDR is 1, so logically ORing it with status bits is pretty useless. Do bitwise OR, instead. Signed-off-by: Alexey Dobriyan [EMAIL PROTECTED] --- drivers/net/tokenring/smctr.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- a/drivers/net/tokenring/smctr.c +++ b/drivers/net/tokenring/smctr.c @@ -3413,7 +3413,7 @@ static int smctr_make_tx_status_code(struct net_device *dev, tsv-svi = TRANSMIT_STATUS_CODE; tsv-svl = S_TRANSMIT_STATUS_CODE; -tsv-svv[0] = ((tx_fstatus 0x0100 6) || IBM_PASS_SOURCE_ADDR); +tsv-svv[0] = ((tx_fstatus 0x0100 6) | IBM_PASS_SOURCE_ADDR); /* Stripped frame status of Transmitted Frame */ tsv-svv[1] = tx_fstatus 0xff; Hmm, here's a question for you: The old code was equivalent to tsv- svv[0] = 1;, what's your proof that we don't rely on this bug elsewhere in the code? In other words, this is a significant behavior change (albeit fixing an apparent bug) from what we've done for a while. You might want to do a git-blame on this bit of code to see who the last person to modify it was and ask them to test or confirm the patch first. The same general questions apply to the other logical-op bugs. Cheers, Kyle Moffett - 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/
Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)
--- Pierre Ossman [EMAIL PROTECTED] wrote:
On Wed, 10 Oct 2007 14:45:40 +0400
Alexey Dobriyan [EMAIL PROTECTED] wrote:
[if (!x y) patch from [EMAIL PROTECTED]
[if (!x y) patch from [EMAIL PROTECTED]
[if (!x y) patches from [EMAIL PROTECTED]
While we're at it, below is somewhat ugly sparse patch for detecting
0x typos.
The maintainer for tifm is Alex Dubov, so cc:ing him.
drivers/mmc/host/tifm_sd.c:183:9: warning: dubious 0x
if ((r_data-flags MMC_DATA_WRITE)
DATA_CARRY)
writel(host-bounce_buf_data[0],
host-dev-addr
+ SOCK_MMCSD_DATA);
given that DATA_CARRY is always used together with
-cmd_flags, this place is asking for obvious fixlet:
[PATCH] tifm_sd.c: fix DATA_CARRY check
Signed-off-by: Alexey Dobriyan [EMAIL PROTECTED]
---
drivers/mmc/host/tifm_sd.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/drivers/mmc/host/tifm_sd.c
+++ b/drivers/mmc/host/tifm_sd.c
@@ -180,7 +180,7 @@ static void tifm_sd_transfer_data(struct tifm_sd
*host) host-sg_pos++;
if (host-sg_pos == host-sg_len) {
if ((r_data-flags MMC_DATA_WRITE)
-DATA_CARRY)
+(host-cmd_flags
DATA_CARRY)) writel(host-bounce_buf_data[0],
host-dev-addr
+ SOCK_MMCSD_DATA);
Rgds
Pierre
Oops. I wonder why this was never triggered (some users are having problems
with dma, so they use
PIO rather extensively). The patch is probably correct, but I can't do any
testing because I'm
currently on vacation.
Shape Yahoo! in your own image. Join our Network Research Panel today!
http://surveylink.yahoo.com/gmrs/yahoo_panel_invite.asp?a=7
-
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/
Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)
Alexey Dobriyan wrote: On Wed, Oct 10, 2007 at 04:45:46AM -0700, Josh Triplett wrote: Alexey Dobriyan wrote: [if (!x y) patch from [EMAIL PROTECTED] [if (!x y) patch from [EMAIL PROTECTED] [if (!x y) patches from [EMAIL PROTECTED] While we're at it, below is somewhat ugly sparse patch for detecting 0x typos. Excellent idea! I think it applies to || as well. Sadly, yes. [PATCH] smctr: fix || 0x typo IBM_PASS_SOURCE_ADDR is 1, so logically ORing it with status bits is pretty useless. Do bitwise OR, instead. Signed-off-by: Alexey Dobriyan [EMAIL PROTECTED] --- drivers/net/tokenring/smctr.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- a/drivers/net/tokenring/smctr.c +++ b/drivers/net/tokenring/smctr.c @@ -3413,7 +3413,7 @@ static int smctr_make_tx_status_code(struct net_device *dev, tsv-svi = TRANSMIT_STATUS_CODE; tsv-svl = S_TRANSMIT_STATUS_CODE; -tsv-svv[0] = ((tx_fstatus 0x0100 6) || IBM_PASS_SOURCE_ADDR); +tsv-svv[0] = ((tx_fstatus 0x0100 6) | IBM_PASS_SOURCE_ADDR); /* Stripped frame status of Transmitted Frame */ tsv-svv[1] = tx_fstatus 0xff; Nice catch. I agree with Kyle, though, that fixing it may require some care to make sure it doesn't break things relying on the bug. I'll most likely add a -Wboolean-logic-on-bit-constant to turn this warning on. Ewww, more options. :-( With heuristic warnings like this, people need a way to turn them off. Any reason why this wouldn't apply to octal constants or to GCC's new binary constants? I can trivially modify this patch to handle those as well, just by dropping the check for an 'x' or 'X', and renaming the flag. OK, let me try too. Go for it. I look forward to your next patch. Thanks, Josh Triplett - 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/
Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)
On Wed, 2007-10-10 at 11:08 -0700, Josh Triplett wrote: > Sparse has a notion of "integer constant expression" already, which it > uses to validate expressions used for things like bitfield widths or > array sizes. I could easily have Sparse warn on any use of an integer > constant expression as an operand of || or &&. However, I can imagine > that that might lead to some false positives when intentionally using > an integer constant expression in a condition and expecting the > compiler to optimize it out at compile time. I can imagine 0 or 1 being used, maybe -1, but hardly anything else. Maybe you could add the code printing the value and then get some statics on the actual Linux kernel to see which values are common and which are not? -- Regards, Pavel Roskin - 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/
Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)
On Wed, 10 Oct 2007 14:45:40 +0400
Alexey Dobriyan <[EMAIL PROTECTED]> wrote:
> ["if (!x & y)" patch from [EMAIL PROTECTED]
> ["if (!x & y)" patch from [EMAIL PROTECTED]
> ["if (!x & y)" patches from [EMAIL PROTECTED]
>
> While we're at it, below is somewhat ugly sparse patch for detecting
> "&& 0x" typos.
>
The maintainer for tifm is Alex Dubov, so cc:ing him.
> drivers/mmc/host/tifm_sd.c:183:9: warning: dubious && 0x
>
> if ((r_data->flags & MMC_DATA_WRITE)
> && DATA_CARRY)
> writel(host->bounce_buf_data[0],
> host->dev->addr
> + SOCK_MMCSD_DATA);
>
> given that DATA_CARRY is always used together with
> ->cmd_flags, this place is asking for obvious fixlet:
>
>
> [PATCH] tifm_sd.c: fix DATA_CARRY check
>
> Signed-off-by: Alexey Dobriyan <[EMAIL PROTECTED]>
> ---
>
> drivers/mmc/host/tifm_sd.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- a/drivers/mmc/host/tifm_sd.c
> +++ b/drivers/mmc/host/tifm_sd.c
> @@ -180,7 +180,7 @@ static void tifm_sd_transfer_data(struct tifm_sd
> *host) host->sg_pos++;
> if (host->sg_pos == host->sg_len) {
> if ((r_data->flags & MMC_DATA_WRITE)
> - && DATA_CARRY)
> + && (host->cmd_flags &
> DATA_CARRY)) writel(host->bounce_buf_data[0],
> host->dev->addr
> + SOCK_MMCSD_DATA);
>
>
>
Rgds
Pierre
signature.asc
Description: PGP signature
Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)
Morten Welinder wrote: >> While we're at it, below is somewhat ugly sparse patch for detecting >> "&& 0x" typos. > > Excellent idea, and there is something to be said about a low-footprint patch > like that. However, if you really want to capture this kind of bugs, you > would > need to have some kind "not a boolean" or "bitfield" attribute that > can propagate. > For example, you would want > > if (foo && (BAR | BAZ)) ...; > > with BAR and BAZ being hex constants to produce the same warning. > > Incidentally, it is probably not just hex constants that deserve this > treatment: > octal constants and variations of (1 << cst) are of the same nature. As well > as enums defined in such manners. Sparse has a notion of "integer constant expression" already, which it uses to validate expressions used for things like bitfield widths or array sizes. I could easily have Sparse warn on any use of an integer constant expression as an operand of || or &&. However, I can imagine that that might lead to some false positives when intentionally using an integer constant expression in a condition and expecting the compiler to optimize it out at compile time. - Josh Triplett - 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/
Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)
> While we're at it, below is somewhat ugly sparse patch for detecting > "&& 0x" typos. Excellent idea, and there is something to be said about a low-footprint patch like that. However, if you really want to capture this kind of bugs, you would need to have some kind "not a boolean" or "bitfield" attribute that can propagate. For example, you would want if (foo && (BAR | BAZ)) ...; with BAR and BAZ being hex constants to produce the same warning. Incidentally, it is probably not just hex constants that deserve this treatment: octal constants and variations of (1 << cst) are of the same nature. As well as enums defined in such manners. Morten - 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/
Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)
Alexey Dobriyan wrote: > ["if (!x & y)" patch from [EMAIL PROTECTED] > ["if (!x & y)" patch from [EMAIL PROTECTED] > ["if (!x & y)" patches from [EMAIL PROTECTED] > > While we're at it, below is somewhat ugly sparse patch for detecting > "&& 0x" typos. Excellent idea! I think it applies to || as well. I'll most likely add a -Wboolean-logic-on-bit-constant to turn this warning on. Any reason why this wouldn't apply to octal constants or to GCC's new binary constants? I can trivially modify this patch to handle those as well, just by dropping the check for an 'x' or 'X', and renaming the flag. As far as patch beauty goes, I think this patch looks just fine. - Josh Triplett - 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/
Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)
Alexey Dobriyan <[EMAIL PROTECTED]> writes:
> --- a/drivers/net/wireless/arlan.h
> +++ b/drivers/net/wireless/arlan.h
> @@ -485,7 +485,7 @@ struct arlan_private {
> #define clearClearInterrupt(dev){\
> writeControlRegister(dev,readControlRegister(dev) &
> ~ARLAN_CLEAR_INTERRUPT);}
> #define setPowerOff(dev){\
> - writeControlRegister(dev,readControlRegister(dev) | (ARLAN_POWER &&
> ARLAN_ACCESS));\
> + writeControlRegister(dev,readControlRegister(dev) | (ARLAN_POWER &
> ARLAN_ACCESS));\
Methinks that this actually wants (ARLAN_POWER | ARLAN_ACCESS), since
(ARLAN_POWER & ARLAN_ACCESS) == 0.
Andreas.
--
Andreas Schwab, SuSE Labs, [EMAIL PROTECTED]
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
-
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/
idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)
["if (!x & y)" patch from [EMAIL PROTECTED]
["if (!x & y)" patch from [EMAIL PROTECTED]
["if (!x & y)" patches from [EMAIL PROTECTED]
While we're at it, below is somewhat ugly sparse patch for detecting
"&& 0x" typos.
diff --git a/evaluate.c b/evaluate.c
index c0e9d17..186756b 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -877,6 +877,13 @@ static struct symbol *evaluate_logical(struct expression
*expr)
if (!evaluate_conditional(expr->right, 0))
return NULL;
+ if (expr->op == SPECIAL_LOGICAL_AND) {
+ if (expr->left->type == EXPR_VALUE && expr->left->orig_in_hex)
+ warning(expr->pos, "dubious && 0x");
+ if (expr->right->type == EXPR_VALUE && expr->right->orig_in_hex)
+ warning(expr->pos, "dubious && 0x");
+ }
+
expr->ctype = _ctype;
if (expr->flags) {
if (!(expr->left->flags & expr->right->flags & Int_const_expr))
diff --git a/expression.c b/expression.c
index 289927a..9bcf586 100644
--- a/expression.c
+++ b/expression.c
@@ -361,6 +361,7 @@ got_it:
expr->flags = Int_const_expr;
expr->ctype = ctype_integer(modifiers);
expr->value = value;
+ expr->orig_in_hex = (str[0] == '0' && (str[1] == 'x' || str[1] == 'X'));
return;
Eoverflow:
error_die(expr->pos, "constant %s is too big even for unsigned long
long",
@@ -70,6 +70,7 @@ struct expression {
struct {
unsigned long long value;
unsigned taint;
+ unsigned int orig_in_hex:1;
};
// EXPR_FVALUE
Here are wonders that it found:
drivers/media/video/se401.c:1283:13: warning: dubious && 0x
if (!cp[2] && SE401_FORMAT_BAYER) {
err("Bayer format not supported!");
return 1;
}
known issue, I sent a patch about it.
drivers/mmc/host/tifm_sd.c:183:9: warning: dubious && 0x
if ((r_data->flags & MMC_DATA_WRITE)
&& DATA_CARRY)
writel(host->bounce_buf_data[0],
host->dev->addr
+ SOCK_MMCSD_DATA);
given that DATA_CARRY is always used together with ->cmd_flags,
this place is asking for obvious fixlet:
[PATCH] tifm_sd.c: fix DATA_CARRY check
Signed-off-by: Alexey Dobriyan <[EMAIL PROTECTED]>
---
drivers/mmc/host/tifm_sd.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/drivers/mmc/host/tifm_sd.c
+++ b/drivers/mmc/host/tifm_sd.c
@@ -180,7 +180,7 @@ static void tifm_sd_transfer_data(struct tifm_sd *host)
host->sg_pos++;
if (host->sg_pos == host->sg_len) {
if ((r_data->flags & MMC_DATA_WRITE)
- && DATA_CARRY)
+ && (host->cmd_flags & DATA_CARRY))
writel(host->bounce_buf_data[0],
host->dev->addr
+ SOCK_MMCSD_DATA);
drivers/net/wireless/arlan-main.c:439:3: warning: dubious && 0x
drivers/net/wireless/arlan-main.c:439:3: warning: dubious && 0x
drivers/net/wireless/arlan-main.c:439:3: warning: dubious && 0x
drivers/net/wireless/arlan-main.c:439:3: warning: dubious && 0x
#define setPowerOff(dev){\
writeControlRegister(dev,readControlRegister(dev) |
(ARLAN_POWER && ARLAN_ACCESS));
^^^
[PATCH] arlan: fix "x && y" typo
Signed-off-by: Alexey Dobriyan <[EMAIL PROTECTED]>
---
drivers/net/wireless/arlan.h |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/drivers/net/wireless/arlan.h
+++ b/drivers/net/wireless/arlan.h
@@ -485,7 +485,7 @@ struct arlan_private {
#define clearClearInterrupt(dev){\
writeControlRegister(dev,readControlRegister(dev) &
~ARLAN_CLEAR_INTERRUPT);}
#define setPowerOff(dev){\
- writeControlRegister(dev,readControlRegister(dev) | (ARLAN_POWER &&
ARLAN_ACCESS));\
+ writeControlRegister(dev,readControlRegister(dev) | (ARLAN_POWER &
ARLAN_ACCESS));\
writeControlRegister(dev,readControlRegister(dev) & ~ARLAN_ACCESS);}
#define setPowerOn(dev){\
writeControlRegister(dev,readControlRegister(dev) & ~(ARLAN_POWER)); }
-
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/
idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)
[if (!x y) patch from [EMAIL PROTECTED]
[if (!x y) patch from [EMAIL PROTECTED]
[if (!x y) patches from [EMAIL PROTECTED]
While we're at it, below is somewhat ugly sparse patch for detecting
0x typos.
diff --git a/evaluate.c b/evaluate.c
index c0e9d17..186756b 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -877,6 +877,13 @@ static struct symbol *evaluate_logical(struct expression
*expr)
if (!evaluate_conditional(expr-right, 0))
return NULL;
+ if (expr-op == SPECIAL_LOGICAL_AND) {
+ if (expr-left-type == EXPR_VALUE expr-left-orig_in_hex)
+ warning(expr-pos, dubious 0x);
+ if (expr-right-type == EXPR_VALUE expr-right-orig_in_hex)
+ warning(expr-pos, dubious 0x);
+ }
+
expr-ctype = bool_ctype;
if (expr-flags) {
if (!(expr-left-flags expr-right-flags Int_const_expr))
diff --git a/expression.c b/expression.c
index 289927a..9bcf586 100644
--- a/expression.c
+++ b/expression.c
@@ -361,6 +361,7 @@ got_it:
expr-flags = Int_const_expr;
expr-ctype = ctype_integer(modifiers);
expr-value = value;
+ expr-orig_in_hex = (str[0] == '0' (str[1] == 'x' || str[1] == 'X'));
return;
Eoverflow:
error_die(expr-pos, constant %s is too big even for unsigned long
long,
@@ -70,6 +70,7 @@ struct expression {
struct {
unsigned long long value;
unsigned taint;
+ unsigned int orig_in_hex:1;
};
// EXPR_FVALUE
Here are wonders that it found:
drivers/media/video/se401.c:1283:13: warning: dubious 0x
if (!cp[2] SE401_FORMAT_BAYER) {
err(Bayer format not supported!);
return 1;
}
known issue, I sent a patch about it.
drivers/mmc/host/tifm_sd.c:183:9: warning: dubious 0x
if ((r_data-flags MMC_DATA_WRITE)
DATA_CARRY)
writel(host-bounce_buf_data[0],
host-dev-addr
+ SOCK_MMCSD_DATA);
given that DATA_CARRY is always used together with -cmd_flags,
this place is asking for obvious fixlet:
[PATCH] tifm_sd.c: fix DATA_CARRY check
Signed-off-by: Alexey Dobriyan [EMAIL PROTECTED]
---
drivers/mmc/host/tifm_sd.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/drivers/mmc/host/tifm_sd.c
+++ b/drivers/mmc/host/tifm_sd.c
@@ -180,7 +180,7 @@ static void tifm_sd_transfer_data(struct tifm_sd *host)
host-sg_pos++;
if (host-sg_pos == host-sg_len) {
if ((r_data-flags MMC_DATA_WRITE)
-DATA_CARRY)
+(host-cmd_flags DATA_CARRY))
writel(host-bounce_buf_data[0],
host-dev-addr
+ SOCK_MMCSD_DATA);
drivers/net/wireless/arlan-main.c:439:3: warning: dubious 0x
drivers/net/wireless/arlan-main.c:439:3: warning: dubious 0x
drivers/net/wireless/arlan-main.c:439:3: warning: dubious 0x
drivers/net/wireless/arlan-main.c:439:3: warning: dubious 0x
#define setPowerOff(dev){\
writeControlRegister(dev,readControlRegister(dev) |
(ARLAN_POWER ARLAN_ACCESS));
^^^
[PATCH] arlan: fix x y typo
Signed-off-by: Alexey Dobriyan [EMAIL PROTECTED]
---
drivers/net/wireless/arlan.h |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/drivers/net/wireless/arlan.h
+++ b/drivers/net/wireless/arlan.h
@@ -485,7 +485,7 @@ struct arlan_private {
#define clearClearInterrupt(dev){\
writeControlRegister(dev,readControlRegister(dev)
~ARLAN_CLEAR_INTERRUPT);}
#define setPowerOff(dev){\
- writeControlRegister(dev,readControlRegister(dev) | (ARLAN_POWER
ARLAN_ACCESS));\
+ writeControlRegister(dev,readControlRegister(dev) | (ARLAN_POWER
ARLAN_ACCESS));\
writeControlRegister(dev,readControlRegister(dev) ~ARLAN_ACCESS);}
#define setPowerOn(dev){\
writeControlRegister(dev,readControlRegister(dev) ~(ARLAN_POWER)); }
-
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/
Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)
Alexey Dobriyan [EMAIL PROTECTED] writes:
--- a/drivers/net/wireless/arlan.h
+++ b/drivers/net/wireless/arlan.h
@@ -485,7 +485,7 @@ struct arlan_private {
#define clearClearInterrupt(dev){\
writeControlRegister(dev,readControlRegister(dev)
~ARLAN_CLEAR_INTERRUPT);}
#define setPowerOff(dev){\
- writeControlRegister(dev,readControlRegister(dev) | (ARLAN_POWER
ARLAN_ACCESS));\
+ writeControlRegister(dev,readControlRegister(dev) | (ARLAN_POWER
ARLAN_ACCESS));\
Methinks that this actually wants (ARLAN_POWER | ARLAN_ACCESS), since
(ARLAN_POWER ARLAN_ACCESS) == 0.
Andreas.
--
Andreas Schwab, SuSE Labs, [EMAIL PROTECTED]
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
And now for something completely different.
-
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/
Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)
Alexey Dobriyan wrote: [if (!x y) patch from [EMAIL PROTECTED] [if (!x y) patch from [EMAIL PROTECTED] [if (!x y) patches from [EMAIL PROTECTED] While we're at it, below is somewhat ugly sparse patch for detecting 0x typos. Excellent idea! I think it applies to || as well. I'll most likely add a -Wboolean-logic-on-bit-constant to turn this warning on. Any reason why this wouldn't apply to octal constants or to GCC's new binary constants? I can trivially modify this patch to handle those as well, just by dropping the check for an 'x' or 'X', and renaming the flag. As far as patch beauty goes, I think this patch looks just fine. - Josh Triplett - 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/
Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)
While we're at it, below is somewhat ugly sparse patch for detecting 0x typos. Excellent idea, and there is something to be said about a low-footprint patch like that. However, if you really want to capture this kind of bugs, you would need to have some kind not a boolean or bitfield attribute that can propagate. For example, you would want if (foo (BAR | BAZ)) ...; with BAR and BAZ being hex constants to produce the same warning. Incidentally, it is probably not just hex constants that deserve this treatment: octal constants and variations of (1 cst) are of the same nature. As well as enums defined in such manners. Morten - 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/
Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)
On Wed, 10 Oct 2007 14:45:40 +0400
Alexey Dobriyan [EMAIL PROTECTED] wrote:
[if (!x y) patch from [EMAIL PROTECTED]
[if (!x y) patch from [EMAIL PROTECTED]
[if (!x y) patches from [EMAIL PROTECTED]
While we're at it, below is somewhat ugly sparse patch for detecting
0x typos.
The maintainer for tifm is Alex Dubov, so cc:ing him.
drivers/mmc/host/tifm_sd.c:183:9: warning: dubious 0x
if ((r_data-flags MMC_DATA_WRITE)
DATA_CARRY)
writel(host-bounce_buf_data[0],
host-dev-addr
+ SOCK_MMCSD_DATA);
given that DATA_CARRY is always used together with
-cmd_flags, this place is asking for obvious fixlet:
[PATCH] tifm_sd.c: fix DATA_CARRY check
Signed-off-by: Alexey Dobriyan [EMAIL PROTECTED]
---
drivers/mmc/host/tifm_sd.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/drivers/mmc/host/tifm_sd.c
+++ b/drivers/mmc/host/tifm_sd.c
@@ -180,7 +180,7 @@ static void tifm_sd_transfer_data(struct tifm_sd
*host) host-sg_pos++;
if (host-sg_pos == host-sg_len) {
if ((r_data-flags MMC_DATA_WRITE)
- DATA_CARRY)
+ (host-cmd_flags
DATA_CARRY)) writel(host-bounce_buf_data[0],
host-dev-addr
+ SOCK_MMCSD_DATA);
Rgds
Pierre
signature.asc
Description: PGP signature
Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)
Morten Welinder wrote: While we're at it, below is somewhat ugly sparse patch for detecting 0x typos. Excellent idea, and there is something to be said about a low-footprint patch like that. However, if you really want to capture this kind of bugs, you would need to have some kind not a boolean or bitfield attribute that can propagate. For example, you would want if (foo (BAR | BAZ)) ...; with BAR and BAZ being hex constants to produce the same warning. Incidentally, it is probably not just hex constants that deserve this treatment: octal constants and variations of (1 cst) are of the same nature. As well as enums defined in such manners. Sparse has a notion of integer constant expression already, which it uses to validate expressions used for things like bitfield widths or array sizes. I could easily have Sparse warn on any use of an integer constant expression as an operand of || or . However, I can imagine that that might lead to some false positives when intentionally using an integer constant expression in a condition and expecting the compiler to optimize it out at compile time. - Josh Triplett - 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/
Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)
On Wed, 2007-10-10 at 11:08 -0700, Josh Triplett wrote: Sparse has a notion of integer constant expression already, which it uses to validate expressions used for things like bitfield widths or array sizes. I could easily have Sparse warn on any use of an integer constant expression as an operand of || or . However, I can imagine that that might lead to some false positives when intentionally using an integer constant expression in a condition and expecting the compiler to optimize it out at compile time. I can imagine 0 or 1 being used, maybe -1, but hardly anything else. Maybe you could add the code printing the value and then get some statics on the actual Linux kernel to see which values are common and which are not? -- Regards, Pavel Roskin - 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/
