| Message ID | 20081222191507.11807.50794.stgit@vmbox.hanneseder.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
Hannes Eder <hannes@hanneseder.net> writes: > Fix this sparse warnings: Or: break the formating to silence sparse. > drivers/net/atp.c:811:8: warning: do-while statement is not a compound statement > drivers/net/atp.c:813:8: warning: do-while statement is not a compound statement > drivers/net/atp.c:815:11: warning: do-while statement is not a compound statement > drivers/net/atp.c:817:11: warning: do-while statement is not a compound statement > --- a/drivers/net/starfire.c > +++ b/drivers/net/starfire.c > @@ -882,9 +882,9 @@ static int mdio_read(struct net_device *dev, int phy_id, int location) > void __iomem *mdio_addr = np->base + MIICtrl + (phy_id<<7) + (location<<2); > int result, boguscnt=1000; > /* ??? Should we add a busy-wait here? */ > - do > + do { > result = readl(mdio_addr); > - while ((result & 0xC0000000) != 0x80000000 && --boguscnt > 0); > + } while ((result & 0xC0000000) != 0x80000000 && --boguscnt > 0); > if (boguscnt == 0) > return 0; > if ((result & 0xffff) == 0xffff) I don't know how one could think the above is an improvement. This "do-while statement is not a compound statement" warning is pure nonsense.
2008/12/22 Krzysztof Halasa <khc@pm.waw.pl>: >> --- a/drivers/net/starfire.c >> +++ b/drivers/net/starfire.c >> @@ -882,9 +882,9 @@ static int mdio_read(struct net_device *dev, int phy_id, int location) >> void __iomem *mdio_addr = np->base + MIICtrl + (phy_id<<7) + (location<<2); >> int result, boguscnt=1000; >> /* ??? Should we add a busy-wait here? */ >> - do >> + do { >> result = readl(mdio_addr); >> - while ((result & 0xC0000000) != 0x80000000 && --boguscnt > 0); >> + } while ((result & 0xC0000000) != 0x80000000 && --boguscnt > 0); >> if (boguscnt == 0) >> return 0; >> if ((result & 0xffff) == 0xffff) > > > I don't know how one could think the above is an improvement. For this specific issue the important aspect is to improve readability (and not just eventually satisfy some warning from a tool), which I assume there is no disagrement on. Now what constitutes improved readbility on the other hand is another issue, and I guess there are next to as many oppinions as developers... :) I would most certainly prefer to have code look consistently and always have brackets, even in the case of just one body line. Of secondary importance is the benefit that always using brackets makes them much more merge friendly. Consider the following scenario: Common base: do result = readl(mdio_addr); while ((result & 0xC0000000) != 0x80000000 && --boguscnt > 0); Branch 1 do { result = readl(mdio_addr); do_something(); } while ((result & 0xC0000000) != 0x80000000 && --boguscnt > 0); Branch 2 do result = readl(mdio_addr); while (NOT_OK(result) && --boguscnt > 0); In this case if both branch 1 and 2 are merged, there will be a merge conflict that has to be resolved manually. If the brackets had been in place from the start tools should be able to resolv this automatically. BR Håkon Løvdal
"Håkon Løvdal" <hlovdal@gmail.com> writes: > For this specific issue the important aspect is to improve readability > (and not just eventually satisfy some warning from a tool), which I > assume there is no disagrement on. Precisely. > Now what constitutes improved > readbility on the other hand is another issue, and I guess there are > next to as many oppinions as developers... :) Yes. Thus the brackets (or lack of them) of do-while loops should never be forced as a part of coding standard/tools. This is just an unimportant detail of personal style and trying to force it is simply damaging to all of us. > Of secondary importance is the benefit that always using brackets > makes them much more merge friendly. There are many ways to make the code more merge friendly at a cost of readability. Hope we don't go this way.
On Tue, 2008-12-23 at 17:31 +0100, Krzysztof Halasa wrote: > "Håkon Løvdal" <hlovdal@gmail.com> writes: > > > For this specific issue the important aspect is to improve readability > > (and not just eventually satisfy some warning from a tool), which I > > assume there is no disagrement on. > > Precisely. > > > Now what constitutes improved > > readbility on the other hand is another issue, and I guess there are > > next to as many oppinions as developers... :) > > Yes. Thus the brackets (or lack of them) of do-while loops should > never be forced as a part of coding standard/tools. This is just an > unimportant detail of personal style and trying to force it is simply > damaging to all of us. > > > Of secondary importance is the benefit that always using brackets > > makes them much more merge friendly. > > There are many ways to make the code more merge friendly at a cost of > readability. Hope we don't go this way. Linus himself added that particular warning to sparse...may want to check with him the reason for it. Harvey -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Harvey Harrison <harvey.harrison@gmail.com> writes: >> There are many ways to make the code more merge friendly at a cost of >> readability. Hope we don't go this way. > > Linus himself added that particular warning to sparse...may want to check > with him the reason for it. Once again, this is a personal thing, and a harmless one. Sometimes I think using this sparse thing does more harm than good. Not that the tool itself is bad - I find it really useful, many sparse warnings actually did cause code improvement. It's like with knives, there are good uses and a bit less good ones.
On Tue, 23 Dec 2008, Krzysztof Halasa wrote: > Harvey Harrison <harvey.harrison@gmail.com> writes: > > >> There are many ways to make the code more merge friendly at a cost of > >> readability. Hope we don't go this way. > > > > Linus himself added that particular warning to sparse...may want to check > > with him the reason for it. > > Once again, this is a personal thing, and a harmless one. It's more than that. I added the check after some person who had been programming the kernel (and thus was supposedly fluent in C) literally could not parse a macro that had "do x while (y)" in it. Why? Because it's so uncommon, and because "while (y)" on its own means something totally different. So the syntactic sugar to _always_ have do-while loops have that brace is a way to avoid one of the rather few places where the C language has syntax that is very context-dependent. Another example of this is "sizeof". The kernel universally (I hope) has parenthesis around the sizeof argument, even though it's clearly not required by the C language. It's a coding standard. And quite frankly, anybody who works on gcc has no place complaining about sparse coding standard warnings. They are a _hell_ of a lot better than some of the really crazy warnings gcc spews out with "-W". At least the sparse warnings you can make go away while making the code more understandable. Some of the -W warnings are unfixable without breaking the source code. Linus -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Linus Torvalds <torvalds@linux-foundation.org> writes: > It's more than that. I added the check after some person who had been > programming the kernel (and thus was supposedly fluent in C) literally > could not parse a macro that had "do x while (y)" in it. A literally that simple macro, or a complicated one? I'm all for using brackets when there is / could be some possibility of increasing code readability. E.g. I always use parentheses in a nested if-if, because if (x) if (y) a; else c; may be confusing, especially if formatted differently. if (x) a; else if (y) b; etc. is simple and unambiguous and I don't put the braces. So is a case like do x; while (y); It can't be made more clear with brackets. IOW: improving the style is great. Changing it only to silence some tool is not. > Another example of this is "sizeof". The kernel universally (I hope) has > parenthesis around the sizeof argument, even though it's clearly not > required by the C language. > > It's a coding standard. Right, but they (at least for me) make it more readable. kmalloc(sizeof i) just doesn't look good, the operator looks like a variable name. But there is this return statement. Some people tend to write return (x); I simply write return x; It's clear, and so is a simple do-while. > And quite frankly, anybody who works on gcc has no place complaining about > sparse coding standard warnings. They are a _hell_ of a lot better than > some of the really crazy warnings gcc spews out with "-W". At least the > sparse warnings you can make go away while making the code more > understandable. Some of the -W warnings are unfixable without breaking the > source code. :-) BTW I think I may use sparse differently. I can see false gcc warnings every time the project is being built. OTOH I run sparse only when I have some (almost) completed project (a patch, a driver etc). I make sure the remaining sparse warnings are (from my POV) invalid and it won't spew them on next build again.
On Wed, 24 Dec 2008, Krzysztof Halasa wrote: > > So is a case like > do > x; > while (y); > It can't be made more clear with brackets. Oh yes it can. People look at that, and it's so uncommon that they literally believe it is a mis-indent. Your example with nested if-statements are totally pointless, because you didn't even apparently understand my comment about "while()" having two totally different meanings (which is not true of "if()"), nor realize the importance of how common something is. Common patterns become things that people take for granted and don't have any trouble with. In contrast, do-while without braces is _extremely_ uncommon. > IOW: improving the style is great. Changing it only to silence some > tool is not. Sorry, you're wrong. It's not changed to silence some tool. THIS IS THE KERNEL CODING STYLE. I don't care one whit about your personal coding style. The kernel has brances. End of discussion. sparse complains about lack of them. Comprende? > Right, but they (at least for me) make it more readable. I don't care. > kmalloc(sizeof i) just doesn't look good, the operator looks like > a variable name. And I agree with you. "sizeof i" doesn't look good. It's uncommon, and doesn't match peoples expectations. > But there is this return statement. Some people tend to write > return (x); I simply write return x; I do to. And it's the common thing to do, and only totally confused people think that 'return' is a somehow remotely like a "function" of its arguments (the way 'sizeof' is - sizeof _is_ a function of its arguments, albeit a very rare one). > It's clear, and so is a simple do-while. No. "return x;" is clear because it's the common thing, which means that peopel are good at reading it. Another example of "common vs non-common" is this: if (0 <= x) do something.. is something that crazy people do (sadly, one of the crazy people taught the git maintainer C programming, so now even sane people do it). It's crazy because it's uncommon, which means that most people have to think about it A LOT MORE than about if (x >= 0) do something.. even though technically both are obviously EXACTLY THE SAME THING. Can you see the argument? Doing things the common way is important, because it allows people to see what they mean without having to think about it. They just scan it, and the meaning is clear. And that's why "do while" without braces is bad. If you scan it quickly on its own, you may well end up just seeing the while (x); part, and get confused ("oh, a delay loop"). But if you see } while (x); you aren't confused, because the latter one is clearly an ending condition of a do-while loop, IN A WAY THAT THE FIRST ONE IS NOT! See? do-while is very special, because as mentioned, "while" is a really magic C keyword that has two TOTALLY DIFFERENT meanings. Don't make people look for the "do". Linus -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Linus Torvalds <torvalds@linux-foundation.org> writes: > Oh yes it can. People look at that, and it's so uncommon that they > literally believe it is a mis-indent. People learn, or should, through the life :-) I'm not sure being common or less common does matter here much. OTOH I think it's pretty common. Approx as common as while (x) y is, isn't it? Except in macros: do-while(0) is commonly used in macros because it "eats" the semicolon. (do { xxx; yyy; zzz } while (0) is also quite common instead of "goto" to add multiple exits from a long block). > Your example with nested if-statements are totally pointless, because you > didn't even apparently understand my comment about "while()" having two > totally different meanings (which is not true of "if()"), nor realize the > importance of how common something is. Only apparently. If there is no ambiguity, there is no problem, the double meaning does not seem relevant. You tell me that do expr; while (condition); (with precisely this formatting) can be unclear? Or you say do { expr; } while (condition); is better? If it is, then certainly not for me. Now: are we going to write {} because you say so, even if it makes the code worse? Or should we rather aim at improving the code, even if only a bit? (I admit that, IMO, the second example is worse = less readable, but only a small bit). > Sorry, you're wrong. It's not changed to silence some tool. Come on, you have it plain in the subject - "fix sparse warnings". If it was "improve obscure notation" I wouldn't say a word. > And I agree with you. "sizeof i" doesn't look good. It's uncommon, and > doesn't match peoples expectations. But it doesn't matter if it's common or not. It doesn't look good and THIS IS WHY it's uncommon/nonexistent. Not the other way around. There are many uncommon things in a project like a Linux kernel. There are some macros which take me a while :-) to understand and they thankfully don't spew warnings. > Another example of "common vs non-common" is this: > > if (0 <= x) > do something.. > > is something that crazy people do (sadly, one of the crazy people taught > the git maintainer C programming, so now even sane people do it). It's > crazy because it's uncommon, which means that most people have to think > about it A LOT MORE than about > > if (x >= 0) > do something.. No. It's crazy not because it's uncommon, but because this is how we have been taught in school. I don't know reasons for "0 >= x" but I know one for if (0 == x) do something.. It's because people sometimes write "=" instead of "==" and "0 = x" doesn't make sense to gcc. I think it's not a valid reason, and (unless you use (()) which is IMHO also crazy) gcc will warn about if (x = 0) (though I think the warning is stupid and "if (a = b)" is a valid usage if a and b are short). It's simple: one has to see the difference between if (x = 0) vs if (x == 0), there is no way around it. if (0 >= x), I don't know. > Can you see the argument? Doing things the common way is important, > because it allows people to see what they mean without having to think > about it. They just scan it, and the meaning is clear. Then I must be different. Things are clear to me if they are clear, even if they are uncommon. And common things aren't necessarily clear. Hmm, I should have expected something like that. > And that's why "do while" without braces is bad. If you scan it quickly on > its own, you may well end up just seeing the > > while (x); > > part, and get confused ("oh, a delay loop"). Nope. It raises a red flag in my head immediately. We don't do such busy loops generally, and if we do, I would write it as: while (x) ; > But if you see > > } while (x); > > you aren't confused, because the latter one is clearly an ending condition > of a do-while loop, IN A WAY THAT THE FIRST ONE IS NOT! > > See? No. I must be different. > do-while is very special, because as mentioned, "while" is a really magic > C keyword that has two TOTALLY DIFFERENT meanings. Probably. I just don't see it. For me, every code fragment is different, and a program like sparse can't even come close to know what's clear for me and what is not. I use sparse as a great help - but only that, a help.
On Wed, 24 Dec 2008, Krzysztof Halasa wrote: > > People learn, or should, through the life :-) Sure. But you should learn about the things that matter - not learn to avoid the stupid pitfalls that come from confusingly doing things so that they visually look similar even when they do different things. So don't make people learn by putting traps in their face. That just wastes everybodys time. > I'm not sure being common or less common does matter here much. > > OTOH I think it's pretty common. Approx as common as while (x) y is, > isn't it? I doubt it. It certainly wasn't in the kernel. When we added the sparse warning, I think we got a couple of hits. Anyway, not worth discussing. The fact is, the kernel does not accept do while without braces. I told you why. You can ignore it. I'll ignore you. Linus -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Krzysztof Halasa <khc@pm.waw.pl> writes: > Linus Torvalds <torvalds@linux-foundation.org> writes: > ... >> Another example of "common vs non-common" is this: >> >> if (0 <= x) >> do something.. >> >> is something that crazy people do (sadly, one of the crazy people taught >> the git maintainer C programming, so now even sane people do it). It's >> crazy because it's uncommon, which means that most people have to think >> about it A LOT MORE than about >> >> if (x >= 0) >> do something.. > > No. It's crazy not because it's uncommon, but because this is how we > have been taught in school. > > I don't know reasons for "0 >= x" but I know one for > if (0 == x) > do something.. > > It's because people sometimes write "=" instead of "==" and "0 = x" > doesn't make sense to gcc. It does not have anything to do with the assignment confusion. It is "textual order should reflect actual order" (aka "have number line in your head when you write your comparison conditional"): http://thread.gmane.org/gmane.comp.version-control.git/3903/focus=4126 Even if it may make logical sense, I would not suggest using it when other people are not familiar with the convention, though. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Linus Torvalds <torvalds@linux-foundation.org> writes: > Anyway, not worth discussing. The fact is, the kernel does not accept do > while without braces. I told you why. You can ignore it. I'll ignore you. Oh, I'm not a martyr.
From: Hannes Eder <hannes@hanneseder.net> Date: Mon, 22 Dec 2008 20:15:07 +0100 > While at it insert some extra curly braces and fix formatting. ... > Signed-off-by: Hannes Eder <hannes@hanneseder.net> Applied. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 24, 2008 at 12:38 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > > On Wed, 24 Dec 2008, Krzysztof Halasa wrote: >> >> So is a case like >> do >> x; >> while (y); >> It can't be made more clear with brackets. > > Oh yes it can. People look at that, and it's so uncommon that they > literally believe it is a mis-indent. > > [snip] > >> kmalloc(sizeof i) just doesn't look good, the operator looks like >> a variable name. > > And I agree with you. "sizeof i" doesn't look good. It's uncommon, and > doesn't match peoples expectations. > >> But there is this return statement. Some people tend to write >> return (x); I simply write return x; > > I do to. And it's the common thing to do, and only totally confused people > think that 'return' is a somehow remotely like a "function" of its > arguments (the way 'sizeof' is - sizeof _is_ a function of its arguments, > albeit a very rare one). > >> It's clear, and so is a simple do-while. > > No. "return x;" is clear because it's the common thing, which means that > peopel are good at reading it. I got curious and conducted a little experiment, here is the outcome for the linux-2.6 kernel tree: hannes@vmbox:~/linux-2.6$ find . -name "*.[ch]" -print0 | xargs -0 cat | ../sparse/cstats - stats for '-': do's: 8092, non compound: 79 ( 1.0%) sizeof's: 51216, without parenthesis: 1543 ( 3.0%) return's: 286167, with parenthesis: 13552 ( 4.7%) 'cstats' is a little program I wrote using the sparse library, see below. The value for "return's with parenthesis" is a bit of an estimation, as 'cstats' operates only at the token level, so "return (x) ? y : z;" counts as "return with parenthesis". some sanity checks, to ensure the magnitudes are right: hannes@vmbox:~/linux-2.6$ git grep -w do -- *.[ch] | wc -l 20599 hannes@vmbox:~/linux-2.6$ git grep '\bdo[ \t]*{' -- *.[ch] | wc -l 7805 I assume 'do' is used frequently in comments: hannes@vmbox:~/linux-2.6$ git grep '\*.*\bdo\b' -- *.[ch] | wc -l 11358 hannes@vmbox:~/linux-2.6$ git grep '^[ \t]*do$' -- *.[ch] | wc -l 34 hannes@vmbox:~/linux-2.6$ git grep -w sizeof -- *.[ch] | wc -l 49631 constructs like sizeof(array)/sizeof(array[0]) or common: hannes@vmbox:~/linux-2.6$ git grep 'sizeof.*sizeof' -- *.[ch] | wc -l 1827 hannes@vmbox:~/linux-2.6$ git grep '\bsizeof [^(]' -- *.[ch] | wc -l 1534 hannes@vmbox:~/linux-2.6$ git grep -w return -- *.[ch] | wc -l 295304 hannes@vmbox:~/linux-2.6$ git grep '\breturn (' -- *.[ch] | wc -l 11067 Best, Hannes #include <stdlib.h> #include <stdio.h> #include <string.h> #include <unistd.h> #include <fcntl.h> #include "token.h" #include "parse.h" int main(int argc, char **argv) { struct string_list *filelist = NULL; char *filename; sparse_initialize(argc, argv, &filelist); FOR_EACH_PTR_NOTAG(filelist, filename) { int fd; struct token *token; int do_stats = 0; int do_stats_nc = 0; int sizeof_exprs = 0; int sizeof_exprs_np = 0; int return_stats = 0; int return_stats_wp = 0; if (strcmp(filename, "-") == 0) { fd = 0; } else { fd = open(filename, O_RDONLY); if (fd < 0) die("No such file: %s", filename); } // Tokenize the input stream token = tokenize(filename, fd, NULL, includepath); close(fd); for ( ; !eof_token(token); token = token->next) { if (token_type(token) == TOKEN_IDENT) { if (token->ident == &do_ident) { do_stats++; if (!match_op(token->next, '{')) do_stats_nc++; } else if (token->ident == &sizeof_ident) { sizeof_exprs++; if (!match_op(token->next, '(')) sizeof_exprs_np++; } else if (token->ident == &return_ident) { return_stats++; if (match_op(token->next, '(')) return_stats_wp++; } } } printf("stats for '%s':\n", filename); printf(" do's: %8d, non compound: %8d (%5.1f%%)\n", do_stats, do_stats_nc, 100.0 * do_stats_nc / (do_stats?:1) ); printf(" sizeof's: %8d, without parenthesis: %8d (%5.1f%%)\n", sizeof_exprs, sizeof_exprs_np, 100.0 * sizeof_exprs_np / (sizeof_exprs?:1) ); printf(" return's: %8d, with parenthesis: %8d (%5.1f%%)\n", return_stats, return_stats_wp, 100.0 * return_stats_wp / (return_stats?:1) ); } END_FOR_EACH_PTR_NOTAG(filename); return 0; } -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/atp.c b/drivers/net/atp.c index 1d6b74c..ea493ce 100644 --- a/drivers/net/atp.c +++ b/drivers/net/atp.c @@ -802,21 +802,22 @@ static void net_rx(struct net_device *dev) static void read_block(long ioaddr, int length, unsigned char *p, int data_mode) { - if (data_mode <= 3) { /* Mode 0 or 1 */ outb(Ctrl_LNibRead, ioaddr + PAR_CONTROL); outb(length == 8 ? RdAddr | HNib | MAR : RdAddr | MAR, ioaddr + PAR_DATA); if (data_mode <= 1) { /* Mode 0 or 1 */ - do *p++ = read_byte_mode0(ioaddr); while (--length > 0); - } else /* Mode 2 or 3 */ - do *p++ = read_byte_mode2(ioaddr); while (--length > 0); - } else if (data_mode <= 5) - do *p++ = read_byte_mode4(ioaddr); while (--length > 0); - else - do *p++ = read_byte_mode6(ioaddr); while (--length > 0); + do { *p++ = read_byte_mode0(ioaddr); } while (--length > 0); + } else { /* Mode 2 or 3 */ + do { *p++ = read_byte_mode2(ioaddr); } while (--length > 0); + } + } else if (data_mode <= 5) { + do { *p++ = read_byte_mode4(ioaddr); } while (--length > 0); + } else { + do { *p++ = read_byte_mode6(ioaddr); } while (--length > 0); + } - outb(EOC+HNib+MAR, ioaddr + PAR_DATA); + outb(EOC+HNib+MAR, ioaddr + PAR_DATA); outb(Ctrl_SelData, ioaddr + PAR_CONTROL); } diff --git a/drivers/net/plip.c b/drivers/net/plip.c index 5d904f7..ed8582e 100644 --- a/drivers/net/plip.c +++ b/drivers/net/plip.c @@ -638,14 +638,14 @@ plip_receive_packet(struct net_device *dev, struct net_local *nl, case PLIP_PK_DATA: lbuf = rcv->skb->data; - do + do { if (plip_receive(nibble_timeout, dev, &rcv->nibble, &lbuf[rcv->byte])) return TIMEOUT; - while (++rcv->byte < rcv->length.h); - do + } while (++rcv->byte < rcv->length.h); + do { rcv->checksum += lbuf[--rcv->byte]; - while (rcv->byte); + } while (rcv->byte); rcv->state = PLIP_PK_CHECKSUM; case PLIP_PK_CHECKSUM: @@ -816,14 +816,14 @@ plip_send_packet(struct net_device *dev, struct net_local *nl, snd->checksum = 0; case PLIP_PK_DATA: - do + do { if (plip_send(nibble_timeout, dev, &snd->nibble, lbuf[snd->byte])) return TIMEOUT; - while (++snd->byte < snd->length.h); - do + } while (++snd->byte < snd->length.h); + do { snd->checksum += lbuf[--snd->byte]; - while (snd->byte); + } while (snd->byte); snd->state = PLIP_PK_CHECKSUM; case PLIP_PK_CHECKSUM: diff --git a/drivers/net/starfire.c b/drivers/net/starfire.c index 01493a4..8bfc15b 100644 --- a/drivers/net/starfire.c +++ b/drivers/net/starfire.c @@ -882,9 +882,9 @@ static int mdio_read(struct net_device *dev, int phy_id, int location) void __iomem *mdio_addr = np->base + MIICtrl + (phy_id<<7) + (location<<2); int result, boguscnt=1000; /* ??? Should we add a busy-wait here? */ - do + do { result = readl(mdio_addr); - while ((result & 0xC0000000) != 0x80000000 && --boguscnt > 0); + } while ((result & 0xC0000000) != 0x80000000 && --boguscnt > 0); if (boguscnt == 0) return 0; if ((result & 0xffff) == 0xffff)
While at it insert some extra curly braces and fix formatting. Fix this sparse warnings: drivers/net/atp.c:811:8: warning: do-while statement is not a compound statement drivers/net/atp.c:813:8: warning: do-while statement is not a compound statement drivers/net/atp.c:815:11: warning: do-while statement is not a compound statement drivers/net/atp.c:817:11: warning: do-while statement is not a compound statement drivers/net/plip.c:642:4: warning: do-while statement is not a compound statement drivers/net/plip.c:647:4: warning: do-while statement is not a compound statement drivers/net/plip.c:820:4: warning: do-while statement is not a compound statement drivers/net/plip.c:825:4: warning: do-while statement is not a compound statement drivers/net/starfire.c:886:3: warning: do-while statement is not a compound statement Signed-off-by: Hannes Eder <hannes@hanneseder.net> --- drivers/net/atp.c | 19 ++++++++++--------- drivers/net/plip.c | 16 ++++++++-------- drivers/net/starfire.c | 4 ++-- 3 files changed, 20 insertions(+), 19 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html