[PATCH] scsi_scan: Send TEST UNIT READY to LUN0 before LUN scanning

[PATCH] scsi_scan: Send TEST UNIT READY to LUN0 before LUN scanning

[Hannes Reinecke]

REPORT_LUN_SCAN does not report any outstanding unit attention
condition as per SAM. However, there are some target implementations
which might not be fully initialized by the time we're starting
the report lun scan.
For these targets we end up getting a default entry
(or even a partially filled one).
And as we're not able to process the REPORT LUN DATA HAS CHANGED
unit attention correctly we'll be missing out some LUNs altogether.

To handle these conditions properly I've added a new blacklist
flag 'BLIST_TEST_LUN0' which will send a TEST UNIT READY
and wait until the unit attention condition goes away.

We cannot make this the default as some other target
implementations rely on the scanning code to complete even though the
firmware might not be fully initialized.

Cc: Martthias Roessler <Matthias.Roessler@ts.fujitsu.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi_devinfo.c |  1 +
 drivers/scsi/scsi_scan.c    | 88 ++++++++++++++++++++++++++++++++++++++-------
 include/scsi/scsi_devinfo.h |  2 ++
 3 files changed, 78 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index 49014a1..4f446e3 100644
--- a/drivers/scsi/scsi_devinfo.c
+++ b/drivers/scsi/scsi_devinfo.c
@@ -166,6 +166,7 @@ static struct {
 	{"easyRAID", "X6P", NULL, BLIST_NOREPORTLUN},
 	{"easyRAID", "F8", NULL, BLIST_NOREPORTLUN},
 	{"FSC", "CentricStor", "*", BLIST_SPARSELUN | BLIST_LARGELUN},
+	{"FUJITSU", "ETERNUS_DX", "*", BLIST_TEST_LUN0 },
 	{"Generic", "USB SD Reader", "1.00", BLIST_FORCELUN | BLIST_INQUIRY_36},
 	{"Generic", "USB Storage-SMC", "0180", BLIST_FORCELUN | BLIST_INQUIRY_36},
 	{"Generic", "USB Storage-SMC", "0207", BLIST_FORCELUN | BLIST_INQUIRY_36},
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 0af7133..ca39e32 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -119,6 +119,13 @@ MODULE_PARM_DESC(inq_timeout,
 		 "Timeout (in seconds) waiting for devices to answer INQUIRY."
 		 " Default is 20. Some devices may need more; most need less.");
 
+static unsigned int scsi_scan_timeout = SCSI_TIMEOUT/HZ + 58;
+
+module_param_named(scan_timeout, scsi_scan_timeout, uint, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(scan_timeout,
+		 "Timeout (in seconds) waiting for devices to become ready"
+		 " after INQUIRY. Default is 60.");
+
 /* This lock protects only this list */
 static DEFINE_SPINLOCK(async_scan_lock);
 static LIST_HEAD(scanning_hosts);
@@ -719,19 +726,6 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	}
 
 	/*
-	 * Related to the above issue:
-	 *
-	 * XXX Devices (disk or all?) should be sent a TEST UNIT READY,
-	 * and if not ready, sent a START_STOP to start (maybe spin up) and
-	 * then send the INQUIRY again, since the INQUIRY can change after
-	 * a device is initialized.
-	 *
-	 * Ideally, start a device if explicitly asked to do so.  This
-	 * assumes that a device is spun up on power on, spun down on
-	 * request, and then spun up on request.
-	 */
-
-	/*
 	 * 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.
@@ -756,6 +750,65 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 }
 
 /**
+ * scsi_test_lun - waiting for a LUN to become ready
+ * @sdev:	scsi_device to test
+ *
+ * Description:
+ *     Wait for the lun associated with @sdev to become ready
+ *
+ *     Send a TEST UNIT READY to detect any unit attention conditions.
+ *     Retry TEST UNIT READY for up to @scsi_scan_timeout if the
+ *     returned sense key is 02/04/01 (Not ready, Logical Unit is
+ *     in process of becoming ready)
+ **/
+static int
+scsi_test_lun(struct scsi_device *sdev)
+{
+	struct scsi_sense_hdr sshdr;
+	int res = SCSI_SCAN_TARGET_PRESENT;
+	int tur_result;
+	unsigned long tur_timeout = jiffies + scsi_scan_timeout * HZ;
+
+	/* Skip for older devices */
+	if (sdev->scsi_level <= SCSI_3)
+		return SCSI_SCAN_LUN_PRESENT;
+
+	/*
+	 * Wait for the device to become ready.
+	 *
+	 * Some targets take some time before the firmware is
+	 * fully initialized, during which time they might not
+	 * be able to fill out any REPORT_LUN command correctly.
+	 * And as we're not capable of handling the
+	 * INQUIRY DATA CHANGED unit attention correctly we'd
+	 * rather wait here.
+	 */
+	do {
+		tur_result = scsi_test_unit_ready(sdev, SCSI_TIMEOUT,
+							  3, &sshdr);
+		if (!tur_result) {
+			res = SCSI_SCAN_LUN_PRESENT;
+			break;
+		}
+		if ((driver_byte(tur_result) & DRIVER_SENSE) &&
+		    scsi_sense_valid(&sshdr)) {
+			SCSI_LOG_SCAN_BUS(3, sdev_printk(KERN_INFO, sdev,
+				"scsi_scan: tur returned %02x/%02x/%02x\n",
+				sshdr.sense_key, sshdr.asc, sshdr.ascq));
+			if (sshdr.sense_key == NOT_READY &&
+			    sshdr.asc == 0x04 && sshdr.ascq == 0x01) {
+				/* Logical Unit is in process
+				 * of becoming ready */
+				msleep(100);
+				continue;
+			}
+		}
+		res = SCSI_SCAN_LUN_PRESENT;
+	} while (time_before_eq(jiffies, tur_timeout));
+	return res;
+}
+
+/**
  * scsi_add_lun - allocate and fully initialze a scsi_device
  * @sdev:	holds information to be stored in the new scsi_device
  * @inq_result:	holds the result of a previous INQUIRY to the LUN
@@ -1159,6 +1212,15 @@ static int scsi_probe_and_add_lun(struct scsi_target *starget,
 		goto out_free_result;
 	}
 
+	if (bflags & BLIST_TEST_LUN0) {
+		res = scsi_test_lun(sdev);
+		if (res == SCSI_SCAN_TARGET_PRESENT) {
+			SCSI_LOG_SCAN_BUS(1, sdev_printk(KERN_INFO, sdev,
+				"scsi scan: device not ready\n"));
+			goto out_free_result;
+		}
+	}
+
 	res = scsi_add_lun(sdev, result, &bflags, shost->async_scan);
 	if (res == SCSI_SCAN_LUN_PRESENT) {
 		if (bflags & BLIST_KEY) {
diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
index 183eaab..cf1887b 100644
--- a/include/scsi/scsi_devinfo.h
+++ b/include/scsi/scsi_devinfo.h
@@ -36,5 +36,7 @@
 					     for sequential scan */
 #define BLIST_TRY_VPD_PAGES	0x10000000 /* Attempt to read VPD pages */
 #define BLIST_NO_RSOC		0x20000000 /* don't try to issue RSOC */
+#define BLIST_TEST_LUN0		0x40000000 /* Send TEST UNIT READY to
+					    * LUN0 before scanning */
 
 #endif
-- 
1.8.4.5

RE: [PATCH] scsi_scan: Send TEST UNIT READY to LUN0 before LUN scanning

[Elliott, Robert (Server Storage)]

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Hannes Reinecke
> Sent: Thursday, 04 December, 2014 10:39 AM
...
>  /**
> + * scsi_test_lun - waiting for a LUN to become ready
> + * @sdev:	scsi_device to test
> + *
> + * Description:
> + *     Wait for the lun associated with @sdev to become ready
> + *
> + *     Send a TEST UNIT READY to detect any unit attention conditions.
> + *     Retry TEST UNIT READY for up to @scsi_scan_timeout if the
> + *     returned sense key is 02/04/01 (Not ready, Logical Unit is
> + *     in process of becoming ready)
> + **/
> +static int
> +scsi_test_lun(struct scsi_device *sdev)
> +{
> +	struct scsi_sense_hdr sshdr;
> +	int res = SCSI_SCAN_TARGET_PRESENT;
> +	int tur_result;
> +	unsigned long tur_timeout = jiffies + scsi_scan_timeout * HZ;
> +
> +	/* Skip for older devices */
> +	if (sdev->scsi_level <= SCSI_3)
> +		return SCSI_SCAN_LUN_PRESENT;
> +
> +	/*
> +	 * Wait for the device to become ready.
> +	 *
> +	 * Some targets take some time before the firmware is
> +	 * fully initialized, during which time they might not
> +	 * be able to fill out any REPORT_LUN command correctly.
> +	 * And as we're not capable of handling the
> +	 * INQUIRY DATA CHANGED unit attention correctly we'd
> +	 * rather wait here.
> +	 */
> +	do {
> +		tur_result = scsi_test_unit_ready(sdev, SCSI_TIMEOUT,
> +							  3, &sshdr);
> +		if (!tur_result) {
> +			res = SCSI_SCAN_LUN_PRESENT;
> +			break;
> +		}
> +		if ((driver_byte(tur_result) & DRIVER_SENSE) &&
> +		    scsi_sense_valid(&sshdr)) {
> +			SCSI_LOG_SCAN_BUS(3, sdev_printk(KERN_INFO, sdev,
> +				"scsi_scan: tur returned %02x/%02x/%02x\n",
> +				sshdr.sense_key, sshdr.asc, sshdr.ascq));
> +			if (sshdr.sense_key == NOT_READY &&
> +			    sshdr.asc == 0x04 && sshdr.ascq == 0x01) {
> +				/* Logical Unit is in process
> +				 * of becoming ready */
> +				msleep(100);
> +				continue;
> +			}
> +		}

In SAS, you may see these after power on or after sending a
START STOP UNIT command with START=1, as the controller or 
expander is performing staggered spinup:
04h/11h LOGICAL UNIT NOT READY, NOTIFY (ENABLE SPINUP) REQUIRED

Once the drive receives NOTIFY (ENABLE SPINUP), then it starts
reporting this until spinup is done:
04h/01h LOGICAL UNIT IS IN PROCESS OF BECOMING READY

If this function is intended for general "wait for ready" use, 
then it should sit through both codes.

If this function is only intended for drives that violate the
rule to always be capable of returning REPORT LUNS data 
indicating LUN 0, it's hard to guess if 04h/11h could also
appear.  

---
Rob Elliott    HP Server Storage

Re: [PATCH] scsi_scan: Send TEST UNIT READY to LUN0 before LUN scanning

[Sebastian Herbszt]

Hannes Reinecke wrote:
> diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
> index 49014a1..4f446e3 100644
> --- a/drivers/scsi/scsi_devinfo.c
> +++ b/drivers/scsi/scsi_devinfo.c
> @@ -166,6 +166,7 @@ static struct {
>  	{"easyRAID", "X6P", NULL, BLIST_NOREPORTLUN},
>  	{"easyRAID", "F8", NULL, BLIST_NOREPORTLUN},
>  	{"FSC", "CentricStor", "*", BLIST_SPARSELUN | BLIST_LARGELUN},
> +	{"FUJITSU", "ETERNUS_DX", "*", BLIST_TEST_LUN0 },
>  	{"Generic", "USB SD Reader", "1.00", BLIST_FORCELUN | BLIST_INQUIRY_36},
>  	{"Generic", "USB Storage-SMC", "0180", BLIST_FORCELUN | BLIST_INQUIRY_36},
>  	{"Generic", "USB Storage-SMC", "0207", BLIST_FORCELUN | BLIST_INQUIRY_36},

Does this apply to all ETERNUS DX models and generations?

Sebastian

Re: [PATCH] scsi_scan: Send TEST UNIT READY to LUN0 before LUN scanning

[James Bottomley]

On Thu, 2014-12-04 at 17:39 +0100, Hannes Reinecke wrote:
[...]
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 0af7133..ca39e32 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -119,6 +119,13 @@ MODULE_PARM_DESC(inq_timeout,
>  		 "Timeout (in seconds) waiting for devices to answer INQUIRY."
>  		 " Default is 20. Some devices may need more; most need less.");
>  
> +static unsigned int scsi_scan_timeout = SCSI_TIMEOUT/HZ + 58;
> +
> +module_param_named(scan_timeout, scsi_scan_timeout, uint, S_IRUGO|S_IWUSR);
> +MODULE_PARM_DESC(scan_timeout,
> +		 "Timeout (in seconds) waiting for devices to become ready"
> +		 " after INQUIRY. Default is 60.");
> +
>  /* This lock protects only this list */
>  static DEFINE_SPINLOCK(async_scan_lock);
>  static LIST_HEAD(scanning_hosts);
> @@ -719,19 +726,6 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
>  	}
>  
>  	/*
> -	 * Related to the above issue:
> -	 *
> -	 * XXX Devices (disk or all?) should be sent a TEST UNIT READY,
> -	 * and if not ready, sent a START_STOP to start (maybe spin up) and
> -	 * then send the INQUIRY again, since the INQUIRY can change after
> -	 * a device is initialized.

You're not sending a start_stop unit, so why is this being deleted?  Any
unstarted unit will still return initialization command required and may
or may not return lun data.

> -	 *
> -	 * Ideally, start a device if explicitly asked to do so.  This
> -	 * assumes that a device is spun up on power on, spun down on
> -	 * request, and then spun up on request.
> -	 */
> -
> -	/*
>  	 * 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.
> @@ -756,6 +750,65 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
>  }
>  
>  /**
> + * scsi_test_lun - waiting for a LUN to become ready
> + * @sdev:	scsi_device to test
> + *
> + * Description:
> + *     Wait for the lun associated with @sdev to become ready
> + *
> + *     Send a TEST UNIT READY to detect any unit attention conditions.
> + *     Retry TEST UNIT READY for up to @scsi_scan_timeout if the
> + *     returned sense key is 02/04/01 (Not ready, Logical Unit is
> + *     in process of becoming ready)
> + **/
> +static int
> +scsi_test_lun(struct scsi_device *sdev)
> +{
> +	struct scsi_sense_hdr sshdr;
> +	int res = SCSI_SCAN_TARGET_PRESENT;
> +	int tur_result;
> +	unsigned long tur_timeout = jiffies + scsi_scan_timeout * HZ;
> +
> +	/* Skip for older devices */
> +	if (sdev->scsi_level <= SCSI_3)
> +		return SCSI_SCAN_LUN_PRESENT;
> +
> +	/*
> +	 * Wait for the device to become ready.
> +	 *
> +	 * Some targets take some time before the firmware is
> +	 * fully initialized, during which time they might not
> +	 * be able to fill out any REPORT_LUN command correctly.
> +	 * And as we're not capable of handling the
> +	 * INQUIRY DATA CHANGED unit attention correctly we'd
> +	 * rather wait here.

Can't we just fix that?  All you need is rescan to re-read the inquiry
data.

> +	 */
> +	do {
> +		tur_result = scsi_test_unit_ready(sdev, SCSI_TIMEOUT,
> +							  3, &sshdr);
> +		if (!tur_result) {
> +			res = SCSI_SCAN_LUN_PRESENT;
> +			break;
> +		}
> +		if ((driver_byte(tur_result) & DRIVER_SENSE) &&
> +		    scsi_sense_valid(&sshdr)) {
> +			SCSI_LOG_SCAN_BUS(3, sdev_printk(KERN_INFO, sdev,
> +				"scsi_scan: tur returned %02x/%02x/%02x\n",
> +				sshdr.sense_key, sshdr.asc, sshdr.ascq));

If we're waiting 60s and coming in here every 100ms we'll get 600 of
these in the log ... that's going to drown out anything that came before
it.

> +			if (sshdr.sense_key == NOT_READY &&
> +			    sshdr.asc == 0x04 && sshdr.ascq == 0x01) {
> +				/* Logical Unit is in process
> +				 * of becoming ready */
> +				msleep(100);
> +				continue;
> +			}
> +		}
> +		res = SCSI_SCAN_LUN_PRESENT;

Aren't you missing a break here?  Otherwise how does an unexpected
return code from the test unit ready do anything other than loop around
dumping log messages until 60s have expired.

> +	} while (time_before_eq(jiffies, tur_timeout));
> +	return res;
> +}
> +
> +/**
>   * scsi_add_lun - allocate and fully initialze a scsi_device
>   * @sdev:	holds information to be stored in the new scsi_device
>   * @inq_result:	holds the result of a previous INQUIRY to the LUN
> @@ -1159,6 +1212,15 @@ static int scsi_probe_and_add_lun(struct scsi_target *starget,
>  		goto out_free_result;
>  	}
>  
> +	if (bflags & BLIST_TEST_LUN0) {
> +		res = scsi_test_lun(sdev);
> +		if (res == SCSI_SCAN_TARGET_PRESENT) {
> +			SCSI_LOG_SCAN_BUS(1, sdev_printk(KERN_INFO, sdev,
> +				"scsi scan: device not ready\n"));
> +			goto out_free_result;
> +		}
> +	}
> +
>  	res = scsi_add_lun(sdev, result, &bflags, shost->async_scan);

So any return other than SCSI_SCAN_TARGET_PRESENT gets thrown away.  Why
not just return true/false from the scsi_test_lun(sdev) and set res to
SCSI_SCAN_TARGET_PRESENT in the if clause if it fails?  That should
greatly simplify the logic inside scsi_test_lun().

James

Re: [PATCH] scsi_scan: Send TEST UNIT READY to LUN0 before LUN scanning

[Hannes Reinecke]

On 12/04/2014 09:00 PM, Elliott, Robert (Server Storage) wrote:
> 
> 
>> -----Original Message-----
>> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
>> owner@vger.kernel.org] On Behalf Of Hannes Reinecke
>> Sent: Thursday, 04 December, 2014 10:39 AM
> ...
>>  /**
>> + * scsi_test_lun - waiting for a LUN to become ready
>> + * @sdev:	scsi_device to test
>> + *
>> + * Description:
>> + *     Wait for the lun associated with @sdev to become ready
>> + *
>> + *     Send a TEST UNIT READY to detect any unit attention conditions.
>> + *     Retry TEST UNIT READY for up to @scsi_scan_timeout if the
>> + *     returned sense key is 02/04/01 (Not ready, Logical Unit is
>> + *     in process of becoming ready)
>> + **/
>> +static int
>> +scsi_test_lun(struct scsi_device *sdev)
>> +{
>> +	struct scsi_sense_hdr sshdr;
>> +	int res = SCSI_SCAN_TARGET_PRESENT;
>> +	int tur_result;
>> +	unsigned long tur_timeout = jiffies + scsi_scan_timeout * HZ;
>> +
>> +	/* Skip for older devices */
>> +	if (sdev->scsi_level <= SCSI_3)
>> +		return SCSI_SCAN_LUN_PRESENT;
>> +
>> +	/*
>> +	 * Wait for the device to become ready.
>> +	 *
>> +	 * Some targets take some time before the firmware is
>> +	 * fully initialized, during which time they might not
>> +	 * be able to fill out any REPORT_LUN command correctly.
>> +	 * And as we're not capable of handling the
>> +	 * INQUIRY DATA CHANGED unit attention correctly we'd
>> +	 * rather wait here.
>> +	 */
>> +	do {
>> +		tur_result = scsi_test_unit_ready(sdev, SCSI_TIMEOUT,
>> +							  3, &sshdr);
>> +		if (!tur_result) {
>> +			res = SCSI_SCAN_LUN_PRESENT;
>> +			break;
>> +		}
>> +		if ((driver_byte(tur_result) & DRIVER_SENSE) &&
>> +		    scsi_sense_valid(&sshdr)) {
>> +			SCSI_LOG_SCAN_BUS(3, sdev_printk(KERN_INFO, sdev,
>> +				"scsi_scan: tur returned %02x/%02x/%02x\n",
>> +				sshdr.sense_key, sshdr.asc, sshdr.ascq));
>> +			if (sshdr.sense_key == NOT_READY &&
>> +			    sshdr.asc == 0x04 && sshdr.ascq == 0x01) {
>> +				/* Logical Unit is in process
>> +				 * of becoming ready */
>> +				msleep(100);
>> +				continue;
>> +			}
>> +		}
> 
> In SAS, you may see these after power on or after sending a
> START STOP UNIT command with START=1, as the controller or 
> expander is performing staggered spinup:
> 04h/11h LOGICAL UNIT NOT READY, NOTIFY (ENABLE SPINUP) REQUIRED
> 
> Once the drive receives NOTIFY (ENABLE SPINUP), then it starts
> reporting this until spinup is done:
> 04h/01h LOGICAL UNIT IS IN PROCESS OF BECOMING READY
> 
> If this function is intended for general "wait for ready" use, 
> then it should sit through both codes.
> 
> If this function is only intended for drives that violate the
> rule to always be capable of returning REPORT LUNS data 
> indicating LUN 0, it's hard to guess if 04h/11h could also
> appear.  
> 
Oh, but the point is they do _NOT_ violate the rule.
They follow SAM to the letter, and return a dummy LUN inventory with
just LUN0. Once the firmware is ready it sets an 'REPORT LUN DATA
CHANGED' UA. But the SCSI stack never acts on that UA,
and the subsequent LUNs will never scanned.

Cheers,

Hannes

-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (AG Nürnberg)
--
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_scan: Send TEST UNIT READY to LUN0 before LUN scanning

[Hannes Reinecke]

On 12/05/2014 12:43 AM, Sebastian Herbszt wrote:
> Hannes Reinecke wrote:
>> diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
>> index 49014a1..4f446e3 100644
>> --- a/drivers/scsi/scsi_devinfo.c
>> +++ b/drivers/scsi/scsi_devinfo.c
>> @@ -166,6 +166,7 @@ static struct {
>>  	{"easyRAID", "X6P", NULL, BLIST_NOREPORTLUN},
>>  	{"easyRAID", "F8", NULL, BLIST_NOREPORTLUN},
>>  	{"FSC", "CentricStor", "*", BLIST_SPARSELUN | BLIST_LARGELUN},
>> +	{"FUJITSU", "ETERNUS_DX", "*", BLIST_TEST_LUN0 },
>>  	{"Generic", "USB SD Reader", "1.00", BLIST_FORCELUN | BLIST_INQUIRY_36},
>>  	{"Generic", "USB Storage-SMC", "0180", BLIST_FORCELUN | BLIST_INQUIRY_36},
>>  	{"Generic", "USB Storage-SMC", "0207", BLIST_FORCELUN | BLIST_INQUIRY_36},
> 
> Does this apply to all ETERNUS DX models and generations?
> 
So I've been given to understand. Matthias Roessler will have details.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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_scan: Send TEST UNIT READY to LUN0 before LUN scanning

[Hannes Reinecke]

On 12/05/2014 01:19 AM, James Bottomley wrote:
> On Thu, 2014-12-04 at 17:39 +0100, Hannes Reinecke wrote:
> [...]
>> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
>> index 0af7133..ca39e32 100644
>> --- a/drivers/scsi/scsi_scan.c
>> +++ b/drivers/scsi/scsi_scan.c
>> @@ -119,6 +119,13 @@ MODULE_PARM_DESC(inq_timeout,
>>  		 "Timeout (in seconds) waiting for devices to answer INQUIRY."
>>  		 " Default is 20. Some devices may need more; most need less.");
>>  
>> +static unsigned int scsi_scan_timeout = SCSI_TIMEOUT/HZ + 58;
>> +
>> +module_param_named(scan_timeout, scsi_scan_timeout, uint, S_IRUGO|S_IWUSR);
>> +MODULE_PARM_DESC(scan_timeout,
>> +		 "Timeout (in seconds) waiting for devices to become ready"
>> +		 " after INQUIRY. Default is 60.");
>> +
>>  /* This lock protects only this list */
>>  static DEFINE_SPINLOCK(async_scan_lock);
>>  static LIST_HEAD(scanning_hosts);
>> @@ -719,19 +726,6 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
>>  	}
>>  
>>  	/*
>> -	 * Related to the above issue:
>> -	 *
>> -	 * XXX Devices (disk or all?) should be sent a TEST UNIT READY,
>> -	 * and if not ready, sent a START_STOP to start (maybe spin up) and
>> -	 * then send the INQUIRY again, since the INQUIRY can change after
>> -	 * a device is initialized.
> 
> You're not sending a start_stop unit, so why is this being deleted?  Any
> unstarted unit will still return initialization command required and may
> or may not return lun data.
> 
Hmm. Okay.

>> -	 *
>> -	 * Ideally, start a device if explicitly asked to do so.  This
>> -	 * assumes that a device is spun up on power on, spun down on
>> -	 * request, and then spun up on request.
>> -	 */
>> -
>> -	/*
>>  	 * 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.
>> @@ -756,6 +750,65 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
>>  }
>>  
>>  /**
>> + * scsi_test_lun - waiting for a LUN to become ready
>> + * @sdev:	scsi_device to test
>> + *
>> + * Description:
>> + *     Wait for the lun associated with @sdev to become ready
>> + *
>> + *     Send a TEST UNIT READY to detect any unit attention conditions.
>> + *     Retry TEST UNIT READY for up to @scsi_scan_timeout if the
>> + *     returned sense key is 02/04/01 (Not ready, Logical Unit is
>> + *     in process of becoming ready)
>> + **/
>> +static int
>> +scsi_test_lun(struct scsi_device *sdev)
>> +{
>> +	struct scsi_sense_hdr sshdr;
>> +	int res = SCSI_SCAN_TARGET_PRESENT;
>> +	int tur_result;
>> +	unsigned long tur_timeout = jiffies + scsi_scan_timeout * HZ;
>> +
>> +	/* Skip for older devices */
>> +	if (sdev->scsi_level <= SCSI_3)
>> +		return SCSI_SCAN_LUN_PRESENT;
>> +
>> +	/*
>> +	 * Wait for the device to become ready.
>> +	 *
>> +	 * Some targets take some time before the firmware is
>> +	 * fully initialized, during which time they might not
>> +	 * be able to fill out any REPORT_LUN command correctly.
>> +	 * And as we're not capable of handling the
>> +	 * INQUIRY DATA CHANGED unit attention correctly we'd
>> +	 * rather wait here.
> 
> Can't we just fix that?  All you need is rescan to re-read the inquiry
> data.
> 
In theory, yes.
However, there is this nasty issue with UA preference, so a
Power-On/Reset UA might have obscured this one.
Plus we would need to enable it in general, in which case it would
also be run if someone unmapped LUNs on the array.
At which point we would have to _delete_ scsi devices on the fly,
something I'm not really comfortable with.
At least _not_ without quite some testing.

Besides, correct UA handling would be a good topic for LSF.
There _are_ areas where we could improve.

>> +	 */
>> +	do {
>> +		tur_result = scsi_test_unit_ready(sdev, SCSI_TIMEOUT,
>> +							  3, &sshdr);
>> +		if (!tur_result) {
>> +			res = SCSI_SCAN_LUN_PRESENT;
>> +			break;
>> +		}
>> +		if ((driver_byte(tur_result) & DRIVER_SENSE) &&
>> +		    scsi_sense_valid(&sshdr)) {
>> +			SCSI_LOG_SCAN_BUS(3, sdev_printk(KERN_INFO, sdev,
>> +				"scsi_scan: tur returned %02x/%02x/%02x\n",
>> +				sshdr.sense_key, sshdr.asc, sshdr.ascq));
> 
> If we're waiting 60s and coming in here every 100ms we'll get 600 of
> these in the log ... that's going to drown out anything that came before
> it.
> 
Ok, I'll be switching to _ratelimit here.

>> +			if (sshdr.sense_key == NOT_READY &&
>> +			    sshdr.asc == 0x04 && sshdr.ascq == 0x01) {
>> +				/* Logical Unit is in process
>> +				 * of becoming ready */
>> +				msleep(100);
>> +				continue;
>> +			}
>> +		}
>> +		res = SCSI_SCAN_LUN_PRESENT;
> 
> Aren't you missing a break here?  Otherwise how does an unexpected
> return code from the test unit ready do anything other than loop around
> dumping log messages until 60s have expired.
> 
Hmm. Indeed. I'll be fixing it up.

>> +	} while (time_before_eq(jiffies, tur_timeout));
>> +	return res;
>> +}
>> +
>> +/**
>>   * scsi_add_lun - allocate and fully initialze a scsi_device
>>   * @sdev:	holds information to be stored in the new scsi_device
>>   * @inq_result:	holds the result of a previous INQUIRY to the LUN
>> @@ -1159,6 +1212,15 @@ static int scsi_probe_and_add_lun(struct scsi_target *starget,
>>  		goto out_free_result;
>>  	}
>>  
>> +	if (bflags & BLIST_TEST_LUN0) {
>> +		res = scsi_test_lun(sdev);
>> +		if (res == SCSI_SCAN_TARGET_PRESENT) {
>> +			SCSI_LOG_SCAN_BUS(1, sdev_printk(KERN_INFO, sdev,
>> +				"scsi scan: device not ready\n"));
>> +			goto out_free_result;
>> +		}
>> +	}
>> +
>>  	res = scsi_add_lun(sdev, result, &bflags, shost->async_scan);
> 
> So any return other than SCSI_SCAN_TARGET_PRESENT gets thrown away.  Why
> not just return true/false from the scsi_test_lun(sdev) and set res to
> SCSI_SCAN_TARGET_PRESENT in the if clause if it fails?  That should
> greatly simplify the logic inside scsi_test_lun().
> 
Ok. Will be updating the patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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_scan: Send TEST UNIT READY to LUN0 before LUN scanning

[Matthias Roessler]

> 
> On 12/05/2014 12:43 AM, Sebastian Herbszt wrote:
> > Hannes Reinecke wrote:
> >> diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
> >> index 49014a1..4f446e3 100644
> >> --- a/drivers/scsi/scsi_devinfo.c
> >> +++ b/drivers/scsi/scsi_devinfo.c
> >> @@ -166,6 +166,7 @@ static struct {
> >>  	{"easyRAID", "X6P", NULL, BLIST_NOREPORTLUN},
> >>  	{"easyRAID", "F8", NULL, BLIST_NOREPORTLUN},
> >>  	{"FSC", "CentricStor", "*", BLIST_SPARSELUN | BLIST_LARGELUN},
> >> +	{"FUJITSU", "ETERNUS_DX", "*", BLIST_TEST_LUN0 },
> >>  	{"Generic", "USB SD Reader", "1.00", BLIST_FORCELUN | BLIST_INQUIRY_36},
> >>  	{"Generic", "USB Storage-SMC", "0180", BLIST_FORCELUN | BLIST_INQUIRY_36},
> >>  	{"Generic", "USB Storage-SMC", "0207", BLIST_FORCELUN | BLIST_INQUIRY_36},
> > 
> > Does this apply to all ETERNUS DX models and generations?
> > 
> So I've been given to understand. Matthias Roessler will have details.
> 

Yes,

Fujitu's main line for Enterprise-grade storage systems 
is named "ETERNUS DX" over the past years and this product line
is still valid. Despite of product names like  "ETERNUS DX90 S2" or
"ETERNUS DX600 S3", they answer the scsi inquiry always with
    Vendor identification:  FUJITSU 
    Product identification: ETERNUS_DX*
where '*' stands for one of [L, M, 400, ...]  depending on the
exact model variant.
There was an ETERNUS4000 raid system with the
Product identification:  E4000
but this product is not of relevance anymore.


Best regards
Matthias

Re: [PATCH] scsi_scan: Send TEST UNIT READY to LUN0 before LUN scanning

[Christoph Hellwig]

Can you resend a version with the small updates pointed out by
James?  I'd rather get the full UA handling, but it seems like that will
take longer, so I'd rather get something in to get the Fujitsu arrays
working ASAP.