[PATCH] Sanitize PQ3 device handling

[PATCH] Sanitize PQ3 device handling

[Hannes Reinecke]

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

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
-- 
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: 4715 bytes --]

diff -pur linux-2.6.12-rc4.orig/drivers/scsi/scsi_scan.c linux-2.6.12-rc4/drivers/scsi/scsi_scan.c
--- 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-18 11:29:47.000000000 +0200
@@ -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

Re: [PATCH] Sanitize PQ3 device handling

[Christoph Hellwig]

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

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.

Re: [PATCH] Sanitize PQ3 device handling

[Hannes Reinecke]

Christoph Hellwig wrote:
>> 	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);
> 
> 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
-- 
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
-
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] Sanitize PQ3 device handling

[Christoph Hellwig]

On Wed, May 18, 2005 at 04:05:36PM +0200, Hannes Reinecke wrote:
> > 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?

Re: [PATCH] Sanitize PQ3 device handling

[Hannes Reinecke]

Christoph Hellwig wrote:
> On Wed, May 18, 2005 at 04:05:36PM +0200, Hannes Reinecke wrote:
>>>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.
> 
> 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
-- 
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
-
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] Sanitize PQ3 device handling

[Luben Tuikov]

On 05/18/05 10:33, Hannes Reinecke wrote:
> Christoph Hellwig wrote:
> 
>>On Wed, May 18, 2005 at 04:05:36PM +0200, Hannes Reinecke wrote:
>>
>>>>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.
> 
>>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