[PATCH 1/2] [SCSI] ufs: update Response UPIU length in dword

[PATCH 1/2] [SCSI] ufs: update Response UPIU length in dword

[Santosh Y]

UFSHCI spec mentions that Response UPIU Length(RUL)
field in Transfer Request Descriptor should be in dword.

Query Response UPIU size is variable depending on the data
to be read/written and the size of a SCSI command Response
UPIU is fixed.

Currently response_upiu_length is being updated in bytes.
If a UFS host controller prepares a Query Response UPIU
with response_upiu_length, it will result in wrong Query
Response UPIU size corrupting the command descriptor list.

This issue will not affect the current UFSHCD Ver 0.1,
since Query function support is not yet implemented.
But this patch also ensures against command descriptor list
corruption if a UFS controller prepares SCSI Response UPIU
with response_upiu_length.

Reported-by: KOBAYASHI Yoshitake <yoshitake.kobayashi@toshiba.co.jp>
Reviewed-by: Vinayak Holikatti <vinholikatti@gmail.com>
Signed-off-by: Santosh Y <santoshsy@gmail.com>
---
 drivers/scsi/ufs/ufshcd.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 52b96e8..1878cd8 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -830,13 +830,16 @@ static void ufshcd_host_memory_configure(struct ufs_hba *hba)
 		utrdlp[i].command_desc_base_addr_hi =
 				cpu_to_le32(upper_32_bits(cmd_desc_element_addr));
 
-		/* Response upiu and prdt offset should be in double words */
+		/*
+		 * Response upiu offset, prdt offset and response upiu length
+		 * should be in double words
+		 */
 		utrdlp[i].response_upiu_offset =
 				cpu_to_le16((response_offset >> 2));
 		utrdlp[i].prd_table_offset =
 				cpu_to_le16((prdt_offset >> 2));
 		utrdlp[i].response_upiu_length =
-				cpu_to_le16(ALIGNED_UPIU_SIZE);
+				cpu_to_le16(ALIGNED_UPIU_SIZE >> 2);
 
 		hba->lrb[i].utr_descriptor_ptr = (utrdlp + i);
 		hba->lrb[i].ucd_cmd_ptr =
-- 
1.7.5.4

[PATCH 2/2] [SCSI] ufs: calculate read/write xfer len from cdb

[Santosh Y]

Currently the Expected data transfer length field in
command UPIU is being updated with "transfersize" from
the scsi_cmnd struct. But, if the read/write data
transfer size exceeds the sector size, the "transfersize"
will be truncated to the sector size. Thanks to KOBAYASHI
Yoshitake for pointing it out.

This patch ensures that the correct read/write data transfer
size is calculated from the "transfer length" available in the
CDB.

Reported-by: KOBAYASHI Yoshitake <yoshitake.kobayashi@toshiba.co.jp>
Reviewed-by: Vinayak Holikatti <vinholikatti@gmail.com>
Signed-off-by: Santosh Y <santoshsy@gmail.com>
---
 drivers/scsi/ufs/ufshcd.c |   37 ++++++++++++++++++++++++++++++++++++-
 1 files changed, 36 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 1878cd8..4f99ab4 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -585,6 +585,41 @@ static void ufshcd_int_config(struct ufs_hba *hba, u32 option)
 }
 
 /**
+ * ufshcd_get_transfer_len - calculate data transfer size for read/write
+ *			     commands from the CDB
+ * @cmdp: pointer to SCSI command
+ */
+static inline u32 ufshcd_get_transfer_len(struct scsi_cmnd *cmdp)
+{
+	u32 transfer_len;
+
+	switch (cmdp->cmnd[0]) {
+	case READ_6:
+	case WRITE_6:
+		transfer_len = cmdp->device->sector_size *
+			       cmdp->cmnd[4];
+		break;
+	case READ_10:
+	case WRITE_10:
+		transfer_len = cmdp->device->sector_size *
+			       ((cmdp->cmnd[7] << 8) |
+				cmdp->cmnd[8]);
+		break;
+	case READ_16:
+	case WRITE_16:
+		transfer_len = cmdp->device->sector_size *
+			       ((cmdp->cmnd[10] << 24) |
+				(cmdp->cmnd[11] << 16) |
+				(cmdp->cmnd[12] << 8) |
+				cmdp->cmnd[13]);
+		break;
+	default:
+		transfer_len = cmdp->transfersize;
+	}
+	return transfer_len;
+}
+
+/**
  * ufshcd_compose_upiu - form UFS Protocol Information Unit(UPIU)
  * @lrb - pointer to local reference block
  */
@@ -640,7 +675,7 @@ static void ufshcd_compose_upiu(struct ufshcd_lrb *lrbp)
 		ucd_cmd_ptr->header.dword_2 = 0;
 
 		ucd_cmd_ptr->exp_data_transfer_len =
-			cpu_to_be32(lrbp->cmd->transfersize);
+			cpu_to_be32(ufshcd_get_transfer_len(lrbp->cmd));
 
 		memcpy(ucd_cmd_ptr->cdb,
 		       lrbp->cmd->cmnd,
-- 
1.7.5.4

Re: [PATCH 2/2] [SCSI] ufs: calculate read/write xfer len from cdb

[Namjae Jeon]

2012/4/6 Santosh Y <santoshsy@gmail.com>:
> Currently the Expected data transfer length field in
> command UPIU is being updated with "transfersize" from
> the scsi_cmnd struct. But, if the read/write data
> transfer size exceeds the sector size, the "transfersize"
> will be truncated to the sector size. Thanks to KOBAYASHI
> Yoshitake for pointing it out.
>
> This patch ensures that the correct read/write data transfer
> size is calculated from the "transfer length" available in the
> CDB.
>
> Reported-by: KOBAYASHI Yoshitake <yoshitake.kobayashi@toshiba.co.jp>
> Reviewed-by: Vinayak Holikatti <vinholikatti@gmail.com>
> Signed-off-by: Santosh Y <santoshsy@gmail.com>

Hi Santosh.
The specific decription header of this patch is very good.
Reviewed-by: Namjae Jeon <linkinjeon@gmail.com>

> ---

Re: [PATCH 2/2] [SCSI] ufs: calculate read/write xfer len from cdb

[James Bottomley]

On Fri, 2012-04-06 at 17:57 +0530, Santosh Y wrote:
> Currently the Expected data transfer length field in
> command UPIU is being updated with "transfersize" from
> the scsi_cmnd struct. But, if the read/write data
> transfer size exceeds the sector size, the "transfersize"
> will be truncated to the sector size. Thanks to KOBAYASHI
> Yoshitake for pointing it out.

I'm a bit confused by this changelog, but I think it's saying you're
using the cmd->transfersize field for the length of data to transfer?
In which case, that's wrong cmd->transfersize is the minimum transfer
size (usually a sector) ... badly named field, sorry.

Based on the calculation below, the actual length you're looking for is

scsi_cmnd->sdb->length

Which is the total size of the entire transfer in bytes.


> This patch ensures that the correct read/write data transfer
> size is calculated from the "transfer length" available in the
> CDB.
> 
> Reported-by: KOBAYASHI Yoshitake <yoshitake.kobayashi@toshiba.co.jp>
> Reviewed-by: Vinayak Holikatti <vinholikatti@gmail.com>
> Signed-off-by: Santosh Y <santoshsy@gmail.com>
> ---
>  drivers/scsi/ufs/ufshcd.c |   37 ++++++++++++++++++++++++++++++++++++-
>  1 files changed, 36 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 1878cd8..4f99ab4 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -585,6 +585,41 @@ static void ufshcd_int_config(struct ufs_hba *hba, u32 option)
>  }
>  
>  /**
> + * ufshcd_get_transfer_len - calculate data transfer size for read/write
> + *			     commands from the CDB
> + * @cmdp: pointer to SCSI command
> + */
> +static inline u32 ufshcd_get_transfer_len(struct scsi_cmnd *cmdp)
> +{
> +	u32 transfer_len;
> +
> +	switch (cmdp->cmnd[0]) {
> +	case READ_6:
> +	case WRITE_6:
> +		transfer_len = cmdp->device->sector_size *
> +			       cmdp->cmnd[4];

Just an efficiency not here, even though the code won't be written this
way, you don't want to multiply by sector_size ... it's always a power
of two, so you do shift or binary log tricks to avoid the expensive
multiply.

James

> +		break;
> +	case READ_10:
> +	case WRITE_10:
> +		transfer_len = cmdp->device->sector_size *
> +			       ((cmdp->cmnd[7] << 8) |
> +				cmdp->cmnd[8]);
> +		break;
> +	case READ_16:
> +	case WRITE_16:
> +		transfer_len = cmdp->device->sector_size *
> +			       ((cmdp->cmnd[10] << 24) |
> +				(cmdp->cmnd[11] << 16) |
> +				(cmdp->cmnd[12] << 8) |
> +				cmdp->cmnd[13]);
> +		break;
> +	default:
> +		transfer_len = cmdp->transfersize;
> +	}
> +	return transfer_len;
> +}
> +
> +/**
>   * ufshcd_compose_upiu - form UFS Protocol Information Unit(UPIU)
>   * @lrb - pointer to local reference block
>   */
> @@ -640,7 +675,7 @@ static void ufshcd_compose_upiu(struct ufshcd_lrb *lrbp)
>  		ucd_cmd_ptr->header.dword_2 = 0;
>  
>  		ucd_cmd_ptr->exp_data_transfer_len =
> -			cpu_to_be32(lrbp->cmd->transfersize);
> +			cpu_to_be32(ufshcd_get_transfer_len(lrbp->cmd));
>  
>  		memcpy(ucd_cmd_ptr->cdb,
>  		       lrbp->cmd->cmnd,

Re: [PATCH 2/2] [SCSI] ufs: calculate read/write xfer len from cdb

[Santosh Y]

On Sun, Apr 8, 2012 at 2:14 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Fri, 2012-04-06 at 17:57 +0530, Santosh Y wrote:
>> Currently the Expected data transfer length field in
>> command UPIU is being updated with "transfersize" from
>> the scsi_cmnd struct. But, if the read/write data
>> transfer size exceeds the sector size, the "transfersize"
>> will be truncated to the sector size. Thanks to KOBAYASHI
>> Yoshitake for pointing it out.
>
> I'm a bit confused by this changelog, but I think it's saying you're
> using the cmd->transfersize field for the length of data to transfer?

Yes, I'll update the comment to make it clear. It might be confusing, if one
is not familiar with the UFS spec.

> In which case, that's wrong cmd->transfersize is the minimum transfer
> size (usually a sector) ... badly named field, sorry.
>
> Based on the calculation below, the actual length you're looking for is
>
> scsi_cmnd->sdb->length
>
> Which is the total size of the entire transfer in bytes.

Thanks, don't know how did I overlook it. Ended up calculating the
same thing again. :-)
I'll resend the patch, which updates Expected data transfer length field in
command UPIU with scsi_cmnd->sdb->length.

>> + */
>> +static inline u32 ufshcd_get_transfer_len(struct scsi_cmnd *cmdp)
>> +{
>> +     u32 transfer_len;
>> +
>> +     switch (cmdp->cmnd[0]) {
>> +     case READ_6:
>> +     case WRITE_6:
>> +             transfer_len = cmdp->device->sector_size *
>> +                            cmdp->cmnd[4];
>
> Just an efficiency not here, even though the code won't be written this
> way, you don't want to multiply by sector_size ... it's always a power
> of two, so you do shift or binary log tricks to avoid the expensive
> multiply.
>

It looks like sdb.length is being calculated the same way in sd_prep_fn(),

SCpnt->sdb.length = this_count * sdp->sector_size;

Does it need to be updated?

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

Re: [PATCH 1/2] [SCSI] ufs: update Response UPIU length in dword

[Namjae Jeon]

2012/4/6 Santosh Y <santoshsy@gmail.com>:
> UFSHCI spec mentions that Response UPIU Length(RUL)
> field in Transfer Request Descriptor should be in dword.
>
> Query Response UPIU size is variable depending on the data
> to be read/written and the size of a SCSI command Response
> UPIU is fixed.
>
> Currently response_upiu_length is being updated in bytes.
> If a UFS host controller prepares a Query Response UPIU
> with response_upiu_length, it will result in wrong Query
> Response UPIU size corrupting the command descriptor list.
>
> This issue will not affect the current UFSHCD Ver 0.1,
> since Query function support is not yet implemented.
> But this patch also ensures against command descriptor list
> corruption if a UFS controller prepares SCSI Response UPIU
> with response_upiu_length.
>
> Reported-by: KOBAYASHI Yoshitake <yoshitake.kobayashi@toshiba.co.jp>
> Reviewed-by: Vinayak Holikatti <vinholikatti@gmail.com>
> Signed-off-by: Santosh Y <santoshsy@gmail.com>

Hi Santosh.
I also checked it on UFS specification. Looks good to me.
Reviewed-by: Namjae Jeon <linkinjeon@gmail.com>