[PATCH] IDE: Silent compiler warning in ide_pio_bytes()

[PATCH] IDE: Silent compiler warning in ide_pio_bytes()

[Jean Delvare]

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 */
 	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;


-- 
Jean Delvare

Re: [PATCH] IDE: Silent compiler warning in ide_pio_bytes()

[Borislav Petkov]

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;

[..]

-- 
Regards/Gruss,
    Boris.

Re: [PATCH] IDE: Silent compiler warning in ide_pio_bytes()

[David Miller]

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.

Re: [PATCH] IDE: Silent compiler warning in ide_pio_bytes()

[Jean Delvare]

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.

-- 
Jean Delvare

Re: [PATCH] IDE: Silent compiler warning in ide_pio_bytes()

[David Miller]

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 :-)

Re: [PATCH] IDE: Silent compiler warning in ide_pio_bytes()

[Jean Delvare]

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.

-- 
Jean Delvare

Re: [PATCH] IDE: Silent compiler warning in ide_pio_bytes()

[David Miller]

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" :-)