[PATCH 1/3] [SCSI] ufs: Remove unused member sense_bufflen

[PATCH 1/3] [SCSI] ufs: Remove unused member sense_bufflen

[Santosh Y]

Remove unused member sense_bufflen from struct ufshcd_lrb.

Signed-off-by: Santosh Y <santoshsy@gmail.com>
---
 drivers/scsi/ufs/ufshcd.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 52b96e8..3e4e102 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -194,7 +194,6 @@ struct ufs_hba {
  * @ucd_prdt_ptr: PRDT address of the command
  * @cmd: pointer to SCSI command
  * @sense_buffer: pointer to sense buffer address of the SCSI command
- * @sense_bufflen: Length of the sense buffer
  * @scsi_status: SCSI status of the command
  * @command_type: SCSI, UFS, Query.
  * @task_tag: Task tag of the command
@@ -208,7 +207,6 @@ struct ufshcd_lrb {
 
 	struct scsi_cmnd *cmd;
 	u8 *sense_buffer;
-	unsigned int sense_bufflen;
 	int scsi_status;
 
 	int command_type;
@@ -684,7 +682,6 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 	lrbp = &hba->lrb[tag];
 
 	lrbp->cmd = cmd;
-	lrbp->sense_bufflen = SCSI_SENSE_BUFFERSIZE;
 	lrbp->sense_buffer = cmd->sense_buffer;
 	lrbp->task_tag = tag;
 	lrbp->lun = cmd->device->lun;
-- 
1.7.5.4

[PATCH 2/3] [SCSI] ufs: Limit sense data size to 18 bytes

[Santosh Y]

As per the UFS specification a UFS device server should return upto
18 bytes of sense data. Currently, if a SCSI command completes with
a check condition, min_t(sense_data_length, SCSI_SENSE_BUFFERSIZE)
bytes of sense data is being copied to sense_buffer.

Correct sense_data_length(<= 18 bytes), from a UFS device will not
cause any problem. If a UFS device returns wrong sense_data_length
( > 18 bytes), then the wrong sense data will be copied to
sense_buffer.

Signed-off-by: Santosh Y <santoshsy@gmail.com>
Reviewed-by: Vinayak Holikatti <vinholikatti@gmail.com>
---
 drivers/scsi/ufs/ufs.h    |    1 +
 drivers/scsi/ufs/ufshcd.c |    2 +-
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index b207529..0af1aac 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -47,6 +47,7 @@
 #define _UFS_H
 
 #define MAX_CDB_SIZE	16
+#define MAX_SENSE_SIZE	18
 
 #define UPIU_HEADER_DWORD(byte3, byte2, byte1, byte0)\
 			((byte3 << 24) | (byte2 << 16) |\
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 3e4e102..662d00f 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -479,7 +479,7 @@ static inline void ufshcd_copy_sense_data(struct ufshcd_lrb *lrbp)
 		len = be16_to_cpu(lrbp->ucd_rsp_ptr->sense_data_len);
 		memcpy(lrbp->sense_buffer,
 			lrbp->ucd_rsp_ptr->sense_data,
-			min_t(int, len, SCSI_SENSE_BUFFERSIZE));
+			min_t(int, len, MAX_SENSE_SIZE));
 	}
 }
 
-- 
1.7.5.4

[PATCH 3/3] [SCSI] ufs: Process SCSI command for UFS device

[Santosh Y]

UFS Spec specifies that the SCSI CDB fields, that are not
supported by UFS should be set to zero. It also mentions
that some of the CDB fields should be set to default
values before issuing the command to a UFS device.
Ex: Mode Select - Page Format bit should be set to 1.

If the fields are not set to values mentioned in UFS spec,
strict CDB checking UFS device may complete the commands
with a check condition.

Signed-off-by: Santosh Y <santoshsy@gmail.com>
Reviewed-by: Vinayak Holikatti <vinholikatti@gmail.com>
---
 drivers/scsi/ufs/ufshcd.c |  100 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 100 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 662d00f..ef416b1 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -583,6 +583,104 @@ static void ufshcd_int_config(struct ufs_hba *hba, u32 option)
 }
 
 /**
+ * ufshcd_process_scsi_cdb - Process SCSI command for UFS device
+ * @cdb: Pointer to SCSI cdb
+ * @cdb_len: SCSI cdb length
+ */
+static inline void
+ufshcd_process_scsi_cdb(unsigned char *cdb, unsigned short cdb_len)
+{
+	switch (cdb[0]) {
+	case READ_10:
+	case READ_16:
+	case WRITE_10:
+	case WRITE_16:
+
+		/* set RDPROTECT/WRPROTECT bits to 0 */
+		cdb[1] &= 0x1F;
+		break;
+	case START_STOP:
+
+		/* set Power Condition Modifier to 0 */
+		cdb[3] &= 0xF0;
+		break;
+	case MODE_SELECT_10:
+
+		/* set Page Format(PF) bit to 1 */
+		cdb[1] |= 0x10;
+		break;
+	case MODE_SENSE_10:
+
+		/*
+		 * set Long LBA Accepted(LLBAA) bit to 0 and
+		 * Disable Block Descriptors(DBD) bit to 1
+		 */
+		cdb[1] = 0x08;
+		break;
+
+	case READ_CAPACITY:
+
+		/* set Partial Medium Indicator(PMI) bit to 0 */
+		cdb[8] &= ~1;
+
+		/* set Logical Block Address(LBA) to 0 */
+		cdb[2] = 0;
+		cdb[3] = 0;
+		break;
+
+	case SERVICE_ACTION_IN:
+		if ((cdb[1] & 0x1F) == SAI_READ_CAPACITY_16) {
+
+			/* set PMI bit to 0 */
+			cdb[14] &= ~1;
+
+			/* set Logical Block Address(LBA) to 0 */
+			cdb[2] = 0;
+			cdb[3] = 0;
+			cdb[4] = 0;
+			cdb[5] = 0;
+			cdb[6] = 0;
+			cdb[7] = 0;
+			cdb[8] = 0;
+			cdb[9] = 0;
+		}
+		break;
+	case VERIFY:
+
+		/* set VRPROTECT, Disable Page Out(DPO), BYTCHK bits to 0 */
+		cdb[1] &= 0x0C;
+
+		/* set Group Number to 0 */
+		cdb[6] &= 0xF0;
+		break;
+
+	case REQUEST_SENSE:
+
+		/* set Descriptor Format(DESC) bit to 0 */
+		cdb[1] = 0;
+
+		/* device server will transfer upto 18 bytes of data */
+		if (cdb[4] > MAX_SENSE_SIZE)
+			cdb[4] = MAX_SENSE_SIZE;
+		break;
+
+	case SECURITY_PROTOCOL_IN:
+	case SECURITY_PROTOCOL_OUT:
+
+		/*
+		 * set INC_512 to 0, to specify Allocation/Transfer Length
+		 * represent the number of bytes to be transferred
+		 */
+		cdb[4] &= 0x7F;
+		break;
+	}
+
+	/* set control field to 0 */
+	if (cdb_len <= MAX_CDB_SIZE)
+		cdb[cdb_len - 1] = 0;
+}
+
+/**
  * ufshcd_compose_upiu - form UFS Protocol Information Unit(UPIU)
  * @lrb - pointer to local reference block
  */
@@ -645,6 +743,8 @@ static void ufshcd_compose_upiu(struct ufshcd_lrb *lrbp)
 		       (min_t(unsigned short,
 			      lrbp->cmd->cmd_len,
 			      MAX_CDB_SIZE)));
+
+		ufshcd_process_scsi_cdb(ucd_cmd_ptr->cdb, lrbp->cmd->cmd_len);
 		break;
 	case UTP_CMD_TYPE_DEV_MANAGE:
 		/* For query function implementation */
-- 
1.7.5.4

Re: [PATCH 3/3] [SCSI] ufs: Process SCSI command for UFS device

[James Bottomley]

On Sat, 2012-04-21 at 14:39 +0530, Santosh Y wrote:
> UFS Spec specifies that the SCSI CDB fields, that are not
> supported by UFS should be set to zero. It also mentions
> that some of the CDB fields should be set to default
> values before issuing the command to a UFS device.
> Ex: Mode Select - Page Format bit should be set to 1.
> 
> If the fields are not set to values mentioned in UFS spec,
> strict CDB checking UFS device may complete the commands
> with a check condition.

Altering CDB field values in the driver is a bad idea.  If you tamper
with commands in the driver unexpected things will be returned to
userspace.  The general principle is that the mid layer tries to be as
correct and conservative as possible, but we don't forbid users from
sending out of spec commands because sometimes that's required.

Are you actually trying to fix a bug, or is this just theoretical?  If
the former, the way to fix it is to get the mid layer to be more
conservative about what it does (or possibly add a quirk flag).  If the
latter, then I think you need to wait until you see a problem first
(unless you can infer a problem from what the mid-layer will do).

James


> Signed-off-by: Santosh Y <santoshsy@gmail.com>
> Reviewed-by: Vinayak Holikatti <vinholikatti@gmail.com>
> ---
>  drivers/scsi/ufs/ufshcd.c |  100 +++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 100 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 662d00f..ef416b1 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -583,6 +583,104 @@ static void ufshcd_int_config(struct ufs_hba *hba, u32 option)
>  }
>  
>  /**
> + * ufshcd_process_scsi_cdb - Process SCSI command for UFS device
> + * @cdb: Pointer to SCSI cdb
> + * @cdb_len: SCSI cdb length
> + */
> +static inline void
> +ufshcd_process_scsi_cdb(unsigned char *cdb, unsigned short cdb_len)
> +{
> +	switch (cdb[0]) {
> +	case READ_10:
> +	case READ_16:
> +	case WRITE_10:
> +	case WRITE_16:
> +
> +		/* set RDPROTECT/WRPROTECT bits to 0 */
> +		cdb[1] &= 0x1F;
> +		break;
> +	case START_STOP:
> +
> +		/* set Power Condition Modifier to 0 */
> +		cdb[3] &= 0xF0;
> +		break;
> +	case MODE_SELECT_10:
> +
> +		/* set Page Format(PF) bit to 1 */
> +		cdb[1] |= 0x10;
> +		break;
> +	case MODE_SENSE_10:
> +
> +		/*
> +		 * set Long LBA Accepted(LLBAA) bit to 0 and
> +		 * Disable Block Descriptors(DBD) bit to 1
> +		 */
> +		cdb[1] = 0x08;
> +		break;
> +
> +	case READ_CAPACITY:
> +
> +		/* set Partial Medium Indicator(PMI) bit to 0 */
> +		cdb[8] &= ~1;
> +
> +		/* set Logical Block Address(LBA) to 0 */
> +		cdb[2] = 0;
> +		cdb[3] = 0;
> +		break;
> +
> +	case SERVICE_ACTION_IN:
> +		if ((cdb[1] & 0x1F) == SAI_READ_CAPACITY_16) {
> +
> +			/* set PMI bit to 0 */
> +			cdb[14] &= ~1;
> +
> +			/* set Logical Block Address(LBA) to 0 */
> +			cdb[2] = 0;
> +			cdb[3] = 0;
> +			cdb[4] = 0;
> +			cdb[5] = 0;
> +			cdb[6] = 0;
> +			cdb[7] = 0;
> +			cdb[8] = 0;
> +			cdb[9] = 0;
> +		}
> +		break;
> +	case VERIFY:
> +
> +		/* set VRPROTECT, Disable Page Out(DPO), BYTCHK bits to 0 */
> +		cdb[1] &= 0x0C;
> +
> +		/* set Group Number to 0 */
> +		cdb[6] &= 0xF0;
> +		break;
> +
> +	case REQUEST_SENSE:
> +
> +		/* set Descriptor Format(DESC) bit to 0 */
> +		cdb[1] = 0;
> +
> +		/* device server will transfer upto 18 bytes of data */
> +		if (cdb[4] > MAX_SENSE_SIZE)
> +			cdb[4] = MAX_SENSE_SIZE;
> +		break;
> +
> +	case SECURITY_PROTOCOL_IN:
> +	case SECURITY_PROTOCOL_OUT:
> +
> +		/*
> +		 * set INC_512 to 0, to specify Allocation/Transfer Length
> +		 * represent the number of bytes to be transferred
> +		 */
> +		cdb[4] &= 0x7F;
> +		break;
> +	}
> +
> +	/* set control field to 0 */
> +	if (cdb_len <= MAX_CDB_SIZE)
> +		cdb[cdb_len - 1] = 0;
> +}
> +
> +/**
>   * ufshcd_compose_upiu - form UFS Protocol Information Unit(UPIU)
>   * @lrb - pointer to local reference block
>   */
> @@ -645,6 +743,8 @@ static void ufshcd_compose_upiu(struct ufshcd_lrb *lrbp)
>  		       (min_t(unsigned short,
>  			      lrbp->cmd->cmd_len,
>  			      MAX_CDB_SIZE)));
> +
> +		ufshcd_process_scsi_cdb(ucd_cmd_ptr->cdb, lrbp->cmd->cmd_len);
>  		break;
>  	case UTP_CMD_TYPE_DEV_MANAGE:
>  		/* For query function implementation */

Re: [PATCH 3/3] [SCSI] ufs: Process SCSI command for UFS device

[Santosh Y]

On Sat, Apr 21, 2012 at 4:29 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Sat, 2012-04-21 at 14:39 +0530, Santosh Y wrote:
>> UFS Spec specifies that the SCSI CDB fields, that are not
>> supported by UFS should be set to zero. It also mentions
>> that some of the CDB fields should be set to default
>> values before issuing the command to a UFS device.
>> Ex: Mode Select - Page Format bit should be set to 1.
>>
>> If the fields are not set to values mentioned in UFS spec,
>> strict CDB checking UFS device may complete the commands
>> with a check condition.
>
> Altering CDB field values in the driver is a bad idea.  If you tamper
> with commands in the driver unexpected things will be returned to
> userspace.  The general principle is that the mid layer tries to be as
> correct and conservative as possible, but we don't forbid users from
> sending out of spec commands because sometimes that's required.
>
> Are you actually trying to fix a bug, or is this just theoretical?  If

At this point it is theoretical, since the UFS spec mentions it.
But I guess soon enough I'll run into this problem when I'll start
testing with updated firmwares. Which may strictly follow UFS spec
and expect some of the SCSI CDB fields must be set to UFS specific
default values.

> the former, the way to fix it is to get the mid layer to be more
> conservative about what it does (or possibly add a quirk flag).

As you suggested, will try to find a better approach.

> James
>

-- 
~Santosh
--
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