[PATCH/RFC V2 07/16] scsi: support well known logical units

[PATCH/RFC V2 07/16] scsi: support well known logical units

[Dolev Raviv]

From: Subhash Jadavani <subhashj@codeaurora.org>

REPORT LUNS command has "SELECT REPORT" field which controls what type of
logical units to be reported by device server. According to UFS device
standard, if this field is set to 0, REPORT LUNS would report only report
standard logical units. If it's set to 1 then it would report only well
known logical unit and if it's set to 2 then device would report both
standard and well known logical units.
Some well-known logical units might not have scsi upper-layer drivers. In
such cases, the runtime PM reference count increased during device
enumeration will not be decreased, causing the parent device (host) to
always be on even during idle.

This change allows the SCSI LLD (Low Level Driver) to choose which type
of logical units should be detected.

Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
Signed-off-by: Dolev Raviv <draviv@codeaurora.org>

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 4a6e4ba..5a0e164 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -802,6 +802,14 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	} else {
 		sdev->type = (inq_result[0] & 0x1f);
 		sdev->removable = (inq_result[1] & 0x80) >> 7;
+
+		/*
+		 * some devices may respond with wrong type for
+		 * well-known logical units. Force well-known type
+		 * to enumerate them correctly.
+		 */
+		if (scsi_is_wlun(sdev->lun) && (sdev->type != TYPE_WLUN))
+			sdev->type = TYPE_WLUN;
 	}
 
 	switch (sdev->type) {
@@ -817,6 +825,7 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	case TYPE_COMM:
 	case TYPE_RAID:
 	case TYPE_OSD:
+	case TYPE_WLUN:
 		sdev->writeable = 1;
 		break;
 	case TYPE_ROM:
@@ -1412,6 +1421,13 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 	 */
 	memset(&scsi_cmd[1], 0, 5);
 
+	if (shost->report_wlus)
+		/*
+		 * Set "SELECT REPORT" field to 0x2 which will make device to
+		 * report well known logical units along with standard LUs.
+		 */
+		scsi_cmd[2] = 0x2;
+
 	/*
 	 * bytes 6 - 9: length of the command.
 	 */
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 5f36788..ba9d0f0 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1060,6 +1060,13 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 		}
 	}
 
+	/*
+	 * put runtime pm reference for well-known logical units,
+	 * drivers are expected to _get_* again during probe.
+	 */
+	if (scsi_is_wlun(sdev->lun))
+		scsi_autopm_put_device(sdev);
+
 	return error;
 }
 
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index 91e2e42..6a9b102 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -332,6 +332,7 @@ static inline int scsi_status_is_good(int status)
 #define TYPE_ENCLOSURE      0x0d    /* Enclosure Services Device */
 #define TYPE_RBC	    0x0e
 #define TYPE_OSD            0x11
+#define TYPE_WLUN           0x1e    /* well-known logical unit */
 #define TYPE_NO_LUN         0x7f
 
 /* SCSI protocols; these are taken from SPC-3 section 7.5 */
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index b2bc519..05664c4 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -675,6 +675,11 @@ struct Scsi_Host {
 	unsigned no_write_same:1;
 
 	/*
+	 * Set "SELECT REPORT" field to allow detection of well known logical
+	 * units along with standard LUs.
+	 */
+	unsigned report_wlus:1;
+	/*
 	 * Optional work queue to be utilized by the transport
 	 */
 	char work_q_name[20];
-- 
1.8.5.2
-- 
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH/RFC V2 07/16] scsi: support well known logical units

[Christoph Hellwig]

On Thu, Aug 14, 2014 at 04:30:58PM +0300, Dolev Raviv wrote:
> From: Subhash Jadavani <subhashj@codeaurora.org>
> 
> REPORT LUNS command has "SELECT REPORT" field which controls what type of
> logical units to be reported by device server. According to UFS device
> standard, if this field is set to 0, REPORT LUNS would report only report
> standard logical units. If it's set to 1 then it would report only well
> known logical unit and if it's set to 2 then device would report both
> standard and well known logical units.
> Some well-known logical units might not have scsi upper-layer drivers. In
> such cases, the runtime PM reference count increased during device
> enumeration will not be decreased, causing the parent device (host) to
> always be on even during idle.
> 
> This change allows the SCSI LLD (Low Level Driver) to choose which type
> of logical units should be detected.
> 
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
> Signed-off-by: Dolev Raviv <draviv@codeaurora.org>
> 
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 4a6e4ba..5a0e164 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -802,6 +802,14 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
>  	} else {
>  		sdev->type = (inq_result[0] & 0x1f);
>  		sdev->removable = (inq_result[1] & 0x80) >> 7;
> +
> +		/*
> +		 * some devices may respond with wrong type for
> +		 * well-known logical units. Force well-known type
> +		 * to enumerate them correctly.
> +		 */
> +		if (scsi_is_wlun(sdev->lun) && (sdev->type != TYPE_WLUN))
> +			sdev->type = TYPE_WLUN;

no need to test for TYPE_WLUN here.

>  	switch (sdev->type) {
> @@ -817,6 +825,7 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
>  	case TYPE_COMM:
>  	case TYPE_RAID:
>  	case TYPE_OSD:
> +	case TYPE_WLUN:
>  		sdev->writeable = 1;
>  		break;
>  	case TYPE_ROM:

This switch statements has been removed in 3.17-rc1, please rebase your
series on top of it.  While you're at it please make this patch, which
introduceѕ core scsi changes first in the series.

> @@ -1412,6 +1421,13 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
>  	 */
>  	memset(&scsi_cmd[1], 0, 5);
>  
> +	if (shost->report_wlus)
> +		/*
> +		 * Set "SELECT REPORT" field to 0x2 which will make device to
> +		 * report well known logical units along with standard LUs.
> +		 */
> +		scsi_cmd[2] = 0x2;

So we're finally getting well known LUN support.  I think we should key this
off the SBC compliance level and not have the drivers opt in.

> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 5f36788..ba9d0f0 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -1060,6 +1060,13 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
>  		}
>  	}
>  
> +	/*
> +	 * put runtime pm reference for well-known logical units,
> +	 * drivers are expected to _get_* again during probe.
> +	 */
> +	if (scsi_is_wlun(sdev->lun))
> +		scsi_autopm_put_device(sdev);

Special casing the well known LUNs here seems wrong.  Shouldn't we do this
for any devices that don't get a driver attached to them?

Re: [PATCH/RFC V2 07/16] scsi: support well known logical units

[Martin K. Petersen]

>>>>> "Christoph" == Christoph Hellwig <hch@infradead.org> writes:

>> + /*
>> + * put runtime pm reference for well-known logical units,
>> + * drivers are expected to _get_* again during probe.
>> + */
>> + if (scsi_is_wlun(sdev->lun))
>> + scsi_autopm_put_device(sdev);

Christoph> Special casing the well known LUNs here seems wrong.
Christoph> Shouldn't we do this for any devices that don't get a driver
Christoph> attached to them?

That would also be my preference. Relying on WLU for this seems like a
kludge.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH/RFC V2 07/16] scsi: support well known logical units

[subhashj]

> On Thu, Aug 14, 2014 at 04:30:58PM +0300, Dolev Raviv wrote:
>> From: Subhash Jadavani <subhashj@codeaurora.org>
>> REPORT LUNS command has "SELECT REPORT" field which controls what type of
>> logical units to be reported by device server. According to UFS device
standard, if this field is set to 0, REPORT LUNS would report only
report
>> standard logical units. If it's set to 1 then it would report only well
known logical unit and if it's set to 2 then device would report both
standard and well known logical units.
>> Some well-known logical units might not have scsi upper-layer drivers. In
>> such cases, the runtime PM reference count increased during device
enumeration will not be decreased, causing the parent device (host) to
always be on even during idle.
>> This change allows the SCSI LLD (Low Level Driver) to choose which type
of logical units should be detected.
>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>> Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
>> Signed-off-by: Dolev Raviv <draviv@codeaurora.org>
>> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c index
4a6e4ba..5a0e164 100644
>> --- a/drivers/scsi/scsi_scan.c
>> +++ b/drivers/scsi/scsi_scan.c
>> @@ -802,6 +802,14 @@ static int scsi_add_lun(struct scsi_device *sdev,
unsigned char *inq_result,
>>  	} else {
>>  		sdev->type = (inq_result[0] & 0x1f);
>>  		sdev->removable = (inq_result[1] & 0x80) >> 7;
>> +
>> +		/*
>> +		 * some devices may respond with wrong type for
>> +		 * well-known logical units. Force well-known type
>> +		 * to enumerate them correctly.
>> +		 */
>> +		if (scsi_is_wlun(sdev->lun) && (sdev->type != TYPE_WLUN))
>> +			sdev->type = TYPE_WLUN;
> no need to test for TYPE_WLUN here.]

Agreed, will fix it.

>>  	switch (sdev->type) {
>> @@ -817,6 +825,7 @@ static int scsi_add_lun(struct scsi_device *sdev,
unsigned char *inq_result,
>>  	case TYPE_COMM:
>>  	case TYPE_RAID:
>>  	case TYPE_OSD:
>> +	case TYPE_WLUN:
>>  		sdev->writeable = 1;
>>  		break;
>>  	case TYPE_ROM:
> This switch statements has been removed in 3.17-rc1, please rebase your
series on top of it.

Sure.

> While you're at it please make this patch, which
> introduceѕ core scsi changes first in the series.

Ok. Will move these core changes first in the current patch series.

>> @@ -1412,6 +1421,13 @@ static int scsi_report_lun_scan(struct
>> scsi_target *starget, int bflags,
>>  	 */
>>  	memset(&scsi_cmd[1], 0, 5);
>> +	if (shost->report_wlus)
>> +		/*
>> +		 * Set "SELECT REPORT" field to 0x2 which will make device to +		 *
report well known logical units along with standard LUs.
>> +		 */
>> +		scsi_cmd[2] = 0x2;
> So we're finally getting well known LUN support.  I think we should key
this
> off the SBC compliance level and not have the drivers opt in.

If i understood this correctly, we want to make all devices report well
known LUs irrespective of device driver (LLD)'s preference? If yes then i
will knock off "if (shost->report_wlus)" check.

>> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 5f36788..ba9d0f0 100644
>> --- a/drivers/scsi/scsi_sysfs.c
>> +++ b/drivers/scsi/scsi_sysfs.c
>> @@ -1060,6 +1060,13 @@ int scsi_sysfs_add_sdev(struct scsi_device
*sdev)
>>  		}
>>  	}
>> +	/*
>> +	 * put runtime pm reference for well-known logical units,
>> +	 * drivers are expected to _get_* again during probe.
>> +	 */
>> +	if (scsi_is_wlun(sdev->lun))
>> +		scsi_autopm_put_device(sdev);
> Special casing the well known LUNs here seems wrong.  Shouldn't we do
this
> for any devices that don't get a driver attached to them?

Agreed, i will replace "if (scsi_is_wlun(sdev->lun))" check with "if
(sdev->sdev_gendev.driver)".

Regards,
Subhash

Re: [PATCH/RFC V2 07/16] scsi: support well known logical units

[Christoph Hellwig]

On Fri, Aug 22, 2014 at 12:02:30AM -0000, subhashj@codeaurora.org wrote:
> >> +	/*
> >> +	 * put runtime pm reference for well-known logical units,
> >> +	 * drivers are expected to _get_* again during probe.
> >> +	 */
> >> +	if (scsi_is_wlun(sdev->lun))
> >> +		scsi_autopm_put_device(sdev);
> > Special casing the well known LUNs here seems wrong.  Shouldn't we do
> this
> > for any devices that don't get a driver attached to them?
> 
> Agreed, i will replace "if (scsi_is_wlun(sdev->lun))" check with "if
> (sdev->sdev_gendev.driver)".

I would really prefer the callers of autopm get/put to be symmetric to
issues with later than probe driver attach or similar.  Think of the
case say the CDROM or tape driver only gets loaded after we already
did a bus scan.  A quick check of the autopm code and it's underlying
implementation seems to suggest that nesting them is okay.  If that's
indeed the case I would suggest to restructure it the following way:

 - make the scsi_autopm_put_device call you add at the end of
   scsi_sysfs_add_sdev unconditional to make it balance out
   the get earlier in the function (and delete the now incorrect comment
   above the scsi_autopm_get_device call).
 - let drivers do paired get/put calls in their probe/remove methods.

If you still need any wlun changes after that please send them as a
separate patch with a detailed description on why you need it.

All in all the autopm code and it's underlying implementation is highly
confusing, so additional documentation on it would also be very welcome.

Re: [PATCH/RFC V2 07/16] scsi: support well known logical units

[subhashj]

> On Fri, Aug 22, 2014 at 12:02:30AM -0000, subhashj@codeaurora.org wrote:
>> >> +	/*
>> >> +	 * put runtime pm reference for well-known logical units,
>> >> +	 * drivers are expected to _get_* again during probe.
>> >> +	 */
>> >> +	if (scsi_is_wlun(sdev->lun))
>> >> +		scsi_autopm_put_device(sdev);
>> > Special casing the well known LUNs here seems wrong.  Shouldn't we do
>> this
>> > for any devices that don't get a driver attached to them?
>>
>> Agreed, i will replace "if (scsi_is_wlun(sdev->lun))" check with "if
>> (sdev->sdev_gendev.driver)".
>
> I would really prefer the callers of autopm get/put to be symmetric to
> issues with later than probe driver attach or similar.  Think of the
> case say the CDROM or tape driver only gets loaded after we already
> did a bus scan.  A quick check of the autopm code and it's underlying
> implementation seems to suggest that nesting them is okay.  If that's
> indeed the case I would suggest to restructure it the following way:
>
>  - make the scsi_autopm_put_device call you add at the end of
>    scsi_sysfs_add_sdev unconditional to make it balance out
>    the get earlier in the function (and delete the now incorrect comment
>    above the scsi_autopm_get_device call).
>  - let drivers do paired get/put calls in their probe/remove methods.
>
> If you still need any wlun changes after that please send them as a
> separate patch with a detailed description on why you need it.

Thanks, yes I was also thinking on same lines but was not sure if it would
work or not. But let me check this more and if it works, will make a
different patch for it.

> All in all the autopm code and it's underlying implementation is highly
> confusing, so additional documentation on it would also be very welcome.

I would agree but not sure if I would be able to add it now, hopefully
some time in future.
> --
> 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
>