* [PATCH] scsi core: fix uninitialized variable error
@ 2006-02-02 21:44 Alan Stern
2006-02-05 15:01 ` Matthew Wilcox
0 siblings, 1 reply; 6+ messages in thread
From: Alan Stern @ 2006-02-02 21:44 UTC (permalink / raw)
To: James Bottomley; +Cc: SCSI development list
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);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi core: fix uninitialized variable error
2006-02-02 21:44 [PATCH] scsi core: fix uninitialized variable error Alan Stern
@ 2006-02-05 15:01 ` Matthew Wilcox
2006-02-05 16:11 ` Alan Stern
0 siblings, 1 reply; 6+ messages in thread
From: Matthew Wilcox @ 2006-02-05 15:01 UTC (permalink / raw)
To: Alan Stern; +Cc: James Bottomley, SCSI development list
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);
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi core: fix uninitialized variable error
2006-02-05 15:01 ` Matthew Wilcox
@ 2006-02-05 16:11 ` Alan Stern
2006-02-05 21:30 ` Matthew Wilcox
0 siblings, 1 reply; 6+ messages in thread
From: Alan Stern @ 2006-02-05 16:11 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: James Bottomley, SCSI development list
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi core: fix uninitialized variable error
2006-02-05 16:11 ` Alan Stern
@ 2006-02-05 21:30 ` Matthew Wilcox
2006-02-05 21:45 ` Alan Stern
2006-02-06 8:42 ` Markus Lidel
0 siblings, 2 replies; 6+ messages in thread
From: Matthew Wilcox @ 2006-02-05 21:30 UTC (permalink / raw)
To: Alan Stern; +Cc: James Bottomley, SCSI development list
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 ...
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi core: fix uninitialized variable error
2006-02-05 21:30 ` Matthew Wilcox
@ 2006-02-05 21:45 ` Alan Stern
2006-02-06 8:42 ` Markus Lidel
1 sibling, 0 replies; 6+ messages in thread
From: Alan Stern @ 2006-02-05 21:45 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: James Bottomley, SCSI development list
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi core: fix uninitialized variable error
2006-02-05 21:30 ` Matthew Wilcox
2006-02-05 21:45 ` Alan Stern
@ 2006-02-06 8:42 ` Markus Lidel
1 sibling, 0 replies; 6+ messages in thread
From: Markus Lidel @ 2006-02-06 8:42 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: Alan Stern, James Bottomley, SCSI development list
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2006-02-06 8:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-02-02 21:44 [PATCH] scsi core: fix uninitialized variable error Alan Stern
2006-02-05 15:01 ` Matthew Wilcox
2006-02-05 16:11 ` Alan Stern
2006-02-05 21:30 ` Matthew Wilcox
2006-02-05 21:45 ` Alan Stern
2006-02-06 8:42 ` Markus Lidel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).