Re: [PATCH -mm] IPS SCSI driver: Check return of scsi_add_host()

Re: [PATCH -mm] IPS SCSI driver: Check return of scsi_add_host()

[Jesper Juhl]

On Sunday 02 September 2007 22:13:49 Satyam Sharma wrote:
> 
> drivers/scsi/ips.c: In function ‘ips_register_scsi’:
> drivers/scsi/ips.c:6869:
> warning: ignoring return value of ‘scsi_add_host’, declared with attribute warn_unused_result
> 
> scsi_add_host() is __must_check, so let's check it's return and cleanup
> appropriately on errors.
> 
> Signed-off-by: Satyam Sharma <satyam@infradead.org>
> 
[snip]

Something seems to be wrong either at your end or mine
> +		IPS_PRINTK(KERN_WARNING, ha->pcidev, "Unable to add SCSI host¥n");

This should end with a newline  "\n"  but I'm seeing "¥n" ...


/Jesper

-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH -mm] IPS SCSI driver: Check return of scsi_add_host()

[Satyam Sharma]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 961 bytes --]


drivers/scsi/ips.c: In function ‘ips_register_scsi’:
drivers/scsi/ips.c:6869:
warning: ignoring return value of ‘scsi_add_host’, declared with attribute warn_unused_result

scsi_add_host() is __must_check, so let's check it's return and cleanup
appropriately on errors.

Signed-off-by: Satyam Sharma <satyam@infradead.org>

---

 drivers/scsi/ips.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

--- linux-2.6.23-rc4-mm1/drivers/scsi/ips.c~fix	2007-09-02 20:21:27.000000000 +0530
+++ linux-2.6.23-rc4-mm1/drivers/scsi/ips.c	2007-09-02 20:25:58.000000000 +0530
@@ -6866,7 +6866,12 @@ ips_register_scsi(int index)
 	sh->max_channel = ha->nbus - 1;
 	sh->can_queue = ha->max_cmds - 1;
 
-	scsi_add_host(sh, NULL);
+	if (scsi_add_host(sh, NULL)) {
+		IPS_PRINTK(KERN_WARNING, ha->pcidev, "Unable to add SCSI host\n");
+		free_irq(ha->irq, ha);
+		scsi_host_put(sh);
+		return -1;
+	}
 	scsi_scan_host(sh);
 
 	return 0;

[PATCH -mm] DC395x SCSI driver: Shut up uninitialized variable build warning

[Satyam Sharma]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 845 bytes --]


drivers/scsi/dc395x.c: In function ‘dc395x_init_one’:
drivers/scsi/dc395x.c:4272: warning: ‘ptr’ may be used uninitialized in this function

has been verified to be a bogus warning. Let's shut it up.

Signed-off-by: Satyam Sharma <satyam@infradead.org>

---

 drivers/scsi/dc395x.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- linux-2.6.23-rc4-mm1/drivers/scsi/dc395x.c~fix	2007-09-02 20:57:51.000000000 +0530
+++ linux-2.6.23-rc4-mm1/drivers/scsi/dc395x.c	2007-09-02 21:10:49.000000000 +0530
@@ -4269,7 +4269,7 @@ static int __devinit adapter_sg_tables_a
 	const unsigned srbs_per_page = PAGE_SIZE/SEGMENTX_LEN;
 	int srb_idx = 0;
 	unsigned i = 0;
-	struct SGentry *ptr;
+	struct SGentry * uninitialized_var(ptr);
 
 	for (i = 0; i < DC395x_MAX_SRB_CNT; i++)
 		acb->srb_array[i].segment_x = NULL;

Re: [PATCH -mm] DC395x SCSI driver: Shut up uninitialized variable build warning

[Jeff Garzik]

Satyam Sharma wrote:
> drivers/scsi/dc395x.c: In function ‘dc395x_init_one’:
> drivers/scsi/dc395x.c:4272: warning: ‘ptr’ may be used uninitialized in this function
> 
> has been verified to be a bogus warning. Let's shut it up.
> 
> Signed-off-by: Satyam Sharma <satyam@infradead.org>
> 
> ---
> 
>  drivers/scsi/dc395x.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- linux-2.6.23-rc4-mm1/drivers/scsi/dc395x.c~fix	2007-09-02 20:57:51.000000000 +0530
> +++ linux-2.6.23-rc4-mm1/drivers/scsi/dc395x.c	2007-09-02 21:10:49.000000000 +0530
> @@ -4269,7 +4269,7 @@ static int __devinit adapter_sg_tables_a
>  	const unsigned srbs_per_page = PAGE_SIZE/SEGMENTX_LEN;
>  	int srb_idx = 0;
>  	unsigned i = 0;
> -	struct SGentry *ptr;
> +	struct SGentry * uninitialized_var(ptr);

For things like this, we need to see (perhaps a separate email in the 
same thread) your build details:  config, arch, compiler version, etc.

This warning does not appear on x86 or x86-64 here, with the current 
version of gcc.  And I'm not so sure we want to be adding these markers 
for older versions of the compiler, or for what is ultimately a 
temporary [situation | configuration] in the grand scheme of things.

I paid careful attention to my recent set of uninitialized_var() markers 
(now upstream), making sure that they persisted across several compiler 
versions, ensuring the warning was not a temporary optimizer bug quickly 
remedied.

I hope to encourage similar diligence in others :)  These markers have 
the very-real potential downside of hiding bugs, so they should be used 
sparingly.

	Jeff


-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH -mm] IPS SCSI driver: Check return of scsi_add_host()

[Jeff Garzik]

Satyam Sharma wrote:
> drivers/scsi/ips.c: In function ‘ips_register_scsi’:
> drivers/scsi/ips.c:6869:
> warning: ignoring return value of ‘scsi_add_host’, declared with attribute warn_unused_result
> 
> scsi_add_host() is __must_check, so let's check it's return and cleanup
> appropriately on errors.
> 
> Signed-off-by: Satyam Sharma <satyam@infradead.org>
> 
> ---
> 
>  drivers/scsi/ips.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> --- linux-2.6.23-rc4-mm1/drivers/scsi/ips.c~fix	2007-09-02 20:21:27.000000000 +0530
> +++ linux-2.6.23-rc4-mm1/drivers/scsi/ips.c	2007-09-02 20:25:58.000000000 +0530
> @@ -6866,7 +6866,12 @@ ips_register_scsi(int index)
>  	sh->max_channel = ha->nbus - 1;
>  	sh->can_queue = ha->max_cmds - 1;
>  
> -	scsi_add_host(sh, NULL);
> +	if (scsi_add_host(sh, NULL)) {
> +		IPS_PRINTK(KERN_WARNING, ha->pcidev, "Unable to add SCSI host\n");
> +		free_irq(ha->irq, ha);
> +		scsi_host_put(sh);
> +		return -1;

ACK, as long as you add a comment to this code and the request_irq() 
failure code:  the oldha/old irq is not restored upon failure, in this 
function.

That is a pre-existing bug, and not your fault, but your patch 
"continues in the same buggy tradition" :)  We should at least note the 
FIXME at each error handling code branch.

Ideally you or somebody should do a detailed analysis to
a) (preferably) get rid of the silly oldha/ double-irq-request weirdness 
and make it look like other drivers,
	or,
b) analyze the code and see what it takes to _really_ unwind the error.

Also, I would recommend moving the error handling code to the end of the 
function and using the standard 'goto' approach for function error 
handling.  This eliminates the duplicate scsi_host_put() calls upon error.

	Jeff





-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH -mm] DC395x SCSI driver: Shut up uninitialized variable build warning

[Jeff Garzik]

Satyam Sharma wrote:
> I'll post the info as a reply to the first mail in this series. I have
> fairly recent gcc (4.1.1) and I don't see us dropping support for it in
> the next few years.

What's important is not support lifetime, but whether or not the warning 
persists through version 4.1.2, 4.1.3, etc.

We don't want to add markers for compiler quirks that come and go.

The current markers tend to exist for a class of problems that gcc 
fundamentally has a tough time "seeing."  Different optimizer behaviors 
from compiler version to compiler version are just noise[1].

	Jeff


[1] sometimes quite literally, in the case of compiler warnings.

Re: [PATCH -mm] DC395x SCSI driver: Shut up uninitialized variable build warning

[Satyam Sharma]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2076 bytes --]



On Sun, 2 Sep 2007, Jeff Garzik wrote:
> 
> Satyam Sharma wrote:
> > drivers/scsi/dc395x.c: In function ‘dc395x_init_one’:
> > drivers/scsi/dc395x.c:4272: warning: ‘ptr’ may be used uninitialized in this
> > function
> > 
> > has been verified to be a bogus warning. Let's shut it up.
> > 
> > Signed-off-by: Satyam Sharma <satyam@infradead.org>
> > 
> > ---
> > 
> >  drivers/scsi/dc395x.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > --- linux-2.6.23-rc4-mm1/drivers/scsi/dc395x.c~fix	2007-09-02
> > 20:57:51.000000000 +0530
> > +++ linux-2.6.23-rc4-mm1/drivers/scsi/dc395x.c	2007-09-02
> > 21:10:49.000000000 +0530
> > @@ -4269,7 +4269,7 @@ static int __devinit adapter_sg_tables_a
> >  	const unsigned srbs_per_page = PAGE_SIZE/SEGMENTX_LEN;
> >  	int srb_idx = 0;
> >  	unsigned i = 0;
> > -	struct SGentry *ptr;
> > +	struct SGentry * uninitialized_var(ptr);
> 
> For things like this, we need to see (perhaps a separate email in the same
> thread) your build details:  config, arch, compiler version, etc.

I'll post the info as a reply to the first mail in this series. I have
fairly recent gcc (4.1.1) and I don't see us dropping support for it in
the next few years.

> I paid careful attention to my recent set of uninitialized_var() markers (now
> upstream), making sure that they persisted across several compiler versions,
> ensuring the warning was not a temporary optimizer bug quickly remedied.
> 
> I hope to encourage similar diligence in others :)  These markers have the
> very-real potential downside of hiding bugs, so they should be used sparingly.

Well, uninitialized_var() is definitely more greppable than "= 0" which
code all over does all the time :-) Anyway, the point is that the
warnings are bogus anyway (hence maintainers Cc:'ed so that they can
confirm if I arrived at the conclusion correctly) /and/ that
uninitialized_var() stands out sorely, so I don't see bugs getting
"hidden" at all -- in fact I did find two bugs during this process, and
fixed them accordingly.

Thanks,

Satyam

Re: [PATCH -mm] IPS SCSI driver: Check return of scsi_add_host()

[Satyam Sharma]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1630 bytes --]



On Sun, 2 Sep 2007, Jeff Garzik wrote:

> Satyam Sharma wrote:
> > drivers/scsi/ips.c: In function ‘ips_register_scsi’:
> > drivers/scsi/ips.c:6869:
> > warning: ignoring return value of ‘scsi_add_host’, declared with attribute
> > warn_unused_result
> > 
> > scsi_add_host() is __must_check, so let's check it's return and cleanup
> > appropriately on errors.
> > 
> > Signed-off-by: Satyam Sharma <satyam@infradead.org>
> > 
> > ---
> > 
> >  drivers/scsi/ips.c |    7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > --- linux-2.6.23-rc4-mm1/drivers/scsi/ips.c~fix	2007-09-02
> > 20:21:27.000000000 +0530
> > +++ linux-2.6.23-rc4-mm1/drivers/scsi/ips.c	2007-09-02 20:25:58.000000000
> > +0530
> > @@ -6866,7 +6866,12 @@ ips_register_scsi(int index)
> >  	sh->max_channel = ha->nbus - 1;
> >  	sh->can_queue = ha->max_cmds - 1;
> >  -	scsi_add_host(sh, NULL);
> > +	if (scsi_add_host(sh, NULL)) {
> > +		IPS_PRINTK(KERN_WARNING, ha->pcidev, "Unable to add SCSI
> > host\n");
> > +		free_irq(ha->irq, ha);
> > +		scsi_host_put(sh);
> > +		return -1;
> 
> ACK, as long as you add a comment to this code and the request_irq() failure
> code:  the oldha/old irq is not restored upon failure, in this function.
> 
> That is a pre-existing bug, and not your fault, but your patch "continues in
> the same buggy tradition" :)  We should at least note the FIXME at each error
> handling code branch.

True, I did notice that (as well as the coding style issue you pointed
out) when making the patch, but felt lazy to actually set it right fully.
And that (when it's done) should be a separate patch anyway.