| Message ID | 20090622211052.45b3cfc5@hyperion.delvare |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
On Mon, Jun 22, 2009 at 09:10:52PM +0200, Jean Delvare wrote: > PageHighMem() isn't cheap so avoid calling it several times on the > same page. I had the hope that this would silent the following > compilation warning: > > drivers/ide/ide-taskfile.c: In function 'ide_pio_bytes': > drivers/ide/ide-taskfile.c:229: warning: 'flags' may be used uninitialized in this function > > which is a false positive, but it did not. So let's just initialize the > flags and be done with it, so that other developers don't waste their > time looking at it. > > Signed-off-by: Jean Delvare <khali@linux-fr.org> > Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > --- > drivers/ide/ide-taskfile.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > --- linux-2.6.31-pre.orig/drivers/ide/ide-taskfile.c 2009-06-21 09:37:02.000000000 +0200 > +++ linux-2.6.31-pre/drivers/ide/ide-taskfile.c 2009-06-21 12:18:47.000000000 +0200 > @@ -226,7 +226,7 @@ void ide_pio_bytes(ide_drive_t *drive, s > struct scatterlist *sg = hwif->sg_table; > struct scatterlist *cursg = cmd->cursg; > struct page *page; > - unsigned long flags; > + unsigned long flags = 0; /* Silent compiler warning */ or maybe unsigned long uninitialized_var(flags); > unsigned int offset; > u8 *buf; [..]
From: Jean Delvare <khali@linux-fr.org> Date: Mon, 22 Jun 2009 21:10:52 +0200 > PageHighMem() isn't cheap so avoid calling it several times on the > same page. I had the hope that this would silent the following > compilation warning: > > drivers/ide/ide-taskfile.c: In function 'ide_pio_bytes': > drivers/ide/ide-taskfile.c:229: warning: 'flags' may be used uninitialized in this function > > which is a false positive, but it did not. So let's just initialize the > flags and be done with it, so that other developers don't waste their > time looking at it. > > Signed-off-by: Jean Delvare <khali@linux-fr.org> > Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> Like Borislav, I think it's better to use uninitialized_var(). It describes the situation completely. Please submit an updated patch, and thank you for this. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Dave, Borislav, On Mon, 22 Jun 2009 16:10:59 -0700 (PDT), David Miller wrote: > From: Jean Delvare <khali@linux-fr.org> > Date: Mon, 22 Jun 2009 21:10:52 +0200 > > > PageHighMem() isn't cheap so avoid calling it several times on the > > same page. I had the hope that this would silent the following > > compilation warning: > > > > drivers/ide/ide-taskfile.c: In function 'ide_pio_bytes': > > drivers/ide/ide-taskfile.c:229: warning: 'flags' may be used uninitialized in this function > > > > which is a false positive, but it did not. So let's just initialize the > > flags and be done with it, so that other developers don't waste their > > time looking at it. > > > > Signed-off-by: Jean Delvare <khali@linux-fr.org> > > Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > > Like Borislav, I think it's better to use uninitialized_var(). > It describes the situation completely. > > Please submit an updated patch, and thank you for this. I fear somebody else will have to do that. I personally think uninitialized_var() should not have been invented, I don't want to have my name associated with any of its uses for it will inevitably lead to bugs in the future. I'll resubmit a patch not fixing the warning (because the rest is still useful I think) but that's about all I can offer.
From: Jean Delvare <khali@linux-fr.org> Date: Tue, 23 Jun 2009 08:53:24 +0200 > I fear somebody else will have to do that. I personally think > uninitialized_var() should not have been invented, I don't want to have > my name associated with any of its uses for it will inevitably lead to > bugs in the future. > > I'll resubmit a patch not fixing the warning (because the rest is still > useful I think) but that's about all I can offer. The alternative will be that someone will ask "when does the '0' case get used" and have to sort through that and potentially ask people here on the lists. But you posted and update patch which solves the problem in an even nicer way :-) -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 23 Jun 2009 02:32:11 -0700 (PDT), David Miller wrote: > From: Jean Delvare <khali@linux-fr.org> > Date: Tue, 23 Jun 2009 08:53:24 +0200 > > > I fear somebody else will have to do that. I personally think > > uninitialized_var() should not have been invented, I don't want to have > > my name associated with any of its uses for it will inevitably lead to > > bugs in the future. > > > > I'll resubmit a patch not fixing the warning (because the rest is still > > useful I think) but that's about all I can offer. > > The alternative will be that someone will ask "when does the '0' case > get used" and have to sort through that and potentially ask people > here on the lists. The comment I added, /* Silent compiler warning */, should have answered this question pretty clearly, methinks. > But you posted and update patch which solves the problem in an > even nicer way :-) Is it enough to silent the warning for you? I wasn't for me (gcc 4.3.2), although I expected it to... gcc should be able to see there is no code path leading to the variable being used uninitialized.
From: Jean Delvare <khali@linux-fr.org> Date: Tue, 23 Jun 2009 11:52:07 +0200 > On Tue, 23 Jun 2009 02:32:11 -0700 (PDT), David Miller wrote: >> From: Jean Delvare <khali@linux-fr.org> >> Date: Tue, 23 Jun 2009 08:53:24 +0200 >> >> > I fear somebody else will have to do that. I personally think >> > uninitialized_var() should not have been invented, I don't want to have >> > my name associated with any of its uses for it will inevitably lead to >> > bugs in the future. >> > >> > I'll resubmit a patch not fixing the warning (because the rest is still >> > useful I think) but that's about all I can offer. >> >> The alternative will be that someone will ask "when does the '0' case >> get used" and have to sort through that and potentially ask people >> here on the lists. > > The comment I added, /* Silent compiler warning */, should have > answered this question pretty clearly, methinks. Yet the uninitialzed_var() tag was created to indicate this tree-wide. A convention for the entire tree. You can try to fight city hall with your protest, but I suspect biting off one's nose to spite one's face is not profitable in the end. >> But you posted and update patch which solves the problem in an >> even nicer way :-) > > Is it enough to silent the warning for you? I wasn't for me (gcc > 4.3.2), although I expected it to... gcc should be able to see there is > no code path leading to the variable being used uninitialized. I didn't check, I suspected that you rewrote the patch this way because it did kill the warning for you. Guess not. I'll be the pragmatist and add the uninitialized_var() myself, and will not for posterity your continued support of "the anti- uninitialized_var() cause" :-) -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- linux-2.6.31-pre.orig/drivers/ide/ide-taskfile.c 2009-06-21 09:37:02.000000000 +0200 +++ linux-2.6.31-pre/drivers/ide/ide-taskfile.c 2009-06-21 12:18:47.000000000 +0200 @@ -226,7 +226,7 @@ void ide_pio_bytes(ide_drive_t *drive, s struct scatterlist *sg = hwif->sg_table; struct scatterlist *cursg = cmd->cursg; struct page *page; - unsigned long flags; + unsigned long flags = 0; /* Silent compiler warning */ unsigned int offset; u8 *buf; @@ -236,6 +236,7 @@ void ide_pio_bytes(ide_drive_t *drive, s while (len) { unsigned nr_bytes = min(len, cursg->length - cmd->cursg_ofs); + int page_is_high; if (nr_bytes > PAGE_SIZE) nr_bytes = PAGE_SIZE; @@ -247,7 +248,8 @@ void ide_pio_bytes(ide_drive_t *drive, s page = nth_page(page, (offset >> PAGE_SHIFT)); offset %= PAGE_SIZE; - if (PageHighMem(page)) + page_is_high = PageHighMem(page); + if (page_is_high) local_irq_save(flags); buf = kmap_atomic(page, KM_BIO_SRC_IRQ) + offset; @@ -268,7 +270,7 @@ void ide_pio_bytes(ide_drive_t *drive, s kunmap_atomic(buf, KM_BIO_SRC_IRQ); - if (PageHighMem(page)) + if (page_is_high) local_irq_restore(flags); len -= nr_bytes;
PageHighMem() isn't cheap so avoid calling it several times on the same page. I had the hope that this would silent the following compilation warning: drivers/ide/ide-taskfile.c: In function 'ide_pio_bytes': drivers/ide/ide-taskfile.c:229: warning: 'flags' may be used uninitialized in this function which is a false positive, but it did not. So let's just initialize the flags and be done with it, so that other developers don't waste their time looking at it. Signed-off-by: Jean Delvare <khali@linux-fr.org> Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> --- drivers/ide/ide-taskfile.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)