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 ...
-
: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
