[PATCH] Don't add scsi_device for devices that return PQ=1, PDT=0x1f

[PATCH] Don't add scsi_device for devices that return PQ=1, PDT=0x1f

[Dave Wysochanski]

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

Some targets may return PQ=1 and PDT=0x1f to indicate no LUN is mapped
(Netapp targets do this).  This seems like a valid way to indicate no
LUN mapped according to SPC-3.

However, the current scsi_probe_and_add_lun() code adds a scsi_device
for targets that return PQ=1 and PDT=0x1f.  This causes LUNs of type
"UNKNOWN" to show up in /proc/scsi/scsi when no LUNs are mapped.
In addition, subsequent rescans fail to recognize LUNs that may be
added on the target, unless preceded by a write to the delete attribute
of the "UNKNOWN" LUN.

This patch addresses this problem by skipping over the scsi_add_lun()
when PQ=1,PDT=0x1f is encountered, and just returns
SCSI_SCAN_TARGET_PRESENT.

If there are objections to this patch, I can add a BLIST flag and entry
for Netapp targets but would like to avoid that if possible, since it
seems like the current code might be closer to SPC-3 with this patch.

Signed-off-by: Dave Wysochanski <davidw@netapp.com>


[-- Attachment #2: scsi_scan_fix_pq_1_pdt_1f.patch --]
[-- Type: text/x-patch, Size: 1057 bytes --]

--- linux-2.6.18-rc2/drivers/scsi/scsi_scan.c	2006-07-15 17:53:08.000000000 -0400
+++ linux-2.6.18-rc2-lun0/drivers/scsi/scsi_scan.c	2006-07-27 15:18:59.000000000 -0400
@@ -955,6 +955,24 @@ static int scsi_probe_and_add_lun(struct
 		goto out_free_result;
 	}
 
+	/*
+	 * Some targets may set PQ=1 and PDT=0x1f to signal that no LUN
+	 * is attached, so don't add device.  From SCSI SPC-3, PQ=1:
+	 * "A peripheral device having the specified peripheral device 
+	 * type is not connected to this logical unit. However, the 
+	 * device server is capable of supporting the specified peripheral
+	 * device type on this logical unit."
+	 * Also, from SCSI SPC-3, PDT=0x1f:
+	 * "Unknown or no device type" 
+	 */
+	if ((result[0] >> 5) == 1 && (result[0] & 0x1f) == 0x1f) {
+		SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO
+					"scsi scan: PQ=1, PDT=0x1f,"
+					" no device added\n"));
+		res = SCSI_SCAN_TARGET_PRESENT;
+		goto out_free_result;
+	}
+
 	res = scsi_add_lun(sdev, result, &bflags);
 	if (res == SCSI_SCAN_LUN_PRESENT) {
 		if (bflags & BLIST_KEY) {

Re: [PATCH] Don't add scsi_device for devices that return PQ=1, PDT=0x1f

[Christoph Hellwig]

On Thu, Jul 27, 2006 at 04:30:36PM -0400, Dave Wysochanski wrote:
> Some targets may return PQ=1 and PDT=0x1f to indicate no LUN is mapped
> (Netapp targets do this).  This seems like a valid way to indicate no
> LUN mapped according to SPC-3.
> 
> However, the current scsi_probe_and_add_lun() code adds a scsi_device
> for targets that return PQ=1 and PDT=0x1f.  This causes LUNs of type
> "UNKNOWN" to show up in /proc/scsi/scsi when no LUNs are mapped.
> In addition, subsequent rescans fail to recognize LUNs that may be
> added on the target, unless preceded by a write to the delete attribute
> of the "UNKNOWN" LUN.
> 
> This patch addresses this problem by skipping over the scsi_add_lun()
> when PQ=1,PDT=0x1f is encountered, and just returns
> SCSI_SCAN_TARGET_PRESENT.
> 
> If there are objections to this patch, I can add a BLIST flag and entry
> for Netapp targets but would like to avoid that if possible, since it
> seems like the current code might be closer to SPC-3 with this patch.

If you look at scsi_probe_and_add_lun in current mainline we already
have a check for PDT=0x1f, keyed of a blacklist flag in the scsi target.
The comment above it says it's for USB UFI.  It's missing the PQ=1
check, though.  Can you reassure with the USB storage people that USB
UFI indeed sets that periphal qualifier aswell (I'd guess so as IIRC
that standard references SPC).  If that's the case replace that check
with your version and remove the target flag.

Also I'd suggest using a comment similar to the one in your patch to
describe it, but mention USB UFI and Netapp targets as real world
examples for this behaviour aswell.

Re: [PATCH] Don't add scsi_device for devices that return PQ=1, PDT=0x1f

[Dave Wysochanski]

Christoph Hellwig wrote:
> On Thu, Jul 27, 2006 at 04:30:36PM -0400, Dave Wysochanski wrote:
>  > Some targets may return PQ=1 and PDT=0x1f to indicate no LUN is mapped
>  > (Netapp targets do this).  This seems like a valid way to indicate no
>  > LUN mapped according to SPC-3.
>  >
>  > However, the current scsi_probe_and_add_lun() code adds a scsi_device
>  > for targets that return PQ=1 and PDT=0x1f.  This causes LUNs of type
>  > "UNKNOWN" to show up in /proc/scsi/scsi when no LUNs are mapped.
>  > In addition, subsequent rescans fail to recognize LUNs that may be
>  > added on the target, unless preceded by a write to the delete attribute
>  > of the "UNKNOWN" LUN.
>  >
>  > This patch addresses this problem by skipping over the scsi_add_lun()
>  > when PQ=1,PDT=0x1f is encountered, and just returns
>  > SCSI_SCAN_TARGET_PRESENT.
>  >
>  > If there are objections to this patch, I can add a BLIST flag and entry
>  > for Netapp targets but would like to avoid that if possible, since it
>  > seems like the current code might be closer to SPC-3 with this patch.
> 
> If you look at scsi_probe_and_add_lun in current mainline we already
> have a check for PDT=0x1f, keyed of a blacklist flag in the scsi target.
> The comment above it says it's for USB UFI.  It's missing the PQ=1
> check, though.  Can you reassure with the USB storage people that USB
> UFI indeed sets that periphal qualifier aswell (I'd guess so as IIRC
> that standard references SPC).  If that's the case replace that check
> with your version and remove the target flag.
> 
Yeah, I saw that.  If you look at that patch description from Alan, he
states the PQ=0 for these devices:
http://marc.theaimsgroup.com/?l=linux-scsi&m=113951679626087&w=2

Unfortunately USB UFI 1.0 spec has "reserved" where the PQ bits normally are,
so maybe that's why there's only a check for PDT=0x1f?  Alan or someone
from the usb-storage list, can you confirm this is correct (PQ really
"reserved" is 0 for the USB UFI devices)?  I thought I had a USB floppy
lying around but no luck.

Also, it does not look like pdt_1f_for_no_lun flag Alan added is used.
Alan do you intend on submitting the patch to the USB slave_alloc() function
as you mentioned?

I'm not sure Alan's pdt_1f_for_no_lun flag should be used in my case,
since I would want to set it based on vid/pid - most appropriate in
scsi_get_device_flags() - and he wanted to set it in slave_alloc().
Maybe setting it in 2 different places for different devices is ok
though - your call.


> Also I'd suggest using a comment similar to the one in your patch to
> describe it, but mention USB UFI and Netapp targets as real world
> examples for this behaviour aswell.
> 
Sure.

Re: [PATCH] Don't add scsi_device for devices that return PQ=1, PDT=0x1f

[Alan Stern]

On Mon, 7 Aug 2006, Dave Wysochanski wrote:

> > If you look at scsi_probe_and_add_lun in current mainline we already
> > have a check for PDT=0x1f, keyed of a blacklist flag in the scsi target.
> > The comment above it says it's for USB UFI.  It's missing the PQ=1
> > check, though.  Can you reassure with the USB storage people that USB
> > UFI indeed sets that periphal qualifier aswell (I'd guess so as IIRC
> > that standard references SPC).  If that's the case replace that check
> > with your version and remove the target flag.
> > 
> Yeah, I saw that.  If you look at that patch description from Alan, he
> states the PQ=0 for these devices:
> http://marc.theaimsgroup.com/?l=linux-scsi&m=113951679626087&w=2
> 
> Unfortunately USB UFI 1.0 spec has "reserved" where the PQ bits normally are,
> so maybe that's why there's only a check for PDT=0x1f?  Alan or someone
> from the usb-storage list, can you confirm this is correct (PQ really
> "reserved" is 0 for the USB UFI devices)?  I thought I had a USB floppy
> lying around but no luck.

I don't have any UFI devices, but Matt Dharm probably does.  Maybe he can
verify this.

Although I've lost track of the original email thread leading to that 
patch, it seems clear that there never would have been a problem if the 
UFI device in question had set PQ=3.  Since the UFI spec really does say 
that those bits are reserved and should be set to 0, it's reasonable to 
assume that lots of UFI devices will do just that.

> Also, it does not look like pdt_1f_for_no_lun flag Alan added is used.
> Alan do you intend on submitting the patch to the USB slave_alloc() function
> as you mentioned?

You know, that was half a year ago and it simply slipped my mind.  Partly 
because I never know when James accepts one of my patches.  But yes, now 
that it's there I will add appropriate code to usb-storage.

> I'm not sure Alan's pdt_1f_for_no_lun flag should be used in my case,
> since I would want to set it based on vid/pid - most appropriate in
> scsi_get_device_flags() - and he wanted to set it in slave_alloc().
> Maybe setting it in 2 different places for different devices is ok
> though - your call.

I don't see any problem with setting the flag from two different places.  
We already do the same sort of thing with other flags.  It doesn't matter
_where_ the flag gets set; all that matters is that it _is_ set during the
scanning.

Alan Stern

Re: [PATCH] Don't add scsi_device for devices that return PQ=1, PDT=0x1f

[Christoph Hellwig]

On Mon, Aug 07, 2006 at 02:03:10AM -0400, Dave Wysochanski wrote:
> I'm not sure Alan's pdt_1f_for_no_lun flag should be used in my case,
> since I would want to set it based on vid/pid

I don't think we should set any flag for the netapp behaviour.  It might
be stretching the standard a little, but it's not completely odd and I
can't think of anything bad that could hapen if we do it unconditonally.

Something like:

	if ((result[0] >> 5) == 1 || starget->some_flag) &&
	    (result[0] & 0x1f) == 0x1f) {
	    	SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO
			"scsi scan: peripheral device type"
			" of 31, no device added\n"));
		res = SCSI_SCAN_TARGET_PRESENT;
		goto out_free_result;
	}