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

PATCH In review Dave Wysochanski 3 participants 5 messages Jul 27, 2006 – Aug 8, 2006 SCSI

Share & tools

Dave Wysochanski

author

2006-07-27 20:30 UTC #0 lore raw

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

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 <[email protected]>

drivers/scsi/scsi_scan.c +18 -0

@@ -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) {

Christoph Hellwig

2006-08-05 12:01 UTC #1 lore raw

Quoted 11 lines · Dave Wysochanski

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.

Quoted 7 lines

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.

Dave Wysochanski

author

2006-08-07 06:03 UTC #2 lore raw

Quoted 20 lines · Christoph Hellwig

Quoted 11 lines · Dave Wysochanski
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.
Quoted 7 lines
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.

Quoted 8 lines

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.

Quoted 4 lines

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.

Alan Stern

2006-08-07 14:45 UTC #3 lore raw

Quoted 12 lines · Dave Wysochanski

Quoted 8 lines
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

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 5 lines

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

Christoph Hellwig

2006-08-08 15:34 UTC #4 lore raw

In reply to Dave Wysochanski

Quoted 2 lines · Dave Wysochanski

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;
	}