[PATCH] scsi core: fix uninitialized variable error

[PATCH] scsi core: fix uninitialized variable error

[Alan Stern]

James:

This patch (as649) fixes an uninitialized variable error (sdev) in 
__scsi_add_device.  I don't understand why the compiler didn't flag the 
error.

It also removes a pair of calls to get_device and put_device; they are no 
longer needed since scsi_target_reap now uses a work_queue.

Alan Stern



Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

Index: usb-2.6/drivers/scsi/scsi_scan.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_scan.c
+++ usb-2.6/drivers/scsi/scsi_scan.c
@@ -368,11 +368,7 @@ static struct scsi_target *scsi_alloc_ta
 
 		if(error) {
 			dev_printk(KERN_ERR, dev, "target allocation failed, error %d\n", error);
-			/* don't want scsi_target_reap to do the final
-			 * put because it will be under the host lock */
-			get_device(dev);
 			scsi_target_reap(starget);
-			put_device(dev);
 			return NULL;
 		}
 	}
@@ -1265,7 +1261,6 @@ struct scsi_device *__scsi_add_device(st
 {
 	struct scsi_device *sdev;
 	struct device *parent = &shost->shost_gendev;
-	int res;
 	struct scsi_target *starget;
 
 	starget = scsi_alloc_target(parent, channel, id);
@@ -1274,12 +1269,10 @@ struct scsi_device *__scsi_add_device(st
 
 	get_device(&starget->dev);
 	mutex_lock(&shost->scan_mutex);
-	if (scsi_host_scan_allowed(shost)) {
-		res = scsi_probe_and_add_lun(starget, lun, NULL, &sdev, 1,
-					     hostdata);
-		if (res != SCSI_SCAN_LUN_PRESENT)
-			sdev = ERR_PTR(-ENODEV);
-	}
+	if (!scsi_host_scan_allowed(shost) ||
+			scsi_probe_and_add_lun(starget, lun, NULL, &sdev, 1,
+				hostdata) != SCSI_SCAN_LUN_PRESENT)
+		sdev = ERR_PTR(-ENODEV);
 	mutex_unlock(&shost->scan_mutex);
 	scsi_target_reap(starget);
 	put_device(&starget->dev);

Re: [PATCH] scsi core: fix uninitialized variable error

[Matthew Wilcox]

On Thu, Feb 02, 2006 at 04:44:20PM -0500, Alan Stern wrote:
> This patch (as649) fixes an uninitialized variable error (sdev) in 
> __scsi_add_device.  I don't understand why the compiler didn't flag the 
> error.

> @@ -1265,7 +1261,6 @@ struct scsi_device *__scsi_add_device(st
>  {
>  	struct scsi_device *sdev;
>  	struct device *parent = &shost->shost_gendev;
> -	int res;
>  	struct scsi_target *starget;
>  
>  	starget = scsi_alloc_target(parent, channel, id);
> @@ -1274,12 +1269,10 @@ struct scsi_device *__scsi_add_device(st
>  
>  	get_device(&starget->dev);
>  	mutex_lock(&shost->scan_mutex);
> -	if (scsi_host_scan_allowed(shost)) {
> -		res = scsi_probe_and_add_lun(starget, lun, NULL, &sdev, 1,
> -					     hostdata);
> -		if (res != SCSI_SCAN_LUN_PRESENT)
> -			sdev = ERR_PTR(-ENODEV);
> -	}
> +	if (!scsi_host_scan_allowed(shost) ||
> +			scsi_probe_and_add_lun(starget, lun, NULL, &sdev, 1,
> +				hostdata) != SCSI_SCAN_LUN_PRESENT)
> +		sdev = ERR_PTR(-ENODEV);
>  	mutex_unlock(&shost->scan_mutex);
>  	scsi_target_reap(starget);
>  	put_device(&starget->dev);

Seems to me it'd be much easier just  to initialise sdev to
ERR_PTR(-ENODEV) at the top of the function.  That way, __scsi_add_device
doesn't need to know what the return codes from scsi_probe_and_add_lun
mean.  Something like this (untested):

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 752fb5d..4c7bd89 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1267,9 +1267,8 @@ static int scsi_report_lun_scan(struct s
 struct scsi_device *__scsi_add_device(struct Scsi_Host *shost, uint channel,
 				      uint id, uint lun, void *hostdata)
 {
-	struct scsi_device *sdev;
+	struct scsi_device *sdev = ERR_PTR(-ENODEV);
 	struct device *parent = &shost->shost_gendev;
-	int res;
 	struct scsi_target *starget;
 
 	starget = scsi_alloc_target(parent, channel, id);
@@ -1278,12 +1277,8 @@ struct scsi_device *__scsi_add_device(st
 
 	get_device(&starget->dev);
 	mutex_lock(&shost->scan_mutex);
-	if (scsi_host_scan_allowed(shost)) {
-		res = scsi_probe_and_add_lun(starget, lun, NULL, &sdev, 1,
-					     hostdata);
-		if (res != SCSI_SCAN_LUN_PRESENT)
-			sdev = ERR_PTR(-ENODEV);
-	}
+	if (scsi_host_scan_allowed(shost))
+		scsi_probe_and_add_lun(starget, lun, NULL, &sdev, 1, hostdata);
 	mutex_unlock(&shost->scan_mutex);
 	scsi_target_reap(starget);
 	put_device(&starget->dev);

Re: [PATCH] scsi core: fix uninitialized variable error

[Alan Stern]

On Sun, 5 Feb 2006, Matthew Wilcox wrote:

> Seems to me it'd be much easier just  to initialise sdev to
> ERR_PTR(-ENODEV) at the top of the function.  That way, __scsi_add_device
> doesn't need to know what the return codes from scsi_probe_and_add_lun
> mean.  Something like this (untested):

> @@ -1278,12 +1277,8 @@ struct scsi_device *__scsi_add_device(st
>  
>  	get_device(&starget->dev);
>  	mutex_lock(&shost->scan_mutex);
> -	if (scsi_host_scan_allowed(shost)) {
> -		res = scsi_probe_and_add_lun(starget, lun, NULL, &sdev, 1,
> -					     hostdata);
> -		if (res != SCSI_SCAN_LUN_PRESENT)
> -			sdev = ERR_PTR(-ENODEV);
> -	}
> +	if (scsi_host_scan_allowed(shost))
> +		scsi_probe_and_add_lun(starget, lun, NULL, &sdev, 1, hostdata);
>  	mutex_unlock(&shost->scan_mutex);
>  	scsi_target_reap(starget);
>  	put_device(&starget->dev);

This assumes that scsi_probe_and_add_lun doesn't assign anything to the 
&sdev pointer if it returns anything other than SCSI_SCAN_LUN_PRESENT.  
Since that assumption is true for the current code, this version of the 
patch will work as well as mine.

Alan Stern

Re: [PATCH] scsi core: fix uninitialized variable error

[Matthew Wilcox]

On Sun, Feb 05, 2006 at 11:11:24AM -0500, Alan Stern wrote:
> > -	if (scsi_host_scan_allowed(shost)) {
> > -		res = scsi_probe_and_add_lun(starget, lun, NULL, &sdev, 1,
> > -					     hostdata);
> > -		if (res != SCSI_SCAN_LUN_PRESENT)
> > -			sdev = ERR_PTR(-ENODEV);
> > -	}
> > +	if (scsi_host_scan_allowed(shost))
> > +		scsi_probe_and_add_lun(starget, lun, NULL, &sdev, 1, hostdata);
> >  	mutex_unlock(&shost->scan_mutex);
> 
> This assumes that scsi_probe_and_add_lun doesn't assign anything to the 
> &sdev pointer if it returns anything other than SCSI_SCAN_LUN_PRESENT.  
> Since that assumption is true for the current code, this version of the 
> patch will work as well as mine.

Perhaps the better way to think about this usage of
scsi_probe_and_add_lun() is "If it finds an sdev, then we should return
it".  Right now, we're assuming that returning SCSI_SCAN_LUN_PRESENT is
equivalent to having found an sdev.

The real problem is that scsi_probe_and_add_lun() has an enormously
complicated interface.  The good news is that it's static, so we can see
all its callers.  The bad news is that the kernel-doc comment is out of
date and not terribly helpful.

Its callers are:

scsi_sequential_lun_scan()
	scsi_probe_and_add_lun(starget, lun, NULL, NULL, rescan, NULL)
		(!= SCSI_SCAN_LUN_PRESENT)
scsi_report_lun_scan()
	scsi_probe_and_add_lun(starget, lun, NULL, NULL, rescan, NULL)
		(== SCSI_SCAN_NO_RESPONSE)
__scsi_add_device()
	scsi_probe_and_add_lun(starget, lun, NULL, &sdev, 1, hostdata)
		(== SCSI_SCAN_LUN_PRESENT in current, unused in my patch)
__scsi_scan_target()
	scsi_probe_and_add_lun(starget, lun, NULL, NULL, rescan, NULL)
		(unused)
	scsi_probe_and_add_lun(starget, 0, &bflags, NULL, rescan, NULL)
		(== SCSI_SCAN_LUN_PRESENT or SCSI_SCAN_TARGET_PRESENT)

I can see some ways to simplify this interface.  As noted in a comment:

                 * XXX add a bflags to scsi_device, and replace the
                 * corresponding bit fields in scsi_device, so bflags
                 * need not be passed as an argument.

I *think* we can get rid of the hostdata parameter.  The only non-NULL
caller is __scsi_add_device().  The only caller of __scsi_add_device()
which specifies hostdata is i2o_scsi.  I don't see why it can't use a
->slave_alloc in the host template to set hostdata rather than passing
it in.

That'd get us down from a 6-argument function to a 4-argument one.  We
still have the problem of wanting to return multiple things (a SCSI_SCAN
constant in most cases and an sdev in another).  Maybe we could do something
like ERR_PTR/IS_ERR ...

Re: [PATCH] scsi core: fix uninitialized variable error

[Alan Stern]

On Sun, 5 Feb 2006, Matthew Wilcox wrote:

> Perhaps the better way to think about this usage of
> scsi_probe_and_add_lun() is "If it finds an sdev, then we should return
> it".  Right now, we're assuming that returning SCSI_SCAN_LUN_PRESENT is
> equivalent to having found an sdev.
> 
> The real problem is that scsi_probe_and_add_lun() has an enormously
> complicated interface.  The good news is that it's static, so we can see
> all its callers.  The bad news is that the kernel-doc comment is out of
> date and not terribly helpful.
> 
> Its callers are:
> 
> scsi_sequential_lun_scan()
> 	scsi_probe_and_add_lun(starget, lun, NULL, NULL, rescan, NULL)
> 		(!= SCSI_SCAN_LUN_PRESENT)
> scsi_report_lun_scan()
> 	scsi_probe_and_add_lun(starget, lun, NULL, NULL, rescan, NULL)
> 		(== SCSI_SCAN_NO_RESPONSE)
> __scsi_add_device()
> 	scsi_probe_and_add_lun(starget, lun, NULL, &sdev, 1, hostdata)
> 		(== SCSI_SCAN_LUN_PRESENT in current, unused in my patch)
> __scsi_scan_target()
> 	scsi_probe_and_add_lun(starget, lun, NULL, NULL, rescan, NULL)
> 		(unused)
> 	scsi_probe_and_add_lun(starget, 0, &bflags, NULL, rescan, NULL)
> 		(== SCSI_SCAN_LUN_PRESENT or SCSI_SCAN_TARGET_PRESENT)
> 
> I can see some ways to simplify this interface.  As noted in a comment:
> 
>                  * XXX add a bflags to scsi_device, and replace the
>                  * corresponding bit fields in scsi_device, so bflags
>                  * need not be passed as an argument.
> 
> I *think* we can get rid of the hostdata parameter.  The only non-NULL
> caller is __scsi_add_device().  The only caller of __scsi_add_device()
> which specifies hostdata is i2o_scsi.  I don't see why it can't use a
> ->slave_alloc in the host template to set hostdata rather than passing
> it in.
> 
> That'd get us down from a 6-argument function to a 4-argument one.  We
> still have the problem of wanting to return multiple things (a SCSI_SCAN
> constant in most cases and an sdev in another).  Maybe we could do something
> like ERR_PTR/IS_ERR ...

That sounds good to me.  It looks like the possible responses fall into 
three classes: LUN present (and return the sdev pointer), LUN not present 
but target present (needs an ERR_PTR value), and no response (simply 
return NULL).  If you want to write those changes, I'll endorse it.

The whole bflags thing is badly in need of updating.  There really should
be a bflags field in the sdev structure, so that the flags can be tailored
for individual devices instead of using a common blacklist entry.  There
already _is_ a similar field in the target structure, although it's not
used for much.  And there already are a number of bitfields in the sdev
structure to hold information which should exist in a single bflags field
-- in effect, people have already started moving the flags into the sdev
but have done it piecemeal.  Of course, this is a separate issue from
scsi_probe_and_add_lun.

Alan Stern

Re: [PATCH] scsi core: fix uninitialized variable error

[Markus Lidel]

Hello,

Matthew Wilcox wrote:
> On Sun, Feb 05, 2006 at 11:11:24AM -0500, Alan Stern wrote:
>>> -	if (scsi_host_scan_allowed(shost)) {
>>> -		res = scsi_probe_and_add_lun(starget, lun, NULL, &sdev, 1,
>>> -					     hostdata);
>>> -		if (res != SCSI_SCAN_LUN_PRESENT)
>>> -			sdev = ERR_PTR(-ENODEV);
>>> -	}
>>> +	if (scsi_host_scan_allowed(shost))
>>> +		scsi_probe_and_add_lun(starget, lun, NULL, &sdev, 1, hostdata);
>>>  	mutex_unlock(&shost->scan_mutex);
>> This assumes that scsi_probe_and_add_lun doesn't assign anything to the 
>> &sdev pointer if it returns anything other than SCSI_SCAN_LUN_PRESENT.  
>> Since that assumption is true for the current code, this version of the 
>> patch will work as well as mine.
> 
> Perhaps the better way to think about this usage of
> scsi_probe_and_add_lun() is "If it finds an sdev, then we should return
> it".  Right now, we're assuming that returning SCSI_SCAN_LUN_PRESENT is
> equivalent to having found an sdev.
> 
> The real problem is that scsi_probe_and_add_lun() has an enormously
> complicated interface.  The good news is that it's static, so we can see
> all its callers.  The bad news is that the kernel-doc comment is out of
> date and not terribly helpful.
> 
> Its callers are:
> 
> scsi_sequential_lun_scan()
> 	scsi_probe_and_add_lun(starget, lun, NULL, NULL, rescan, NULL)
> 		(!= SCSI_SCAN_LUN_PRESENT)
> scsi_report_lun_scan()
> 	scsi_probe_and_add_lun(starget, lun, NULL, NULL, rescan, NULL)
> 		(== SCSI_SCAN_NO_RESPONSE)
> __scsi_add_device()
> 	scsi_probe_and_add_lun(starget, lun, NULL, &sdev, 1, hostdata)
> 		(== SCSI_SCAN_LUN_PRESENT in current, unused in my patch)
> __scsi_scan_target()
> 	scsi_probe_and_add_lun(starget, lun, NULL, NULL, rescan, NULL)
> 		(unused)
> 	scsi_probe_and_add_lun(starget, 0, &bflags, NULL, rescan, NULL)
> 		(== SCSI_SCAN_LUN_PRESENT or SCSI_SCAN_TARGET_PRESENT)
> 
> I can see some ways to simplify this interface.  As noted in a comment:
> 
>                  * XXX add a bflags to scsi_device, and replace the
>                  * corresponding bit fields in scsi_device, so bflags
>                  * need not be passed as an argument.
> 
> I *think* we can get rid of the hostdata parameter.  The only non-NULL
> caller is __scsi_add_device().  The only caller of __scsi_add_device()
> which specifies hostdata is i2o_scsi.  I don't see why it can't use a
> ->slave_alloc in the host template to set hostdata rather than passing
> it in.

Please don't remove the hostdata parameter. It's the only way to get the 
mapping between I2O device and SCSI devices.


Thank you very much.



Best regards,


Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)

Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany

Phone:  +49 82 82/99 51-0
Fax:    +49 82 82/99 51-11

E-Mail: Markus.Lidel@shadowconnect.com
URL:    http://www.shadowconnect.com