[PATCH] Sanitize PQ3 device handling (Take #2)

[PATCH] Sanitize PQ3 device handling (Take #2)

[Hannes Reinecke]

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

Hi James,

this is an updated version of the patch. As I suspected, the issue isn't
quite as easy. Problem is that older SCSI-2 device have the habit for
responding to LUN1 in addition to LUN0, even though only one device is
attached. LUN1 has set PQ to 3 accordingly.
And of course we _don't_ want those fake devices to stick around.
So I've improved the logic to not register devices with LUN != 0 and a
PQ of 1 or 3. All other devices are registered accordingly.

I can't really see why we should register devices with PQ 1 and LUN !=
0; if one wants to have them he still can do a REPORT LUN on LUN 0 and
an explicit scan for the missing device.

And I do think the locking is slightly wrong for the failure case;
without this adaptecs have a habit of crashing when doing a rmmod.

But again, comments etc are welcome.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke			hare@suse.de
SuSE Linux AG				S390 & zSeries
Maxfeldstraße 5				+49 911 74053 688
90409 Nürnberg				http://www.suse.de

[-- Attachment #2: linux-2.6.12-rc4-scsi-scan-sparse-luns --]
[-- Type: text/plain, Size: 8994 bytes --]

From: Hannes Reinecke <hare@suse.de>
Subject: Sanitize PQ 3 devices

The current handling of devices with a PQ of 3 is plain broken.
Whenever we detect such a device, no device check with REPORT LUNs is
made but rather a SCSI-2 sequential scan is done. This is plain wrong
for modern storage servers who use LUN 0 as a virtual device to be
used for configuration, eg REPORT LUN queries.

Improved scanning logic:
The SCSI_SCAN_XX flags have been renamed to the more precise
SCSI_SCAN_TARGET_PRESENT and SCSI_SCAN_TARGET_IGNORED.
First LUN 0 is scanned; if any device is detected a report_lun scan is
attempted. This will bail out for any device which does not support
it; for those a sequential scan is performed.
All devices are registered with the midlayer except those which report
a PQ of 1 or 3 and have a LUN != 0. LUN 0 will always be registered as
we might need it for configuration / queries.

Oh, and IMHO the locking is wrong for failed devices.

--- linux-2.6.12-rc4/drivers/scsi/scsi_scan.c.orig	2005-05-19 15:57:51.000000000 +0200
+++ linux-2.6.12-rc4/drivers/scsi/scsi_scan.c	2005-05-19 16:18:26.000000000 +0200
@@ -64,15 +64,15 @@
  * SCSI_SCAN_NO_RESPONSE: no valid response received from the target, this
  * includes allocation or general failures preventing IO from being sent.
  *
- * SCSI_SCAN_TARGET_PRESENT: target responded, but no device is available
- * on the given LUN.
+ * SCSI_SCAN_TARGET_IGNORED: target responded, but is ignored from the
+ * midlayer
  *
- * SCSI_SCAN_LUN_PRESENT: target responded, and a device is available on a
- * given LUN.
+ * SCSI_SCAN_TARGET_PRESENT: target responded, and is registered with the
+ * midlayer
  */
 #define SCSI_SCAN_NO_RESPONSE		0
-#define SCSI_SCAN_TARGET_PRESENT	1
-#define SCSI_SCAN_LUN_PRESENT		2
+#define SCSI_SCAN_TARGET_IGNORED	1
+#define SCSI_SCAN_TARGET_PRESENT	2
 
 static char *scsi_null_device_strs = "nullnullnullnull";
 
@@ -282,6 +282,9 @@ static struct scsi_device *scsi_alloc_sd
 out_device_destroy:
 	transport_destroy_device(&sdev->sdev_gendev);
 	scsi_free_queue(sdev->request_queue);
+	put_device(&starget->dev);
+	put_device(&sdev->transport_classdev);
+	put_device(&sdev->sdev_classdev);
 	put_device(&sdev->sdev_gendev);
 out:
 	if (display_failure_msg)
@@ -553,8 +556,7 @@ static void scsi_probe_lun(struct scsi_r
 
 	/*
 	 * The scanning code needs to know the scsi_level, even if no
-	 * device is attached at LUN 0 (SCSI_SCAN_TARGET_PRESENT) so
-	 * non-zero LUNs can be scanned.
+	 * device is attached at LUN 0 so that non-zero LUNs can be scanned.
 	 */
 	sdev->scsi_level = inq_result[2] & 0x07;
 	if (sdev->scsi_level >= 2 ||
@@ -579,7 +581,7 @@ static void scsi_probe_lun(struct scsi_r
  *
  * Return:
  *     SCSI_SCAN_NO_RESPONSE: could not allocate or setup a Scsi_Device
- *     SCSI_SCAN_LUN_PRESENT: a new Scsi_Device was allocated and initialized
+ *     SCSI_SCAN_TARGET_PRESENT: a new Scsi_Device was allocated and initialized
  **/
 static int scsi_add_lun(struct scsi_device *sdev, char *inq_result, int *bflags)
 {
@@ -651,6 +653,11 @@ static int scsi_add_lun(struct scsi_devi
 	 * Don't set the device offline here; rather let the upper
 	 * level drivers eval the PQ to decide whether they should
 	 * attach. So remove ((inq_result[0] >> 5) & 7) == 1 check.
+	 *
+	 * Since we don't set the device offline anymore there is
+	 * no sense at all in treating PQ 1 and PQ 3 differently
+	 * as both do not have a device attached. If PQ 1 targets
+	 * ever get it's device back we need to rescan anyway.
 	 */ 
 
 	sdev->inq_periph_qual = (inq_result[0] >> 5) & 7;
@@ -739,7 +746,7 @@ static int scsi_add_lun(struct scsi_devi
 	 */
 	scsi_sysfs_add_sdev(sdev);
 
-	return SCSI_SCAN_LUN_PRESENT;
+	return SCSI_SCAN_TARGET_PRESENT;
 }
 
 /**
@@ -756,9 +763,8 @@ static int scsi_add_lun(struct scsi_devi
  *
  * Return:
  *     SCSI_SCAN_NO_RESPONSE: could not allocate or setup a Scsi_Device
- *     SCSI_SCAN_TARGET_PRESENT: target responded, but no device is
- *         attached at the LUN
- *     SCSI_SCAN_LUN_PRESENT: a new Scsi_Device was allocated and initialized
+ *     SCSI_SCAN_TARGET_IGNORED: target responded but was ignored.
+ *     SCSI_SCAN_TARGET_PRESENT: a new Scsi_Device was allocated and initialized
  **/
 static int scsi_probe_and_add_lun(struct scsi_target *starget,
 				  uint lun, int *bflagsp,
@@ -790,7 +796,7 @@ static int scsi_probe_and_add_lun(struct
 				*bflagsp = scsi_get_device_flags(sdev,
 								 sdev->vendor,
 								 sdev->model);
-			return SCSI_SCAN_LUN_PRESENT;
+			return SCSI_SCAN_TARGET_PRESENT;
 		}
 	}
 
@@ -812,26 +818,23 @@ static int scsi_probe_and_add_lun(struct
 	/*
 	 * result contains valid SCSI INQUIRY data.
 	 */
-	if ((result[0] >> 5) == 3) {
+	if ( lun != 0 && ((result[0] >> 5) == 3) || ((result[0] >> 5) == 1)) {
 		/*
-		 * For a Peripheral qualifier 3 (011b), the SCSI
-		 * spec says: The device server is not capable of
-		 * supporting a physical device on this logical
-		 * unit.
+		 * The Peripheral qualifier indicates that there
+		 * is no physical device connected.
 		 *
-		 * For disks, this implies that there is no
-		 * logical disk configured at sdev->lun, but there
-		 * is a target id responding.
+		 * So ignore this device if it's not LUN 0;
+		 * keep LUN 0 around so as to send inquiries to it.
 		 */
 		SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO
-					"scsi scan: peripheral qualifier of 3,"
-					" no device added\n"));
-		res = SCSI_SCAN_TARGET_PRESENT;
+					"scsi scan: no physical device present,"
+					" device not added\n"));
+		res = SCSI_SCAN_TARGET_IGNORED;
 		goto out_free_result;
 	}
 
 	res = scsi_add_lun(sdev, result, &bflags);
-	if (res == SCSI_SCAN_LUN_PRESENT) {
+	if (res != SCSI_SCAN_NO_RESPONSE) {
 		if (bflags & BLIST_KEY) {
 			sdev->lockable = 0;
 			scsi_unlock_floptical(sreq, result);
@@ -845,7 +848,7 @@ static int scsi_probe_and_add_lun(struct
  out_free_sreq:
 	scsi_release_request(sreq);
  out_free_sdev:
-	if (res == SCSI_SCAN_LUN_PRESENT) {
+	if (res == SCSI_SCAN_TARGET_PRESENT) {
 		if (sdevp) {
 			scsi_device_get(sdev);
 			*sdevp = sdev;
@@ -854,6 +857,8 @@ static int scsi_probe_and_add_lun(struct
 		if (sdev->host->hostt->slave_destroy)
 			sdev->host->hostt->slave_destroy(sdev);
 		transport_destroy_device(&sdev->sdev_gendev);
+		put_device(&sdev->transport_classdev);
+		put_device(&sdev->sdev_classdev);
 		put_device(&sdev->sdev_gendev);
 	}
  out:
@@ -899,7 +904,7 @@ static void scsi_sequential_lun_scan(str
 	 * If not sparse lun and no device attached at LUN 0 do not scan
 	 * any further.
 	 */
-	if (!sparse_lun && (lun0_res != SCSI_SCAN_LUN_PRESENT))
+	if (!sparse_lun && (lun0_res != SCSI_SCAN_TARGET_PRESENT))
 		return;
 
 	/*
@@ -944,7 +949,7 @@ static void scsi_sequential_lun_scan(str
 	 */
 	for (lun = 1; lun < max_dev_lun; ++lun)
 		if ((scsi_probe_and_add_lun(starget, lun, NULL, NULL, rescan,
-					    NULL) != SCSI_SCAN_LUN_PRESENT) &&
+					    NULL) != SCSI_SCAN_TARGET_PRESENT) &&
 		    !sparse_lun)
 			return;
 }
@@ -1199,7 +1204,7 @@ struct scsi_device *__scsi_add_device(st
 
 	down(&shost->scan_mutex);
 	res = scsi_probe_and_add_lun(starget, lun, NULL, &sdev, 1, hostdata);
-	if (res != SCSI_SCAN_LUN_PRESENT)
+	if (res != SCSI_SCAN_TARGET_PRESENT)
 		sdev = ERR_PTR(-ENODEV);
 	up(&shost->scan_mutex);
 	scsi_target_reap(starget);
@@ -1215,7 +1220,6 @@ void scsi_rescan_device(struct device *d
 	
 	if (!dev->driver)
 		return;
-
 	drv = to_scsi_driver(dev->driver);
 	if (try_module_get(drv->owner)) {
 		if (drv->rescan)
@@ -1279,23 +1283,30 @@ void scsi_scan_target(struct device *par
 	 * would not configure LUN 0 until all LUNs are scanned.
 	 */
 	res = scsi_probe_and_add_lun(starget, 0, &bflags, &sdev, rescan, NULL);
-	if (res == SCSI_SCAN_LUN_PRESENT) {
-		if (scsi_report_lun_scan(sdev, bflags, rescan) != 0)
+	if (res != SCSI_SCAN_NO_RESPONSE) {
+		if (scsi_report_lun_scan(sdev, bflags, rescan) != 0) {
 			/*
 			 * The REPORT LUN did not scan the target,
 			 * do a sequential scan.
 			 */
+			if (res == SCSI_SCAN_TARGET_IGNORED)
+				/*
+				 * There's a target here, but lun 0 is not 
+				 * connected to a device and does not support
+				 * the report_lun scan.  Fall back to a 
+				 * sequential lun scan with a bflags of 
+				 * SPARSELUN.
+				 *
+				 * The old code also used a default scsi level
+				 * of SCSI_2 which seems a bit spurious. Any
+				 * misbehaving device should rather be added
+				 * to the blacklist.
+				 */
+				bflags |= BLIST_SPARSELUN;
+
 			scsi_sequential_lun_scan(starget, bflags,
 				       	res, sdev->scsi_level, rescan);
-	} else if (res == SCSI_SCAN_TARGET_PRESENT) {
-		/*
-		 * There's a target here, but lun 0 is offline so we
-		 * can't use the report_lun scan.  Fall back to a
-		 * sequential lun scan with a bflags of SPARSELUN and
-		 * a default scsi level of SCSI_2
-		 */
-		scsi_sequential_lun_scan(starget, BLIST_SPARSELUN,
-				SCSI_SCAN_TARGET_PRESENT, SCSI_2, rescan);
+		}
 	}
 	if (sdev)
 		scsi_device_put(sdev);

Re: [PATCH] Sanitize PQ3 device handling (Take #2)

[Patrick Mansfield]

On Thu, May 19, 2005 at 04:38:27PM +0200, Hannes Reinecke wrote:
> Hi James,
> 
> this is an updated version of the patch. As I suspected, the issue isn't
> quite as easy. Problem is that older SCSI-2 device have the habit for
> responding to LUN1 in addition to LUN0, even though only one device is
> attached. LUN1 has set PQ to 3 accordingly.
> And of course we _don't_ want those fake devices to stick around.
> So I've improved the logic to not register devices with LUN != 0 and a
> PQ of 1 or 3. All other devices are registered accordingly.
> 
> I can't really see why we should register devices with PQ 1 and LUN !=
> 0; if one wants to have them he still can do a REPORT LUN on LUN 0 and
> an explicit scan for the missing device.
> 
> And I do think the locking is slightly wrong for the failure case;
> without this adaptecs have a habit of crashing when doing a rmmod.

Is that what the extra put's are fixing? That should be a separate patch.

> But again, comments etc are welcome.

I had a similar patch, that did not leave LUN 0 configured: 

http://marc.theaimsgroup.com/?l=linux-scsi&m=110297733824960&w=2

It did not change handling of PQ.

I don't have a strong opinion for either direction, but having LUN 0 with
and sometimes without an sd there bothers me. Pick your poison ...

Long term, a way to access any LUN on a target from user space would be
very useful, not just LUN 0.

And ...

> --- linux-2.6.12-rc4/drivers/scsi/scsi_scan.c.orig	2005-05-19 15:57:51.000000000 +0200
> +++ linux-2.6.12-rc4/drivers/scsi/scsi_scan.c	2005-05-19 16:18:26.000000000 +0200
> @@ -64,15 +64,15 @@
>   * SCSI_SCAN_NO_RESPONSE: no valid response received from the target, this
>   * includes allocation or general failures preventing IO from being sent.
>   *
> - * SCSI_SCAN_TARGET_PRESENT: target responded, but no device is available
> - * on the given LUN.
> + * SCSI_SCAN_TARGET_IGNORED: target responded, but is ignored from the
> + * midlayer
>   *
> - * SCSI_SCAN_LUN_PRESENT: target responded, and a device is available on a
> - * given LUN.
> + * SCSI_SCAN_TARGET_PRESENT: target responded, and is registered with the
> + * midlayer
>   */
>  #define SCSI_SCAN_NO_RESPONSE		0
> -#define SCSI_SCAN_TARGET_PRESENT	1
> -#define SCSI_SCAN_LUN_PRESENT		2
> +#define SCSI_SCAN_TARGET_IGNORED	1
> +#define SCSI_SCAN_TARGET_PRESENT	2

I prefer naming with all LUN in all the defines since we are returning LUN
status, or leave as is. For SCSI_SCAN_TARGET_IGNORED, we are not
configuring the LUN.

i.e. keep the original:

#define SCSI_SCAN_NO_RESPONSE		0
#define SCSI_SCAN_TARGET_PRESENT	1
#define SCSI_SCAN_LUN_PRESENT		2

Or:

#define SCSI_SCAN_NO_RESPONSE		0
#define SCSI_SCAN_LUN_IGNORED		1
#define SCSI_SCAN_LUN_PRESENT		2


You should add code to not print the "unknown device type" if PQ 3 or 1,
or special case type 31 (0x1f).

SCSI spec says for PQ of 011 (3):

	The device server is not capable of supporting a peripheral device
	on this logical unit.  For this peripheral qualifier the
	peripheral device type shall be set to 1Fh to provide
	compatibility with previous versions of SCSI.

We see the warnings now even for PQ of 1 (I don't know the age of these
devices):

scsi: unknown device type 31
  Vendor: IBM       Model: 5703001           Rev: 0150
    Type:   Unknown                            ANSI SCSI revision: 00

For PQ of 3 there should be a lot more.

-- Patrick Mansfield

Re: [PATCH] Sanitize PQ3 device handling (Take #2)

[Hannes Reinecke]

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

Patrick Mansfield wrote:
> On Thu, May 19, 2005 at 04:38:27PM +0200, Hannes Reinecke wrote:
>>Hi James,
>>
>>this is an updated version of the patch. As I suspected, the issue isn't
>>quite as easy. Problem is that older SCSI-2 device have the habit for
>>responding to LUN1 in addition to LUN0, even though only one device is
>>attached. LUN1 has set PQ to 3 accordingly.
>>And of course we _don't_ want those fake devices to stick around.
>>So I've improved the logic to not register devices with LUN != 0 and a
>>PQ of 1 or 3. All other devices are registered accordingly.
>>
>>I can't really see why we should register devices with PQ 1 and LUN !=
>>0; if one wants to have them he still can do a REPORT LUN on LUN 0 and
>>an explicit scan for the missing device.
>>
>>And I do think the locking is slightly wrong for the failure case;
>>without this adaptecs have a habit of crashing when doing a rmmod.
> 
> Is that what the extra put's are fixing? That should be a separate patch.
> 
Hm. Yes, probably.

[ .. ]
> I don't have a strong opinion for either direction, but having LUN 0 with
> and sometimes without an sd there bothers me. Pick your poison ...
> 
I don't really have a problem with having sd attached to LUN 0
sometimes. We're doing only what the SCSI device tells us ...

> Long term, a way to access any LUN on a target from user space would be
> very useful, not just LUN 0.
> 
That's what I thought also; only some dastardly SCSI-2 drives respond to
LUN 0 and LUN 1 (actually all SCSI-2 device which I have connected here;
wonder whether this is again some 'interesting' specification
interpretation). So it's doubtful whether this a possible way to go.
Or maybe return all LUNs for SCSI-3 and higher?

[ .. ]
> I prefer naming with all LUN in all the defines since we are returning LUN
> status, or leave as is. For SCSI_SCAN_TARGET_IGNORED, we are not
> configuring the LUN.
> 
> i.e. keep the original:
> 
> #define SCSI_SCAN_NO_RESPONSE		0
> #define SCSI_SCAN_TARGET_PRESENT	1
> #define SCSI_SCAN_LUN_PRESENT		2
> 
> Or:
> 
> #define SCSI_SCAN_NO_RESPONSE		0
> #define SCSI_SCAN_LUN_IGNORED		1
> #define SCSI_SCAN_LUN_PRESENT		2
> 
> 
Yes, this makes more sense. Still think having SCSI_SCAN_LUN_IGNORED
instead of SCSI_SCAN_TARGET_PRESENT is more sensible as the meaning of
those flags changed slightly (flags indicate now the device state in
scsi-ml, not the state according to the SCSI-bus).

> You should add code to not print the "unknown device type" if PQ 3 or 1,
> or special case type 31 (0x1f).
> 
> SCSI spec says for PQ of 011 (3):
> 
> 	The device server is not capable of supporting a peripheral device
> 	on this logical unit.  For this peripheral qualifier the
> 	peripheral device type shall be set to 1Fh to provide
> 	compatibility with previous versions of SCSI.
> 
Agreed.

Fixed patch attached.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke			hare@suse.de
SuSE Linux AG				S390 & zSeries
Maxfeldstraße 5				+49 911 74053 688
90409 Nürnberg				http://www.suse.de

[-- Attachment #2: linux-2.6.12-rc4-scsi-scan-sparse-luns --]
[-- Type: text/plain, Size: 6024 bytes --]

From: Hannes Reinecke <hare@suse.de>
Subject: Sanitize PQ3 device handling

The current handling of devices with a PQ of 3 is plain broken.
Whenever we detect such a device, no device check with REPORT LUNs is
made but rather a SCSI-2 sequential scan is done. This is plain wrong
for modern storage servers who use LUN 0 as a virtual device to be
used for configuration, eg REPORT LUN queries.

Improved scanning logic:
First LUN 0 is scanned then a report_lun scan is attempted if a device
is detected. This will bail out for any device which does not support
it; for those a sequential scan is performed.
All devices are registered with the midlayer except those which report
a PQ of 1 or 3 and have a LUN != 0. LUN 0 will always be registered as
we might need it for configuration / queries.

--- linux-2.6.12-rc4.orig/drivers/scsi/scsi_scan.c	2005-05-07 07:20:31.000000000 +0200
+++ linux-2.6.12-rc4/drivers/scsi/scsi_scan.c	2005-05-23 15:51:24.000000000 +0200
@@ -64,15 +64,15 @@
  * SCSI_SCAN_NO_RESPONSE: no valid response received from the target, this
  * includes allocation or general failures preventing IO from being sent.
  *
- * SCSI_SCAN_TARGET_PRESENT: target responded, but no device is available
- * on the given LUN.
+ * SCSI_SCAN_LUN_IGNORED: target responded, but is ignored from the
+ * midlayer
  *
- * SCSI_SCAN_LUN_PRESENT: target responded, and a device is available on a
- * given LUN.
+ * SCSI_SCAN_LUN_PRESENT: target responded, and is registered with the
+ * midlayer
  */
 #define SCSI_SCAN_NO_RESPONSE		0
-#define SCSI_SCAN_TARGET_PRESENT	1
-#define SCSI_SCAN_LUN_PRESENT		2
+#define SCSI_SCAN_LUN_IGNORED		1
+#define SCSI_SCAN_LUN_PRESENT		2
 
 static char *scsi_null_device_strs = "nullnullnullnull";
 
@@ -553,8 +556,7 @@
 
 	/*
 	 * The scanning code needs to know the scsi_level, even if no
-	 * device is attached at LUN 0 (SCSI_SCAN_TARGET_PRESENT) so
-	 * non-zero LUNs can be scanned.
+	 * device is attached at LUN 0 so that non-zero LUNs can be scanned.
 	 */
 	sdev->scsi_level = inq_result[2] & 0x07;
 	if (sdev->scsi_level >= 2 ||
@@ -615,6 +617,7 @@
 	} else if (*bflags & BLIST_NO_ULD_ATTACH)
 		sdev->no_uld_attach = 1;
 
+	sdev->inq_periph_qual = (inq_result[0] >> 5) & 7;
 	switch (sdev->type = (inq_result[0] & 0x1f)) {
 	case TYPE_TAPE:
 	case TYPE_DISK:
@@ -632,7 +635,8 @@
 		sdev->writeable = 0;
 		break;
 	default:
-		printk(KERN_INFO "scsi: unknown device type %d\n", sdev->type);
+		if (sdev->inq_periph_qual != 1 && sdev->inq_periph_qual != 3)
+			printk(KERN_INFO "scsi: unknown device type %d\n", sdev->type);
 	}
 
 	print_inquiry(inq_result);
@@ -651,9 +655,13 @@
 	 * Don't set the device offline here; rather let the upper
 	 * level drivers eval the PQ to decide whether they should
 	 * attach. So remove ((inq_result[0] >> 5) & 7) == 1 check.
+	 *
+	 * Since we don't set the device offline anymore there is
+	 * no sense at all in treating PQ 1 and PQ 3 differently
+	 * as both do not have a device attached. If PQ 1 targets
+	 * ever get it's device back we need to rescan anyway.
 	 */ 
 
-	sdev->inq_periph_qual = (inq_result[0] >> 5) & 7;
 	sdev->removable = (0x80 & inq_result[1]) >> 7;
 	sdev->lockable = sdev->removable;
 	sdev->soft_reset = (inq_result[7] & 1) && ((inq_result[3] & 7) == 2);
@@ -756,8 +764,7 @@
  *
  * Return:
  *     SCSI_SCAN_NO_RESPONSE: could not allocate or setup a Scsi_Device
- *     SCSI_SCAN_TARGET_PRESENT: target responded, but no device is
- *         attached at the LUN
+ *     SCSI_SCAN_LUN_IGNORED: target responded but was ignored.
  *     SCSI_SCAN_LUN_PRESENT: a new Scsi_Device was allocated and initialized
  **/
 static int scsi_probe_and_add_lun(struct scsi_target *starget,
@@ -812,21 +819,18 @@
 	/*
 	 * result contains valid SCSI INQUIRY data.
 	 */
-	if ((result[0] >> 5) == 3) {
+	if ( lun != 0 && ((result[0] >> 5) == 3) || ((result[0] >> 5) == 1)) {
 		/*
-		 * For a Peripheral qualifier 3 (011b), the SCSI
-		 * spec says: The device server is not capable of
-		 * supporting a physical device on this logical
-		 * unit.
+		 * The Peripheral qualifier indicates that there
+		 * is no physical device connected.
 		 *
-		 * For disks, this implies that there is no
-		 * logical disk configured at sdev->lun, but there
-		 * is a target id responding.
+		 * So ignore this device if it's not LUN 0;
+		 * keep LUN 0 around so as to send inquiries to it.
 		 */
 		SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO
-					"scsi scan: peripheral qualifier of 3,"
-					" no device added\n"));
-		res = SCSI_SCAN_TARGET_PRESENT;
+					"scsi scan: no physical device present,"
+					" device not added\n"));
+		res = SCSI_SCAN_LUN_IGNORED;
 		goto out_free_result;
 	}
 
@@ -1280,22 +1285,29 @@
 	 */
 	res = scsi_probe_and_add_lun(starget, 0, &bflags, &sdev, rescan, NULL);
 	if (res == SCSI_SCAN_LUN_PRESENT) {
-		if (scsi_report_lun_scan(sdev, bflags, rescan) != 0)
+		if (scsi_report_lun_scan(sdev, bflags, rescan) != 0) {
 			/*
 			 * The REPORT LUN did not scan the target,
 			 * do a sequential scan.
 			 */
+			if (res == SCSI_SCAN_LUN_IGNORED)
+				/*
+				 * There's a target here, but lun 0 is not 
+				 * connected to a device and does not support
+				 * the report_lun scan.  Fall back to a 
+				 * sequential lun scan with a bflags of 
+				 * SPARSELUN.
+				 *
+				 * The old code also used a default scsi level
+				 * of SCSI_2 which seems a bit spurious. Any
+				 * misbehaving device should rather be added
+				 * to the blacklist.
+				 */
+				bflags |= BLIST_SPARSELUN;
+
 			scsi_sequential_lun_scan(starget, bflags,
 				       	res, sdev->scsi_level, rescan);
-	} else if (res == SCSI_SCAN_TARGET_PRESENT) {
-		/*
-		 * There's a target here, but lun 0 is offline so we
-		 * can't use the report_lun scan.  Fall back to a
-		 * sequential lun scan with a bflags of SPARSELUN and
-		 * a default scsi level of SCSI_2
-		 */
-		scsi_sequential_lun_scan(starget, BLIST_SPARSELUN,
-				SCSI_SCAN_TARGET_PRESENT, SCSI_2, rescan);
+		}
 	}
 	if (sdev)
 		scsi_device_put(sdev);

Re: [PATCH] Sanitize PQ3 device handling (Take #2)

[James Bottomley]

On Tue, 2005-05-24 at 10:24 +0200, Hannes Reinecke wrote:
> -		printk(KERN_INFO "scsi: unknown device type %d\n", sdev->type);
> +		if (sdev->inq_periph_qual != 1 && sdev->inq_periph_qual != 3)
> +			printk(KERN_INFO "scsi: unknown device type %d\n", sdev->type);

The device type for PQ1 should be reliable, it's only PQ3 that has 0x1f

Also, we have defines for this (SCSI_INQ_PQ_NOT_CON and
SCSI_INQ_PQ_NOT_CAP)

>  	if (res == SCSI_SCAN_LUN_PRESENT) {
> -		if (scsi_report_lun_scan(sdev, bflags, rescan) != 0)
> +		if (scsi_report_lun_scan(sdev, bflags, rescan) != 0) {
>  			/*
>  			 * The REPORT LUN did not scan the target,
>  			 * do a sequential scan.
>  			 */
> +			if (res == SCSI_SCAN_LUN_IGNORED)

This condition is impossible.

> +				/*
> +				 * There's a target here, but lun 0 is not 
> +				 * connected to a device and does not support
> +				 * the report_lun scan.  Fall back to a 
> +				 * sequential lun scan with a bflags of 
> +				 * SPARSELUN.
> +				 *
> +				 * The old code also used a default scsi level
> +				 * of SCSI_2 which seems a bit spurious. Any
> +				 * misbehaving device should rather be added
> +				 * to the blacklist.
> +				 */
> +				bflags |= BLIST_SPARSELUN;

The reason the old code did all that is because the inquiry data of a
PQ3 device isn't credible and shouldn't be acted on.

>  			scsi_sequential_lun_scan(starget, bflags,
>  				       	res, sdev->scsi_level, rescan);

If you really have the SCSI_SCAN_LUN_IGNORED case here, you've already
released sdev in scsi_probe_and_add_lun, so you can't use it here.

Finally, I think you should probably update the scsi_match function not
to attach on PQ3, just in case someone violates spec and has a valid
type.

James