Skip to contentSkip to conversation

Sanitize PQ3 device handling

PATCH In review Hannes Reinecke 3 participants 6 messages May 18, 2005 SCSI

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Sanitize PQ3 device handling

Hi James,

the attached patch sanitzes the handling of devices with a periperal qualifier of 3. As we don't set non-existing devices offline anymore there is no real sense in distinguishing between PQ 1 and PQ3, as the net effect (no available device) is identical. However, we really want to have access to the target via sg.

Please apply.

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke			[email protected]
SuSE Linux AG				S390 & zSeries
Maxfeldstraße 5				+49 911 74053 688
90409 Nürnberg				http://www.suse.de
drivers/scsi/scsi_scan.c +37 -33
diff -pur linux-2.6.12-rc4.orig/drivers/scsi/scsi_scan.c linux-2.6.12-rc4/drivers/scsi/scsi_scan.c
@@ -579,6 +579,8 @@ static void scsi_probe_lun(struct scsi_r
  *
  * Return:
  *     SCSI_SCAN_NO_RESPONSE: could not allocate or setup a Scsi_Device
+ *     SCSI_SCAN_TARGET_PRESENT: a Scsi_Device was allocated and initialized
+ *                               however there is (currently) no LUN connected.
  *     SCSI_SCAN_LUN_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 a PQ 1 target
+	 * ever get it's device back we need to rescan it anyway.
 	 */ 
 
 	sdev->inq_periph_qual = (inq_result[0] >> 5) & 7;
@@ -739,7 +746,16 @@ static int scsi_add_lun(struct scsi_devi
 	 */
 	scsi_sysfs_add_sdev(sdev);
 
-	return SCSI_SCAN_LUN_PRESENT;
+	/*
+	 * Targets with a PQ of 1 or 3 do not have
+	 * a device attached; tell the upper layers
+	 * about it.
+	 */
+	if (((inq_result[0] >> 5) & 7) == 1 ||
+	    ((inq_result[0] >> 5) & 7) == 3)
+		return SCSI_SCAN_TARGET_PRESENT;
+	else
+		return SCSI_SCAN_LUN_PRESENT;
 }
 
 /**
@@ -812,26 +828,8 @@ static int scsi_probe_and_add_lun(struct
 	/*
 	 * result contains valid SCSI INQUIRY data.
 	 */
-	if ((result[0] >> 5) == 3) {
-		/*
-		 * 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.
-		 *
-		 * For disks, this implies that there is no
-		 * logical disk configured at sdev->lun, but there
-		 * is a target id responding.
-		 */
-		SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO
-					"scsi scan: peripheral qualifier of 3,"
-					" 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 (res != SCSI_SCAN_NO_RESPONSE) {
 		if (bflags & BLIST_KEY) {
 			sdev->lockable = 0;
 			scsi_unlock_floptical(sreq, result);
@@ -845,7 +843,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_NO_RESPONSE) {
 		if (sdevp) {
 			scsi_device_get(sdev);
 			*sdevp = sdev;
@@ -1199,7 +1197,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_NO_RESPONSE)
 		sdev = ERR_PTR(-ENODEV);
 	up(&shost->scan_mutex);
 	scsi_target_reap(starget);
@@ -1279,23 +1277,29 @@ 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_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.
+				 *
+				 * 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);

Only in linux-2.6.12-rc4/drivers/scsi: scsi_scan.c~ Only in linux-2.6.12-rc4/drivers/scsi: scsi_scan.c.orig

#1 lore raw
Quoted 15 lines
drivers/scsi/scsi_scan.c +8 -2
 	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_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.

drivers/scsi/scsi_scan.c void scsi_scan_target(struct device *par · line 1301

Quoted 10 lines
drivers/scsi/scsi_scan.c +8 -0
+				 *
+				 * 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);

If scsi_probe_and_add_lun returns SCSI_SCAN_TARGET_PRESENT sdev isn't valid at this point. You should probably default to SCSI_2 for that case.

#2 lore raw
Quoted 20 lines · Christoph Hellwig
Quoted 20 lines
	res = scsi_probe_and_add_lun(starget, 0, &bflags, &sdev, rescan, NULL);
drivers/scsi/scsi_scan.c +2 -2
-	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.
			 */
drivers/scsi/scsi_scan.c +11 -0
+			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.
+				 *
+				 * 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.
Quoted 9 lines
Quoted 5 lines
drivers/scsi/scsi_scan.c +3 -0
+				 */
+				bflags |= BLIST_SPARSELUN;
+
			scsi_sequential_lun_scan(starget, bflags,
				       	res, sdev->scsi_level, rescan);

If scsi_probe_and_add_lun returns SCSI_SCAN_TARGET_PRESENT sdev isn't valid at this point. You should probably default to SCSI_2 for that case.

How so? This is exactly the behaviour I've removed. sdev is valid even for SCSI_SCAN_TARGET_PRESENT.
If not please show me.

Cheers,

Hannes

Signature · 8 lines
Dr. Hannes Reinecke			[email protected]
SuSE Linux AG				S390 & zSeries
Maxfeldstraße 5				+49 911 74053 688
90409 Nürnberg				http://www.suse.de
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#3 lore raw
Quoted 6 lines · Hannes Reinecke
Quoted 3 lines

If scsi_probe_and_add_lun returns SCSI_SCAN_TARGET_PRESENT sdev isn't valid at this point. You should probably default to SCSI_2 for that case.

How so? This is exactly the behaviour I've removed. sdev is valid even for SCSI_SCAN_TARGET_PRESENT.
If not please show me.

Indeed, you replaced the check at the end of scsi_probe_and_add_lun. I don't think it makes a lot of sense to leave around the stale scsi_dev for PQ3, though.

Doing a REPORT_LUNS for SCSI_SCAN_TARGET_PRESENT does OTOH make a lot of sense, there's nothing in the spec against it. So what about dropping that device after we're done with REPORT_LUNS?

#4 lore raw
Quoted 10 lines · Christoph Hellwig
Quoted 6 lines · Hannes Reinecke
Quoted 3 lines

If scsi_probe_and_add_lun returns SCSI_SCAN_TARGET_PRESENT sdev isn't valid at this point. You should probably default to SCSI_2 for that case.

How so? This is exactly the behaviour I've removed. sdev is valid even for SCSI_SCAN_TARGET_PRESENT.
If not please show me.

Indeed, you replaced the check at the end of scsi_probe_and_add_lun. I don't think it makes a lot of sense to leave around the stale scsi_dev for PQ3, though.

Why not? sg connects to it and you can ask it to provide lots of useless information. Eg doing an report_luns by hand if the configuration of the SAN has changed; PQ3 only states that _this_ device will never have a LUN attached.

Quoted 5 lines

Doing a REPORT_LUNS for SCSI_SCAN_TARGET_PRESENT does OTOH make a lot of sense, there's nothing in the spec against it. So what about dropping that device after we're done with REPORT_LUNS?

See above. I'd like to have them around to be able to send commands to it.

Cheers,

Hannes

Signature · 8 lines
Dr. Hannes Reinecke			[email protected]
SuSE Linux AG				S390 & zSeries
Maxfeldstraße 5				+49 911 74053 688
90409 Nürnberg				http://www.suse.de
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#5 lore raw
Quoted 20 lines · Hannes Reinecke
Quoted 6 lines · Christoph Hellwig
Quoted 4 lines · Hannes Reinecke
Quoted 3 lines

If scsi_probe_and_add_lun returns SCSI_SCAN_TARGET_PRESENT sdev isn't valid at this point. You should probably default to SCSI_2 for that case.

Quoted 6 lines
Quoted 3 lines

How so? This is exactly the behaviour I've removed. sdev is valid even for SCSI_SCAN_TARGET_PRESENT.
If not please show me.

Indeed, you replaced the check at the end of scsi_probe_and_add_lun. I don't think it makes a lot of sense to leave around the stale scsi_dev for PQ3, though.

Why not? sg connects to it and you can ask it to provide lots of useless information. Eg doing an report_luns by hand if the configuration of the SAN has changed; PQ3 only states that _this_ device will never have a LUN attached.

Quoted 6 lines
Quoted 4 lines

Doing a REPORT_LUNS for SCSI_SCAN_TARGET_PRESENT does OTOH make a lot of sense, there's nothing in the spec against it. So what about dropping that device after we're done with REPORT_LUNS?

See above. I'd like to have them around to be able to send commands to it.

I like this (Hannes') patch. It makes those targets "addressable".

We do not necessarily want to care only about "live sdevs" -- a quiet SCSI target sitting in the domain should be addressable as Hannes points out. We should be able to send commands to it and get back responses as it may cater other services, not as apparent as a block device.

Luben