[PATCH] Restart scsi_device_lookup_by_target

[PATCH] Restart scsi_device_lookup_by_target

[Hannes Reinecke]

[-- Attachment #1: Type: text/plain, Size: 1344 bytes --]

Hi all,

currently we have this:

struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *starget,
                                                 uint lun)
{
        struct scsi_device *sdev;
        struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
        unsigned long flags;

        spin_lock_irqsave(shost->host_lock, flags);
        sdev = __scsi_device_lookup_by_target(starget, lun);
        if (sdev && scsi_device_get(sdev))
                sdev = NULL;
        spin_unlock_irqrestore(shost->host_lock, flags);

        return sdev;
}

now consider an sdev list with two entries for LUN 0, the first
being in SDEV_DEL and the second being in SDEV_RUNNING.

scsi_device_lookup_by_target will always return NULL here, as
it will never be able to skip past the first (unuseable)
entry. So scsi_report_lun_scan will happily create duplicate
sdevs for LUN 0 and we'll be getting the infamous

sysfs: duplicate filename 'xxxx' can not be created

errors.

What we should be doing here is to restart
__scsi_device_lookup_by_target() if we find an
sdev but cannot use it (ie is in SDEV_DEL).

James, please apply.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

[-- Attachment #2: scsi-restart-lookup-by-target --]
[-- Type: text/plain, Size: 3627 bytes --]

Restart scsi_device_lookup_by_target()

When scsi_device_lookup_by_target() will always return
the first sdev with a matching LUN, regardless of
the state. However, when this sdev is in SDEV_DEL
it's quite possible to have additional sdevs in the
list with the same LUN which are active.
So we should restart __scsi_device_lookup_by_target()
whenever we find an sdev with state SDEV_DEL.

Signed-off-by: Hannes Reinecke <hare@suse.de>

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index 62a4618..ac0488b 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -1158,7 +1158,7 @@ static int esp_reconnect(struct esp *esp)
 			   ESP_BUSID);
 
 	tp = &esp->target[target];
-	dev = __scsi_device_lookup_by_target(tp->starget, lun);
+	dev = __scsi_device_lookup_by_target(tp->starget, NULL, lun);
 	if (!dev) {
 		printk(KERN_ERR PFX "esp%d: Reconnect, no lp "
 		       "tgt[%u] lun[%u]\n",
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index f8b79d4..b2c3c61 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -1094,23 +1094,29 @@ EXPORT_SYMBOL(__starget_for_each_device);
 /**
  * __scsi_device_lookup_by_target - find a device given the target (UNLOCKED)
  * @starget:	SCSI target pointer
+ * @sdev:	SCSI device pointer
  * @lun:	SCSI Logical Unit Number
  *
  * Description: Looks up the scsi_device with the specified @lun for a given
  * @starget.  The returned scsi_device does not have an additional
  * reference.  You must hold the host's host_lock over this call and
- * any access to the returned scsi_device.
+ * any access to the returned scsi_device. If @sdev is not NULL use the
+ * specified @sdev as a starting point after which to start the lookup.
  *
  * Note:  The only reason why drivers should use this is because
  * they need to access the device list in irq context.  Otherwise you
  * really want to use scsi_device_lookup_by_target instead.
  **/
 struct scsi_device *__scsi_device_lookup_by_target(struct scsi_target *starget,
+						   struct scsi_device *sdev,
 						   uint lun)
 {
-	struct scsi_device *sdev;
+	if (!sdev)
+		sdev = list_prepare_entry(sdev, &starget->devices,
+					  same_target_siblings);
 
-	list_for_each_entry(sdev, &starget->devices, same_target_siblings) {
+	list_for_each_entry_continue(sdev, &starget->devices,
+				     same_target_siblings) {
 		if (sdev->lun ==lun)
 			return sdev;
 	}
@@ -1131,14 +1137,15 @@ EXPORT_SYMBOL(__scsi_device_lookup_by_target);
 struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *starget,
 						 uint lun)
 {
-	struct scsi_device *sdev;
+	struct scsi_device *sdev = NULL;
 	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
 	unsigned long flags;
 
 	spin_lock_irqsave(shost->host_lock, flags);
-	sdev = __scsi_device_lookup_by_target(starget, lun);
+ restart:
+	sdev = __scsi_device_lookup_by_target(starget, sdev, lun);
 	if (sdev && scsi_device_get(sdev))
-		sdev = NULL;
+		goto restart;
 	spin_unlock_irqrestore(shost->host_lock, flags);
 
 	return sdev;
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index bb80937..710f150 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -285,6 +285,7 @@ extern struct scsi_device *__scsi_device_lookup(struct Scsi_Host *,
 extern struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *,
 							uint);
 extern struct scsi_device *__scsi_device_lookup_by_target(struct scsi_target *,
+							  struct scsi_device *,
 							  uint);
 extern void starget_for_each_device(struct scsi_target *, void *,
 		     void (*fn)(struct scsi_device *, void *));

Re: [PATCH] Restart scsi_device_lookup_by_target

[Matthew Wilcox]

On Tue, Jan 13, 2009 at 02:50:30PM +0100, Hannes Reinecke wrote:
> @@ -1131,14 +1137,15 @@ EXPORT_SYMBOL(__scsi_device_lookup_by_target);
>  struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *starget,
>  						 uint lun)
>  {
> -	struct scsi_device *sdev;
> +	struct scsi_device *sdev = NULL;
>  	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(shost->host_lock, flags);
> -	sdev = __scsi_device_lookup_by_target(starget, lun);
> + restart:
> +	sdev = __scsi_device_lookup_by_target(starget, sdev, lun);
>  	if (sdev && scsi_device_get(sdev))
> -		sdev = NULL;
> +		goto restart;
>  	spin_unlock_irqrestore(shost->host_lock, flags);
>  
>  	return sdev;

Backwards jumps are generally disapproved of.  How about:

	spin_lock_irqsave(shost->host_lock, flags);
	for (;;) {
		sdev = __scsi_device_lookup_by_target(starget, sdev, lun);
		if (!sdev || !scsi_device_get(sdev))
			break;
	}
	spin_unlock_irqrestore(shost->host_lock, flags);


-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] Restart scsi_device_lookup_by_target

[Hannes Reinecke]

Hi Matthew,

Matthew Wilcox wrote:
> On Tue, Jan 13, 2009 at 02:50:30PM +0100, Hannes Reinecke wrote:
>> @@ -1131,14 +1137,15 @@ EXPORT_SYMBOL(__scsi_device_lookup_by_target);
>>  struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *starget,
>>  						 uint lun)
>>  {
>> -	struct scsi_device *sdev;
>> +	struct scsi_device *sdev = NULL;
>>  	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
>>  	unsigned long flags;
>>  
>>  	spin_lock_irqsave(shost->host_lock, flags);
>> -	sdev = __scsi_device_lookup_by_target(starget, lun);
>> + restart:
>> +	sdev = __scsi_device_lookup_by_target(starget, sdev, lun);
>>  	if (sdev && scsi_device_get(sdev))
>> -		sdev = NULL;
>> +		goto restart;
>>  	spin_unlock_irqrestore(shost->host_lock, flags);
>>  
>>  	return sdev;
> 
> Backwards jumps are generally disapproved of.  How about:
> 
> 	spin_lock_irqsave(shost->host_lock, flags);
> 	for (;;) {
> 		sdev = __scsi_device_lookup_by_target(starget, sdev, lun);
> 		if (!sdev || !scsi_device_get(sdev))
> 			break;
> 	}
> 	spin_unlock_irqrestore(shost->host_lock, flags);
> 
> 
I must say I don't really like the for(;;) construct.
And it's really confusing as we want to find an sdev, so breaking
if it's _not_ found is ... weird.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)
--
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] Restart scsi_device_lookup_by_target

[Hannes Reinecke]

Hi Matthew,

Hannes Reinecke wrote:
> Matthew Wilcox wrote:
>> On Tue, Jan 13, 2009 at 02:50:30PM +0100, Hannes Reinecke wrote:
>>> @@ -1131,14 +1137,15 @@ EXPORT_SYMBOL(__scsi_device_lookup_by_target);
>>>  struct scsi_device *scsi_device_lookup_by_target(struct scsi_target 
>>> *starget,
>>>                           uint lun)
>>>  {
>>> -    struct scsi_device *sdev;
>>> +    struct scsi_device *sdev = NULL;
>>>      struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
>>>      unsigned long flags;
>>>  
>>>      spin_lock_irqsave(shost->host_lock, flags);
>>> -    sdev = __scsi_device_lookup_by_target(starget, lun);
>>> + restart:
>>> +    sdev = __scsi_device_lookup_by_target(starget, sdev, lun);
>>>      if (sdev && scsi_device_get(sdev))
>>> -        sdev = NULL;
>>> +        goto restart;
>>>      spin_unlock_irqrestore(shost->host_lock, flags);
>>>  
>>>      return sdev;
>>
>> Backwards jumps are generally disapproved of.  How about:
>>
>>     spin_lock_irqsave(shost->host_lock, flags);
>>     for (;;) {
>>         sdev = __scsi_device_lookup_by_target(starget, sdev, lun);
>>         if (!sdev || !scsi_device_get(sdev))
>>             break;
>>     }
>>     spin_unlock_irqrestore(shost->host_lock, flags);
>>
>>
> I must say I don't really like the for(;;) construct.
> And it's really confusing as we want to find an sdev, so breaking
> if it's _not_ found is ... weird.
> 
What about

	do {
		sdev = __scsi_device_lookup_by_target(starget, sdev, lun);
	} while (sdev && scsi_device_get(sdev));

?
Should be identical and easier to read ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)
--
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] Restart scsi_device_lookup_by_target

[Matthew Wilcox]

On Tue, Jan 13, 2009 at 03:02:27PM +0100, Hannes Reinecke wrote:
> Matthew Wilcox wrote:
> >Backwards jumps are generally disapproved of.  How about:
> >
> >	spin_lock_irqsave(shost->host_lock, flags);
> >	for (;;) {
> >		sdev = __scsi_device_lookup_by_target(starget, sdev, lun);
> >		if (!sdev || !scsi_device_get(sdev))
> >			break;
> >	}
> >	spin_unlock_irqrestore(shost->host_lock, flags);

> I must say I don't really like the for(;;) construct.

I'd be fine with:

	do {
		sdev = __scsi_device_lookup_by_target(starget, sdev, lun);
	} while (sdev && scsi_device_get(sdev));

though I find that slightly less clear than the for (;;) construct.

> And it's really confusing as we want to find an sdev, so breaking
> if it's _not_ found is ... weird.

Those are the two conditions when we want to stop trying -- if there's
no device or if the device we've found is bad.  I can definitely see an
argument for splitting the two conditions to make that more obvious.  I
can also see an argument for not returning an sdev in the _DEL state
from __scsi_device_lookup_by_target in the first place.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] Restart scsi_device_lookup_by_target

[Hannes Reinecke]

Hi Matthew,

Matthew Wilcox wrote:
> On Tue, Jan 13, 2009 at 03:02:27PM +0100, Hannes Reinecke wrote:
>> Matthew Wilcox wrote:
>>> Backwards jumps are generally disapproved of.  How about:
>>>
>>> 	spin_lock_irqsave(shost->host_lock, flags);
>>> 	for (;;) {
>>> 		sdev = __scsi_device_lookup_by_target(starget, sdev, lun);
>>> 		if (!sdev || !scsi_device_get(sdev))
>>> 			break;
>>> 	}
>>> 	spin_unlock_irqrestore(shost->host_lock, flags);
> 
>> I must say I don't really like the for(;;) construct.
> 
> I'd be fine with:
> 
> 	do {
> 		sdev = __scsi_device_lookup_by_target(starget, sdev, lun);
> 	} while (sdev && scsi_device_get(sdev));
> 
> though I find that slightly less clear than the for (;;) construct.
> 
Opinons differ.
Something for the maintainer to decide. James?

>> And it's really confusing as we want to find an sdev, so breaking
>> if it's _not_ found is ... weird.
> 
> Those are the two conditions when we want to stop trying -- if there's
> no device or if the device we've found is bad.  I can definitely see an
> argument for splitting the two conditions to make that more obvious.  I
> can also see an argument for not returning an sdev in the _DEL state
> from __scsi_device_lookup_by_target in the first place.
> 
Not sure if that would work with esp_scsi.c (the other user of
__scsi_device_lookup_by_target()).
And don't have a device to try it with.

But modifying __scsi_device_lookup_by_target() would alter the behaviour,
whereas this patch does not.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)
--
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] Restart scsi_device_lookup_by_target

[James Bottomley]

On Tue, 2009-01-13 at 14:50 +0100, Hannes Reinecke wrote:
> Hi all,
> 
> currently we have this:
> 
> struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *starget,
>                                                  uint lun)
> {
>         struct scsi_device *sdev;
>         struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
>         unsigned long flags;
> 
>         spin_lock_irqsave(shost->host_lock, flags);
>         sdev = __scsi_device_lookup_by_target(starget, lun);
>         if (sdev && scsi_device_get(sdev))
>                 sdev = NULL;
>         spin_unlock_irqrestore(shost->host_lock, flags);
> 
>         return sdev;
> }
> 
> now consider an sdev list with two entries for LUN 0, the first
> being in SDEV_DEL and the second being in SDEV_RUNNING.
> 
> scsi_device_lookup_by_target will always return NULL here, as
> it will never be able to skip past the first (unuseable)
> entry. So scsi_report_lun_scan will happily create duplicate
> sdevs for LUN 0 and we'll be getting the infamous
> 
> sysfs: duplicate filename 'xxxx' can not be created
> 
> errors.
> 
> What we should be doing here is to restart
> __scsi_device_lookup_by_target() if we find an
> sdev but cannot use it (ie is in SDEV_DEL).
> 
> James, please apply.
> 
> Cheers,
> 
> Hannes
> plain text document attachment (scsi-restart-lookup-by-target)
> Restart scsi_device_lookup_by_target()
> 
> When scsi_device_lookup_by_target() will always return
> the first sdev with a matching LUN, regardless of
> the state. However, when this sdev is in SDEV_DEL
> it's quite possible to have additional sdevs in the
> list with the same LUN which are active.
> So we should restart __scsi_device_lookup_by_target()
> whenever we find an sdev with state SDEV_DEL.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

That's brilliant, thanks for finding this bug.

rather than go to the trouble of restarting the scan looking for
duplicates, since none of the users of __scsi_device_lookup_by_target
actually want invisible dying devices in SDEV_DEL, why not just make it
skip over them.  If we ignore devices in SDEV_DEL, the target id of the
visible devices should all be unique, so alter the

if (sdev->lun == lun)

to

if (sdev->lun == lun && sdev->sdev_state != SDEV_DEL)

And everything should work.

I think this is the correct fix because scsi_device_get() is also
programmed to ignore devices in SDEV_DEL (for the same reason).

James

Re: [PATCH] Restart scsi_device_lookup_by_target

[Hannes Reinecke]

Hi James,

James Bottomley wrote:
> On Tue, 2009-01-13 at 14:50 +0100, Hannes Reinecke wrote:
>> Hi all,
>>
>> currently we have this:
>>
>> struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *starget,
>>                                                  uint lun)
>> {
>>         struct scsi_device *sdev;
>>         struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
>>         unsigned long flags;
>>
>>         spin_lock_irqsave(shost->host_lock, flags);
>>         sdev = __scsi_device_lookup_by_target(starget, lun);
>>         if (sdev && scsi_device_get(sdev))
>>                 sdev = NULL;
>>         spin_unlock_irqrestore(shost->host_lock, flags);
>>
>>         return sdev;
>> }
>>
>> now consider an sdev list with two entries for LUN 0, the first
>> being in SDEV_DEL and the second being in SDEV_RUNNING.
>>
>> scsi_device_lookup_by_target will always return NULL here, as
>> it will never be able to skip past the first (unuseable)
>> entry. So scsi_report_lun_scan will happily create duplicate
>> sdevs for LUN 0 and we'll be getting the infamous
>>
>> sysfs: duplicate filename 'xxxx' can not be created
>>
>> errors.
>>
>> What we should be doing here is to restart
>> __scsi_device_lookup_by_target() if we find an
>> sdev but cannot use it (ie is in SDEV_DEL).
>>
>> James, please apply.
>>
>> Cheers,
>>
>> Hannes
>> plain text document attachment (scsi-restart-lookup-by-target)
>> Restart scsi_device_lookup_by_target()
>>
>> When scsi_device_lookup_by_target() will always return
>> the first sdev with a matching LUN, regardless of
>> the state. However, when this sdev is in SDEV_DEL
>> it's quite possible to have additional sdevs in the
>> list with the same LUN which are active.
>> So we should restart __scsi_device_lookup_by_target()
>> whenever we find an sdev with state SDEV_DEL.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
> 
> That's brilliant, thanks for finding this bug.
> 
> rather than go to the trouble of restarting the scan looking for
> duplicates, since none of the users of __scsi_device_lookup_by_target
> actually want invisible dying devices in SDEV_DEL, why not just make it
> skip over them.  If we ignore devices in SDEV_DEL, the target id of the
> visible devices should all be unique, so alter the
> 
> if (sdev->lun == lun)
> 
> to
> 
> if (sdev->lun == lun && sdev->sdev_state != SDEV_DEL)
> 
> And everything should work.
> 
> I think this is the correct fix because scsi_device_get() is also
> programmed to ignore devices in SDEV_DEL (for the same reason).
> 
As already mentioned, I'm not sure if esp_scsi.c expects it to
be that way. If someone confirms that we won't break it with this
change, then of course it's the easier way.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)
--
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] Restart scsi_device_lookup_by_target

[James Bottomley]

On Tue, 2009-01-13 at 16:28 +0100, Hannes Reinecke wrote:
> Hi James,
> 
> James Bottomley wrote:
> > On Tue, 2009-01-13 at 14:50 +0100, Hannes Reinecke wrote:
> >> Hi all,
> >>
> >> currently we have this:
> >>
> >> struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *starget,
> >>                                                  uint lun)
> >> {
> >>         struct scsi_device *sdev;
> >>         struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
> >>         unsigned long flags;
> >>
> >>         spin_lock_irqsave(shost->host_lock, flags);
> >>         sdev = __scsi_device_lookup_by_target(starget, lun);
> >>         if (sdev && scsi_device_get(sdev))
> >>                 sdev = NULL;
> >>         spin_unlock_irqrestore(shost->host_lock, flags);
> >>
> >>         return sdev;
> >> }
> >>
> >> now consider an sdev list with two entries for LUN 0, the first
> >> being in SDEV_DEL and the second being in SDEV_RUNNING.
> >>
> >> scsi_device_lookup_by_target will always return NULL here, as
> >> it will never be able to skip past the first (unuseable)
> >> entry. So scsi_report_lun_scan will happily create duplicate
> >> sdevs for LUN 0 and we'll be getting the infamous
> >>
> >> sysfs: duplicate filename 'xxxx' can not be created
> >>
> >> errors.
> >>
> >> What we should be doing here is to restart
> >> __scsi_device_lookup_by_target() if we find an
> >> sdev but cannot use it (ie is in SDEV_DEL).
> >>
> >> James, please apply.
> >>
> >> Cheers,
> >>
> >> Hannes
> >> plain text document attachment (scsi-restart-lookup-by-target)
> >> Restart scsi_device_lookup_by_target()
> >>
> >> When scsi_device_lookup_by_target() will always return
> >> the first sdev with a matching LUN, regardless of
> >> the state. However, when this sdev is in SDEV_DEL
> >> it's quite possible to have additional sdevs in the
> >> list with the same LUN which are active.
> >> So we should restart __scsi_device_lookup_by_target()
> >> whenever we find an sdev with state SDEV_DEL.
> >>
> >> Signed-off-by: Hannes Reinecke <hare@suse.de>
> > 
> > That's brilliant, thanks for finding this bug.
> > 
> > rather than go to the trouble of restarting the scan looking for
> > duplicates, since none of the users of __scsi_device_lookup_by_target
> > actually want invisible dying devices in SDEV_DEL, why not just make it
> > skip over them.  If we ignore devices in SDEV_DEL, the target id of the
> > visible devices should all be unique, so alter the
> > 
> > if (sdev->lun == lun)
> > 
> > to
> > 
> > if (sdev->lun == lun && sdev->sdev_state != SDEV_DEL)
> > 
> > And everything should work.
> > 
> > I think this is the correct fix because scsi_device_get() is also
> > programmed to ignore devices in SDEV_DEL (for the same reason).
> > 
> As already mentioned, I'm not sure if esp_scsi.c expects it to
> be that way. If someone confirms that we won't break it with this
> change, then of course it's the easier way.

I did check esp ... it's looking for the hostdata in the device, so
returning a SDEV_DEL device to it while another is in the list will
really confuse it, because it will have a stale (and possibly freed)
hostdata structure, so checking for SDEV_DEL will correct a potential
bug in it as well ... that's why I was suggesting this way.  All of the
users are expecting live and visible devices.

James