[PATCH scsi-misc-2.6] allow REPORT LUN scanning even for LUN 0 PQ of 3

[PATCH scsi-misc-2.6] allow REPORT LUN scanning even for LUN 0 PQ of 3

[Patrick Mansfield]

James -

Can you ack or comment?

Allow REPORT LUN scanning even if LUN 0 reports a peripheral qualifier of
3 (effectively no storage is available on LUN 0).

Similar patch I previously posted:

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

There is also Hannes patch, it leaves LUN 0 available for use via user
space, for compatibility it likely requires that it stay visible in the
future:

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

Hannes' patch also causes a lot of extra vSCSI devices to show up on some
platforms :)

Signed-off-by: Patrick Mansfield <patmans@us.ibm.com>

diff -uprN -X /home/patman/dontdiff scsi-misc-2.6.git/drivers/scsi/scsi_scan.c lun0-scsi-misc-2.6.git/drivers/scsi/scsi_scan.c
--- scsi-misc-2.6.git/drivers/scsi/scsi_scan.c	2005-08-16 15:02:20.000000000 -0700
+++ lun0-scsi-misc-2.6.git/drivers/scsi/scsi_scan.c	2005-08-30 14:59:15.000000000 -0700
@@ -64,14 +64,14 @@
  * 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
+ * SCSI_SCAN_LUN_IGNORED: target responded, but no device is available
  * on the given LUN.
  *
  * SCSI_SCAN_LUN_PRESENT: target responded, and a device is available on a
  * given LUN.
  */
 #define SCSI_SCAN_NO_RESPONSE		0
-#define SCSI_SCAN_TARGET_PRESENT	1
+#define SCSI_SCAN_LUN_IGNORED		1
 #define SCSI_SCAN_LUN_PRESENT		2
 
 static char *scsi_null_device_strs = "nullnullnullnull";
@@ -578,7 +578,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
+	 * device is attached at LUN 0 (SCSI_SCAN_LUN_IGNORED) so
 	 * non-zero LUNs can be scanned.
 	 */
 	sdev->scsi_level = inq_result[2] & 0x07;
@@ -769,6 +769,19 @@ static int scsi_add_lun(struct scsi_devi
 	return SCSI_SCAN_LUN_PRESENT;
 }
 
+/*
+ * scsi_scan_remove: Helper funciton to free up sdev's that are not added
+ * as sys devices. That is, we never call scsi_probe_and_add_lun for these
+ * sdev's.
+ */
+static void scsi_scan_remove(struct scsi_device *sdev)
+{
+	if (sdev->host->hostt->slave_destroy)
+		sdev->host->hostt->slave_destroy(sdev);
+	transport_destroy_device(&sdev->sdev_gendev);
+	put_device(&sdev->sdev_gendev);
+}
+
 /**
  * scsi_probe_and_add_lun - probe a LUN, if a LUN is found add it
  * @starget:	pointer to target device structure
@@ -783,8 +796,10 @@ 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_IGNORED: target responded, but no device is
+ *         attached at the LUN. The sdev is never made visible, and must
+ *         be freed by the caller via scsi_scan_remove(). This way, LUN 0
+ *         is avaiable for use with the REPORT LUN command.
  *     SCSI_SCAN_LUN_PRESENT: a new Scsi_Device was allocated and initialized
  **/
 static int scsi_probe_and_add_lun(struct scsi_target *starget,
@@ -853,7 +868,7 @@ static int scsi_probe_and_add_lun(struct
 		SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO
 					"scsi scan: peripheral qualifier of 3,"
 					" no device added\n"));
-		res = SCSI_SCAN_TARGET_PRESENT;
+		res = SCSI_SCAN_LUN_IGNORED;
 		goto out_free_result;
 	}
 
@@ -872,17 +887,13 @@ 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_LUN_PRESENT) || (res == SCSI_SCAN_LUN_IGNORED)) {
 		if (sdevp) {
 			scsi_device_get(sdev);
 			*sdevp = sdev;
 		}
-	} else {
-		if (sdev->host->hostt->slave_destroy)
-			sdev->host->hostt->slave_destroy(sdev);
-		transport_destroy_device(&sdev->sdev_gendev);
-		put_device(&sdev->sdev_gendev);
-	}
+	} else
+		scsi_scan_remove(sdev);
  out:
 	return res;
 }
@@ -1228,7 +1239,8 @@ static int scsi_report_lun_scan(struct s
 				       " from %s lun %d while scanning, scan"
 				       " aborted\n", devname, lun);
 				break;
-			}
+			} else if (res == SCSI_SCAN_LUN_IGNORED) 
+				scsi_scan_remove(sdev);
 		}
 	}
 
@@ -1262,6 +1274,8 @@ struct scsi_device *__scsi_add_device(st
 	if (scsi_host_scan_allowed(shost)) {
 		res = scsi_probe_and_add_lun(starget, lun, NULL, &sdev, 1,
 					     hostdata);
+		if (res == SCSI_SCAN_LUN_IGNORED)
+			scsi_scan_remove(sdev);
 		if (res != SCSI_SCAN_LUN_PRESENT)
 			sdev = ERR_PTR(-ENODEV);
 	}
@@ -1334,7 +1348,8 @@ void scsi_scan_target(struct device *par
 		/*
 		 * Scan for a specific host/chan/id/lun.
 		 */
-		scsi_probe_and_add_lun(starget, lun, NULL, NULL, rescan, NULL);
+		res = scsi_probe_and_add_lun(starget, lun, NULL, &sdev,
+					     rescan, NULL);
 		goto out_reap;
 	}
 
@@ -1342,8 +1357,10 @@ void scsi_scan_target(struct device *par
 	 * Scan LUN 0, if there is some response, scan further. Ideally, we
 	 * 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) {
+	res = scsi_probe_and_add_lun(starget, 0, &bflags, &sdev, rescan,
+				     NULL);
+	WARN_ON(!sdev && res != SCSI_SCAN_NO_RESPONSE);
+	if (sdev && res != SCSI_SCAN_NO_RESPONSE)
 		if (scsi_report_lun_scan(sdev, bflags, rescan) != 0)
 			/*
 			 * The REPORT LUN did not scan the target,
@@ -1351,20 +1368,12 @@ void scsi_scan_target(struct device *par
 			 */
 			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);
 
  out_reap:
+	if (res == SCSI_SCAN_LUN_IGNORED)
+		scsi_scan_remove(sdev);
 	/* now determine if the target has any children at all
 	 * and if not, nuke it */
 	scsi_target_reap(starget);
@@ -1540,4 +1549,3 @@ void scsi_free_host_dev(struct scsi_devi
 	put_device(&sdev->sdev_gendev);
 }
 EXPORT_SYMBOL(scsi_free_host_dev);
-

[PATCH scsi-misc-2.6] scsi_debug testing patch, return LUN 0 with PQ 3

[Patrick Mansfield]

Just a patch for scanning with PQ == 3 for LUN 0, only for use in testing
previous patch, don't apply.

diff -uprN -X /home/patman/dontdiff scsi-misc-2.6/drivers/scsi/scsi_debug.c lun0-replun-scsi-misc-2.6/drivers/scsi/scsi_debug.c
--- scsi-misc-2.6/drivers/scsi/scsi_debug.c	2005-07-26 11:08:51.000000000 -0700
+++ lun0-replun-scsi-misc-2.6/drivers/scsi/scsi_debug.c	2005-07-27 18:24:05.000000000 -0700
@@ -619,7 +619,10 @@ static int resp_inquiry(struct scsi_cmnd
 
 	alloc_len = (cmd[3] << 8) + cmd[4];
 	memset(arr, 0, SDEBUG_MAX_INQ_ARR_SZ);
-	pq_pdt = (scsi_debug_ptype & 0x1f);
+	if (devip->lun == 0)
+		pq_pdt = 0x7f;
+	else
+		pq_pdt = (scsi_debug_ptype & 0x1f);
 	arr[0] = pq_pdt;
 	if (0x2 & cmd[1]) {  /* CMDDT bit set */
 		mk_sense_buffer(devip, ILLEGAL_REQUEST, INVALID_FIELD_IN_CDB,

Re: [PATCH scsi-misc-2.6] allow REPORT LUN scanning even for LUN 0 PQ of 3

[Hannes Reinecke]

Patrick Mansfield wrote:
> James -
> 
> Can you ack or comment?
> 
> Allow REPORT LUN scanning even if LUN 0 reports a peripheral qualifier of
> 3 (effectively no storage is available on LUN 0).
> 
> Similar patch I previously posted:
> 
> http://marc.theaimsgroup.com/?l=linux-scsi&m=110297733824960&w=2
> 
> There is also Hannes patch, it leaves LUN 0 available for use via user
> space, for compatibility it likely requires that it stay visible in the
> future:
> 
> http://marc.theaimsgroup.com/?l=linux-scsi&m=111692497200507&w=2
> 
> Hannes' patch also causes a lot of extra vSCSI devices to show up on some
> platforms :)
> 
We definitely need something like this. The current scan algorithms is
just plain broken for targets where LUN 0 has no device attached.
It is perfectly valid for storage servers to have a LUN 0 with no device
attached (say, for control purposes) and let the client figure out the
current configuration via REPORT LUN.
Without this patch Linux simply cannot handle such setups.

James, do consider to include this patch into scsi-misc.


> Signed-off-by: Patrick Mansfield <patmans@us.ibm.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke			hare@suse.de
SuSE Linux Products GmbH		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 scsi-misc-2.6] allow REPORT LUN scanning even for LUN 0 PQ of 3

[James Bottomley]

On Tue, 2005-08-30 at 15:43 -0700, Patrick Mansfield wrote:
> James -
> 
> Can you ack or comment?
> 
> Allow REPORT LUN scanning even if LUN 0 reports a peripheral qualifier of
> 3 (effectively no storage is available on LUN 0).
> 
> Similar patch I previously posted:
> 
> http://marc.theaimsgroup.com/?l=linux-scsi&m=110297733824960&w=2
> 
> There is also Hannes patch, it leaves LUN 0 available for use via user
> space, for compatibility it likely requires that it stay visible in the
> future:
> 
> http://marc.theaimsgroup.com/?l=linux-scsi&m=111692497200507&w=2
> 
> Hannes' patch also causes a lot of extra vSCSI devices to show up on some
> platforms :)

Sorry, I didn't really like either patch.

The problem, essentially is this rather evil return refcounted sdev
under certain circumstances.  What I didn't like about your patch was
that you actually extended it to be under more circumstances.

The correct fix, I think, is to make scsi_report_lun_scan() actually
work on the target device; this will also set us up nicely for WLUN
scans if they prove necessary.  This being well into -rc for linux, I'll
resist the strong urge to take the hatchet to the sdev returning code
and stomach the duplication of scsi_level and resist the urge to
investigate dumping the rescan parameter ... they can all be fixed
properly after 2.6.14.

How does the attached look (and more importantly, does it work)?

James

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -587,6 +587,7 @@ static int scsi_probe_lun(struct scsi_de
 	if (sdev->scsi_level >= 2 ||
 	    (sdev->scsi_level == 1 && (inq_result[3] & 0x0f) == 1))
 		sdev->scsi_level++;
+	sdev->sdev_target->scsi_level = sdev->scsi_level;
 
 	return 0;
 }
@@ -771,6 +772,15 @@ static int scsi_add_lun(struct scsi_devi
 	return SCSI_SCAN_LUN_PRESENT;
 }
 
+static inline void scsi_destroy_sdev(struct scsi_device *sdev)
+{
+	if (sdev->host->hostt->slave_destroy)
+		sdev->host->hostt->slave_destroy(sdev);
+	transport_destroy_device(&sdev->sdev_gendev);
+	put_device(&sdev->sdev_gendev);
+}
+
+
 /**
  * scsi_probe_and_add_lun - probe a LUN, if a LUN is found add it
  * @starget:	pointer to target device structure
@@ -803,9 +813,9 @@ static int scsi_probe_and_add_lun(struct
 	 * The rescan flag is used as an optimization, the first scan of a
 	 * host adapter calls into here with rescan == 0.
 	 */
-	if (rescan) {
-		sdev = scsi_device_lookup_by_target(starget, lun);
-		if (sdev) {
+	sdev = scsi_device_lookup_by_target(starget, lun);
+	if (sdev) {
+		if (rescan || sdev->sdev_state != SDEV_CREATED) {
 			SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO
 				"scsi scan: device exists on %s\n",
 				sdev->sdev_gendev.bus_id));
@@ -820,12 +830,15 @@ static int scsi_probe_and_add_lun(struct
 								 sdev->model);
 			return SCSI_SCAN_LUN_PRESENT;
 		}
-	}
-
-	sdev = scsi_alloc_sdev(starget, lun, hostdata);
+	} else
+		sdev = scsi_alloc_sdev(starget, lun, hostdata);
 	if (!sdev)
 		goto out;
 
+	BUG_ON(sdev->sdev_state == SDEV_RUNNING ||
+	       sdev->sdev_state == SDEV_QUIESCE ||
+	       sdev->sdev_state == SDEV_BLOCK);
+
 	result = kmalloc(result_len, GFP_ATOMIC |
 			((shost->unchecked_isa_dma) ? __GFP_DMA : 0));
 	if (!result)
@@ -877,12 +890,8 @@ static int scsi_probe_and_add_lun(struct
 				res = SCSI_SCAN_NO_RESPONSE;
 			}
 		}
-	} else {
-		if (sdev->host->hostt->slave_destroy)
-			sdev->host->hostt->slave_destroy(sdev);
-		transport_destroy_device(&sdev->sdev_gendev);
-		put_device(&sdev->sdev_gendev);
-	}
+	} else
+		scsi_destroy_sdev(sdev);
  out:
 	return res;
 }
@@ -1054,7 +1063,7 @@ EXPORT_SYMBOL(int_to_scsilun);
  *     0: scan completed (or no memory, so further scanning is futile)
  *     1: no report lun scan, or not configured
  **/
-static int scsi_report_lun_scan(struct scsi_device *sdev, int bflags,
+static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 				int rescan)
 {
 	char devname[64];
@@ -1067,7 +1076,8 @@ static int scsi_report_lun_scan(struct s
 	struct scsi_lun *lunp, *lun_data;
 	u8 *data;
 	struct scsi_sense_hdr sshdr;
-	struct scsi_target *starget = scsi_target(sdev);
+	struct scsi_device *sdev;
+	struct Scsi_Host *shost = dev_to_shost(&starget->dev);
 
 	/*
 	 * Only support SCSI-3 and up devices if BLIST_NOREPORTLUN is not set.
@@ -1075,15 +1085,23 @@ static int scsi_report_lun_scan(struct s
 	 * support more than 8 LUNs.
 	 */
 	if ((bflags & BLIST_NOREPORTLUN) || 
-	     sdev->scsi_level < SCSI_2 ||
-	    (sdev->scsi_level < SCSI_3 && 
-	     (!(bflags & BLIST_REPORTLUN2) || sdev->host->max_lun <= 8)) )
+	     starget->scsi_level < SCSI_2 ||
+	    (starget->scsi_level < SCSI_3 && 
+	     (!(bflags & BLIST_REPORTLUN2) || shost->max_lun <= 8)) )
 		return 1;
 	if (bflags & BLIST_NOLUN)
 		return 0;
 
+	if (!(sdev = scsi_device_lookup_by_target(starget, 0))) {
+		sdev = scsi_alloc_sdev(starget, 0, NULL);
+		if (!sdev)
+			return 0;
+		if (!scsi_device_get(sdev))
+			return 0;
+	}
+
 	sprintf(devname, "host %d channel %d id %d",
-		sdev->host->host_no, sdev->channel, sdev->id);
+		shost->host_no, sdev->channel, sdev->id);
 
 	/*
 	 * Allocate enough to hold the header (the same size as one scsi_lun)
@@ -1098,8 +1116,10 @@ static int scsi_report_lun_scan(struct s
 	length = (max_scsi_report_luns + 1) * sizeof(struct scsi_lun);
 	lun_data = kmalloc(length, GFP_ATOMIC |
 			   (sdev->host->unchecked_isa_dma ? __GFP_DMA : 0));
-	if (!lun_data)
+	if (!lun_data) {
+		printk(ALLOC_FAILURE_MSG, __FUNCTION__);
 		goto out;
+	}
 
 	scsi_cmd[0] = REPORT_LUNS;
 
@@ -1201,10 +1221,6 @@ static int scsi_report_lun_scan(struct s
 			for (i = 0; i < sizeof(struct scsi_lun); i++)
 				printk("%02x", data[i]);
 			printk(" has a LUN larger than currently supported.\n");
-		} else if (lun == 0) {
-			/*
-			 * LUN 0 has already been scanned.
-			 */
 		} else if (lun > sdev->host->max_lun) {
 			printk(KERN_WARNING "scsi: %s lun%d has a LUN larger"
 			       " than allowed by the host adapter\n",
@@ -1227,13 +1243,13 @@ static int scsi_report_lun_scan(struct s
 	}
 
 	kfree(lun_data);
-	return 0;
-
  out:
-	/*
-	 * We are out of memory, don't try scanning any further.
-	 */
-	printk(ALLOC_FAILURE_MSG, __FUNCTION__);
+	scsi_device_put(sdev);
+	if (sdev->sdev_state == SDEV_CREATED)
+		/*
+		 * the sdev we used didn't appear in the report luns scan
+		 */
+		scsi_destroy_sdev(sdev);
 	return 0;
 }
 
@@ -1326,23 +1342,14 @@ static void __scsi_scan_target(struct de
 	 * 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_LUN_PRESENT || res == SCSI_SCAN_TARGET_PRESENT) {
+		if (scsi_report_lun_scan(starget, bflags, rescan) != 0)
 			/*
 			 * The REPORT LUN did not scan the target,
 			 * do a sequential scan.
 			 */
 			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);
+				       	res, starget->scsi_level, rescan);
 	}
 	if (sdev)
 		scsi_device_put(sdev);
@@ -1542,10 +1549,7 @@ void scsi_free_host_dev(struct scsi_devi
 {
 	BUG_ON(sdev->id != sdev->host->this_id);
 
-	if (sdev->host->hostt->slave_destroy)
-		sdev->host->hostt->slave_destroy(sdev);
-	transport_destroy_device(&sdev->sdev_gendev);
-	put_device(&sdev->sdev_gendev);
+	scsi_destroy_sdev(sdev);
 }
 EXPORT_SYMBOL(scsi_free_host_dev);
 
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -163,6 +163,7 @@ struct scsi_target {
 	unsigned int		id; /* target id ... replace
 				     * scsi_device.id eventually */
 	unsigned long		create:1; /* signal that it needs to be added */
+	char			scsi_level;
 	void 			*hostdata; /* available to low-level driver */
 	unsigned long		starget_data[0]; /* for the transport */
 	/* starget_data must be the last element!!!! */

Re: [PATCH scsi-misc-2.6] allow REPORT LUN scanning even for LUN 0 PQ of 3

[Patrick Mansfield]

Hi James -

On Tue, Sep 20, 2005 at 10:11:10PM -0500, James Bottomley wrote:
> Sorry, I didn't really like either patch.

OK :-/

> The problem, essentially is this rather evil return refcounted sdev
> under certain circumstances.  What I didn't like about your patch was
> that you actually extended it to be under more circumstances.

I don't follow the "certain circumstances", but OK.

> The correct fix, I think, is to make scsi_report_lun_scan() actually
> work on the target device; this will also set us up nicely for WLUN
> scans if they prove necessary.  This being well into -rc for linux, I'll

Yes, I agree. I was forward porting the patch from some time ago, and
didn't think the starget was far enough along.

> resist the strong urge to take the hatchet to the sdev returning code
> and stomach the duplication of scsi_level and resist the urge to
> investigate dumping the rescan parameter ... they can all be fixed
> properly after 2.6.14.

Yes for scsi_level. 

With your patch, rescan could go away, but I do not like the lookup for
each LU when we first scan a target (I think O(n^2)? i.e. for many LUs,
1000 + 999 + ... + 1 and except for LUN 0, none will match). Long term we
could avoid this, maybe still special case LUN 0 in scsi_report_lun_scan();
we could also auto-set the rescan setting (and not externalize it) when
there no LU's on a target (or host) at the start

> How does the attached look (and more importantly, does it work)?

I tried it on top of scsi-rc-fixes, it oopsed (non-scsi) on ppc/JS20, so I
patched on a 2.6.14-rc2 git tree from sept 21, that boots up OK on IDE.

But the module refcount stays non-zero for scsi-debug, probably
scsi_device_put/get. I am still debugging ...

Did your try it with the scsi_debug patch? And multiple insmod/rmmod of
it?

This chunk (and "insmod scsi_debug.ko max_luns=5" or such):

--- scsi-misc-2.6/drivers/scsi/scsi_debug.c	2005-07-26 11:08:51.000000000 -0700
+++ lun0-replun-scsi-misc-2.6/drivers/scsi/scsi_debug.c	2005-07-27 18:24:05.000000000 -0700
@@ -619,7 +619,10 @@ static int resp_inquiry(struct scsi_cmnd
 
 	alloc_len = (cmd[3] << 8) + cmd[4];
 	memset(arr, 0, SDEBUG_MAX_INQ_ARR_SZ);
-	pq_pdt = (scsi_debug_ptype & 0x1f);
+	if (devip->lun == 0)
+		pq_pdt = 0x7f;
+	else
+		pq_pdt = (scsi_debug_ptype & 0x1f);
 	arr[0] = pq_pdt;
 	if (0x2 & cmd[1]) {  /* CMDDT bit set */
 		mk_sense_buffer(devip, ILLEGAL_REQUEST, INVALID_FIELD_IN_CDB,

-- Patrick Mansfield

Re: [PATCH scsi-misc-2.6] allow REPORT LUN scanning even for LUN 0 PQ of 3

[Patrick Mansfield]

On Wed, Sep 21, 2005 at 03:15:00PM -0700, Patrick Mansfield wrote:

> But the module refcount stays non-zero for scsi-debug, probably
> scsi_device_put/get. I am still debugging ...

I forgot to mention no scsi_debug devices showed up.

I removed the bad "!" on !scsi_device_get(), and the "insmod ./scsi_debug.ko
max_luns=3' found 2 sd's as expected.

But, scsi_debug module count is still at 1. Removing (via delete attr) the
2 scsi_debug devices, and module count is still at 1. So probably still a
missing scsi_device_put(). I'm trying to debug a bit more.

-- Patrick Mansfield

Re: [PATCH scsi-misc-2.6] allow REPORT LUN scanning even for LUN 0 PQ of 3

[James Bottomley]

On Wed, 2005-09-21 at 17:10 -0700, Patrick Mansfield wrote:
> I removed the bad "!" on !scsi_device_get(), and the "insmod ./scsi_debug.ko
> max_luns=3' found 2 sd's as expected.

Fixed it.

> But, scsi_debug module count is still at 1. Removing (via delete attr) the
> 2 scsi_debug devices, and module count is still at 1. So probably still a
> missing scsi_device_put(). I'm trying to debug a bit more.

Found it and fixed that too ... missing a put after the find_device.

James

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -587,6 +587,7 @@ static int scsi_probe_lun(struct scsi_de
 	if (sdev->scsi_level >= 2 ||
 	    (sdev->scsi_level == 1 && (inq_result[3] & 0x0f) == 1))
 		sdev->scsi_level++;
+	sdev->sdev_target->scsi_level = sdev->scsi_level;
 
 	return 0;
 }
@@ -771,6 +772,15 @@ static int scsi_add_lun(struct scsi_devi
 	return SCSI_SCAN_LUN_PRESENT;
 }
 
+static inline void scsi_destroy_sdev(struct scsi_device *sdev)
+{
+	if (sdev->host->hostt->slave_destroy)
+		sdev->host->hostt->slave_destroy(sdev);
+	transport_destroy_device(&sdev->sdev_gendev);
+	put_device(&sdev->sdev_gendev);
+}
+
+
 /**
  * scsi_probe_and_add_lun - probe a LUN, if a LUN is found add it
  * @starget:	pointer to target device structure
@@ -803,9 +813,9 @@ static int scsi_probe_and_add_lun(struct
 	 * The rescan flag is used as an optimization, the first scan of a
 	 * host adapter calls into here with rescan == 0.
 	 */
-	if (rescan) {
-		sdev = scsi_device_lookup_by_target(starget, lun);
-		if (sdev) {
+	sdev = scsi_device_lookup_by_target(starget, lun);
+	if (sdev) {
+		if (rescan || sdev->sdev_state != SDEV_CREATED) {
 			SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO
 				"scsi scan: device exists on %s\n",
 				sdev->sdev_gendev.bus_id));
@@ -820,9 +830,9 @@ static int scsi_probe_and_add_lun(struct
 								 sdev->model);
 			return SCSI_SCAN_LUN_PRESENT;
 		}
-	}
-
-	sdev = scsi_alloc_sdev(starget, lun, hostdata);
+		scsi_device_put(sdev);
+	} else
+		sdev = scsi_alloc_sdev(starget, lun, hostdata);
 	if (!sdev)
 		goto out;
 
@@ -877,12 +887,8 @@ static int scsi_probe_and_add_lun(struct
 				res = SCSI_SCAN_NO_RESPONSE;
 			}
 		}
-	} else {
-		if (sdev->host->hostt->slave_destroy)
-			sdev->host->hostt->slave_destroy(sdev);
-		transport_destroy_device(&sdev->sdev_gendev);
-		put_device(&sdev->sdev_gendev);
-	}
+	} else
+		scsi_destroy_sdev(sdev);
  out:
 	return res;
 }
@@ -1054,7 +1060,7 @@ EXPORT_SYMBOL(int_to_scsilun);
  *     0: scan completed (or no memory, so further scanning is futile)
  *     1: no report lun scan, or not configured
  **/
-static int scsi_report_lun_scan(struct scsi_device *sdev, int bflags,
+static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 				int rescan)
 {
 	char devname[64];
@@ -1067,7 +1073,8 @@ static int scsi_report_lun_scan(struct s
 	struct scsi_lun *lunp, *lun_data;
 	u8 *data;
 	struct scsi_sense_hdr sshdr;
-	struct scsi_target *starget = scsi_target(sdev);
+	struct scsi_device *sdev;
+	struct Scsi_Host *shost = dev_to_shost(&starget->dev);
 
 	/*
 	 * Only support SCSI-3 and up devices if BLIST_NOREPORTLUN is not set.
@@ -1075,15 +1082,23 @@ static int scsi_report_lun_scan(struct s
 	 * support more than 8 LUNs.
 	 */
 	if ((bflags & BLIST_NOREPORTLUN) || 
-	     sdev->scsi_level < SCSI_2 ||
-	    (sdev->scsi_level < SCSI_3 && 
-	     (!(bflags & BLIST_REPORTLUN2) || sdev->host->max_lun <= 8)) )
+	     starget->scsi_level < SCSI_2 ||
+	    (starget->scsi_level < SCSI_3 && 
+	     (!(bflags & BLIST_REPORTLUN2) || shost->max_lun <= 8)) )
 		return 1;
 	if (bflags & BLIST_NOLUN)
 		return 0;
 
+	if (!(sdev = scsi_device_lookup_by_target(starget, 0))) {
+		sdev = scsi_alloc_sdev(starget, 0, NULL);
+		if (!sdev)
+			return 0;
+		if (scsi_device_get(sdev))
+			return 0;
+	}
+
 	sprintf(devname, "host %d channel %d id %d",
-		sdev->host->host_no, sdev->channel, sdev->id);
+		shost->host_no, sdev->channel, sdev->id);
 
 	/*
 	 * Allocate enough to hold the header (the same size as one scsi_lun)
@@ -1098,8 +1113,10 @@ static int scsi_report_lun_scan(struct s
 	length = (max_scsi_report_luns + 1) * sizeof(struct scsi_lun);
 	lun_data = kmalloc(length, GFP_ATOMIC |
 			   (sdev->host->unchecked_isa_dma ? __GFP_DMA : 0));
-	if (!lun_data)
+	if (!lun_data) {
+		printk(ALLOC_FAILURE_MSG, __FUNCTION__);
 		goto out;
+	}
 
 	scsi_cmd[0] = REPORT_LUNS;
 
@@ -1201,10 +1218,6 @@ static int scsi_report_lun_scan(struct s
 			for (i = 0; i < sizeof(struct scsi_lun); i++)
 				printk("%02x", data[i]);
 			printk(" has a LUN larger than currently supported.\n");
-		} else if (lun == 0) {
-			/*
-			 * LUN 0 has already been scanned.
-			 */
 		} else if (lun > sdev->host->max_lun) {
 			printk(KERN_WARNING "scsi: %s lun%d has a LUN larger"
 			       " than allowed by the host adapter\n",
@@ -1227,13 +1240,13 @@ static int scsi_report_lun_scan(struct s
 	}
 
 	kfree(lun_data);
-	return 0;
-
  out:
-	/*
-	 * We are out of memory, don't try scanning any further.
-	 */
-	printk(ALLOC_FAILURE_MSG, __FUNCTION__);
+	scsi_device_put(sdev);
+	if (sdev->sdev_state == SDEV_CREATED)
+		/*
+		 * the sdev we used didn't appear in the report luns scan
+		 */
+		scsi_destroy_sdev(sdev);
 	return 0;
 }
 
@@ -1299,7 +1312,6 @@ static void __scsi_scan_target(struct de
 	struct Scsi_Host *shost = dev_to_shost(parent);
 	int bflags = 0;
 	int res;
-	struct scsi_device *sdev = NULL;
 	struct scsi_target *starget;
 
 	if (shost->this_id == id)
@@ -1325,27 +1337,16 @@ static void __scsi_scan_target(struct de
 	 * Scan LUN 0, if there is some response, scan further. Ideally, we
 	 * 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)
+	res = scsi_probe_and_add_lun(starget, 0, &bflags, NULL, rescan, NULL);
+	if (res == SCSI_SCAN_LUN_PRESENT || res == SCSI_SCAN_TARGET_PRESENT) {
+		if (scsi_report_lun_scan(starget, bflags, rescan) != 0)
 			/*
 			 * The REPORT LUN did not scan the target,
 			 * do a sequential scan.
 			 */
 			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);
+				       	res, starget->scsi_level, rescan);
 	}
-	if (sdev)
-		scsi_device_put(sdev);
 
  out_reap:
 	/* now determine if the target has any children at all
@@ -1542,10 +1543,7 @@ void scsi_free_host_dev(struct scsi_devi
 {
 	BUG_ON(sdev->id != sdev->host->this_id);
 
-	if (sdev->host->hostt->slave_destroy)
-		sdev->host->hostt->slave_destroy(sdev);
-	transport_destroy_device(&sdev->sdev_gendev);
-	put_device(&sdev->sdev_gendev);
+	scsi_destroy_sdev(sdev);
 }
 EXPORT_SYMBOL(scsi_free_host_dev);
 
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -163,6 +163,7 @@ struct scsi_target {
 	unsigned int		id; /* target id ... replace
 				     * scsi_device.id eventually */
 	unsigned long		create:1; /* signal that it needs to be added */
+	char			scsi_level;
 	void 			*hostdata; /* available to low-level driver */
 	unsigned long		starget_data[0]; /* for the transport */
 	/* starget_data must be the last element!!!! */

Re: [PATCH scsi-misc-2.6] allow REPORT LUN scanning even for LUN 0 PQ of 3

[Patrick Mansfield]

On Thu, Sep 22, 2005 at 08:33:28PM -0500, James Bottomley wrote:
> On Wed, 2005-09-21 at 17:10 -0700, Patrick Mansfield wrote:
> > I removed the bad "!" on !scsi_device_get(), and the "insmod ./scsi_debug.ko
> > max_luns=3' found 2 sd's as expected.
> 
> Fixed it.
> 
> > But, scsi_debug module count is still at 1. Removing (via delete attr) the
> > 2 scsi_debug devices, and module count is still at 1. So probably still a
> > missing scsi_device_put(). I'm trying to debug a bit more.
> 
> Found it and fixed that too ... missing a put after the find_device.

OK, works fine now ...

> +static inline void scsi_destroy_sdev(struct scsi_device *sdev)
> +{
> +	if (sdev->host->hostt->slave_destroy)
> +		sdev->host->hostt->slave_destroy(sdev);
> +	transport_destroy_device(&sdev->sdev_gendev);
> +	put_device(&sdev->sdev_gendev);

One nit ... get rid of the inline, as the code path is not performance
critical.

Also, note that most devices won't behave as scsi_debug configured as
previosly patched: it had PQ !=0 for LUN 0, but then put LUN 0 in REPORT
LUN output.

So, we hit the already existing sdev case in James' patch. 

But it was a good test case :)

For the record, here's a scsi_debug patch to not have LUN 0 show up, and
not have it in REPORT LUNS output.

--- linux-2.6.14-rc2-git1/drivers/scsi/scsi_debug.c	2005-08-28 17:51:48.000000000 -0700
+++ jamesb-lun0-replun/drivers/scsi/scsi_debug.c	2005-09-23 15:33:51.000000000 -0700
@@ -619,7 +619,10 @@ static int resp_inquiry(struct scsi_cmnd
 
 	alloc_len = (cmd[3] << 8) + cmd[4];
 	memset(arr, 0, SDEBUG_MAX_INQ_ARR_SZ);
-	pq_pdt = (scsi_debug_ptype & 0x1f);
+	if (devip->lun == 0)
+		pq_pdt = 0x7f; /* XXX hack no LUN 0 */
+	else
+		pq_pdt = (scsi_debug_ptype & 0x1f);
 	arr[0] = pq_pdt;
 	if (0x2 & cmd[1]) {  /* CMDDT bit set */
 		mk_sense_buffer(devip, ILLEGAL_REQUEST, INVALID_FIELD_IN_CDB,
@@ -946,7 +949,7 @@ static int resp_report_luns(struct scsi_
 			    struct sdebug_dev_info * devip)
 {
 	unsigned int alloc_len;
-	int lun_cnt, i, upper;
+	int lun_cnt, i, upper, lun;
 	unsigned char *cmd = (unsigned char *)scp->cmnd;
 	int select_report = (int)cmd[2];
 	struct scsi_lun *one_lun;
@@ -960,18 +963,18 @@ static int resp_report_luns(struct scsi_
 	}
 	/* can produce response with up to 16k luns (lun 0 to lun 16383) */
 	memset(arr, 0, SDEBUG_RLUN_ARR_SZ);
-	lun_cnt = scsi_debug_max_luns;
+	lun_cnt = scsi_debug_max_luns - 1;
 	arr[2] = ((sizeof(struct scsi_lun) * lun_cnt) >> 8) & 0xff;
 	arr[3] = (sizeof(struct scsi_lun) * lun_cnt) & 0xff;
 	lun_cnt = min((int)((SDEBUG_RLUN_ARR_SZ - 8) /
 			    sizeof(struct scsi_lun)), lun_cnt);
 	one_lun = (struct scsi_lun *) &arr[8];
-	for (i = 0; i < lun_cnt; i++) {
-		upper = (i >> 8) & 0x3f;
+	for (i = 0, lun = 1; i < lun_cnt; i++, lun++) {
+		upper = (lun >> 8) & 0x3f;
 		if (upper)
 			one_lun[i].scsi_lun[0] =
 			    (upper | (SAM2_LUN_ADDRESS_METHOD << 6));
-		one_lun[i].scsi_lun[1] = i & 0xff;
+		one_lun[i].scsi_lun[1] = lun & 0xff;
 	}
 	return fill_from_dev_buffer(scp, arr,
 				    min((int)alloc_len, SDEBUG_RLUN_ARR_SZ));

-- Patrick Mansfield