diff mbox series

[v5,4/4] ufs: core: Remove ufshcd_map_desc_id_to_length function

Message ID 1670763911-8695-5-git-send-email-Arthur.Simchaev@wdc.com
State Superseded
Headers show
Series ufs: core: Always read the descriptors with max length | expand

Commit Message

Arthur Simchaev Dec. 11, 2022, 1:05 p.m. UTC 
There shouldn't be any restriction of the descriptor size
(not the descriptor id for that matter) up to QUERY_DESC_MAX_SIZE.
According to the spec, the caller can use any descriptor size,
and it is up to the device to return the actual size.
Therefore there shouldn't be any sizes hardcoded in the kernel,
nor any need to cache it, hence ufshcd_map_desc_id_to_length function is
redundant. Always read the descriptors with QUERY_DESC_MAX_SIZE size.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Bean Huo <beanhuo@micron.com>
Suggested-by: Bean Huo <beanhuo@micron.com>
Signed-off-by: Arthur Simchaev <Arthur.Simchaev@wdc.com>
---
 drivers/ufs/core/ufs_bsg.c |  1 -
 drivers/ufs/core/ufshcd.c  | 23 +++++++++++------------
 2 files changed, 11 insertions(+), 13 deletions(-)

Comments

Bart Van Assche Dec. 12, 2022, 12:18 a.m. UTC | #1 
On 12/11/22 05:05, Arthur Simchaev wrote:
> There shouldn't be any restriction of the descriptor size
> (not the descriptor id for that matter) up to QUERY_DESC_MAX_SIZE.
> According to the spec, the caller can use any descriptor size,
> and it is up to the device to return the actual size.
> Therefore there shouldn't be any sizes hardcoded in the kernel,
> nor any need to cache it, hence ufshcd_map_desc_id_to_length function is
> redundant. Always read the descriptors with QUERY_DESC_MAX_SIZE size.
> 
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>

I have not yet replied with "Reviewed-by" to this patch so you are not
yet allowed to add my Reviewed-by tag to this patch.

> +	/* Update descriptor length */
> +	buff_len = desc_buf[QUERY_DESC_LENGTH_OFFSET];

Should a check be added here that desc_buf[QUERY_DESC_LENGTH_OFFSET]
<= QUERY_DESC_MAX_SIZE to protect against firmware bugs? Since
QUERY_DESC_MAX_SIZE == 255, adding BUILD_BUG_ON(QUERY_DESC_SIZE != 255)
and a comment may be sufficient.

Thanks,

Bart.
Arthur Simchaev Dec. 12, 2022, 9:10 a.m. UTC | #2 
> Should a check be added here that desc_buf[QUERY_DESC_LENGTH_OFFSET]
> <= QUERY_DESC_MAX_SIZE to protect against firmware bugs? Since
> QUERY_DESC_MAX_SIZE == 255, adding BUILD_BUG_ON(QUERY_DESC_SIZE !=
> 255)
> and a comment may be sufficient.

Sorry - my bad.
In the previous review you mentioned that the patch looks good to you, hence the "Review by".
Regarding your comment, we can do that, although I don't think we should cover for those FW basic bugs.
Please let me know that you prefer.

Regards
Arthur

> -----Original Message-----
> From: Bart Van Assche <bvanassche@acm.org>
> Sent: Monday, December 12, 2022 2:19 AM
> To: Arthur Simchaev <Arthur.Simchaev@wdc.com>;
> martin.petersen@oracle.com
> Cc: beanhuo@micron.com; linux-scsi@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v5 4/4] ufs: core: Remove ufshcd_map_desc_id_to_length
> function
> 
> CAUTION: This email originated from outside of Western Digital. Do not click
> on links or open attachments unless you recognize the sender and know that
> the content is safe.
> 
> 
> On 12/11/22 05:05, Arthur Simchaev wrote:
> > There shouldn't be any restriction of the descriptor size
> > (not the descriptor id for that matter) up to QUERY_DESC_MAX_SIZE.
> > According to the spec, the caller can use any descriptor size,
> > and it is up to the device to return the actual size.
> > Therefore there shouldn't be any sizes hardcoded in the kernel,
> > nor any need to cache it, hence ufshcd_map_desc_id_to_length function is
> > redundant. Always read the descriptors with QUERY_DESC_MAX_SIZE size.
> >
> > Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> 
> I have not yet replied with "Reviewed-by" to this patch so you are not
> yet allowed to add my Reviewed-by tag to this patch.
> 
> > +     /* Update descriptor length */
> > +     buff_len = desc_buf[QUERY_DESC_LENGTH_OFFSET];
> 
> Should a check be added here that desc_buf[QUERY_DESC_LENGTH_OFFSET]
> <= QUERY_DESC_MAX_SIZE to protect against firmware bugs? Since
> QUERY_DESC_MAX_SIZE == 255, adding BUILD_BUG_ON(QUERY_DESC_SIZE !=
> 255)
> and a comment may be sufficient.
> 
> Thanks,
> 
> Bart.
Bart Van Assche Dec. 12, 2022, 7:25 p.m. UTC | #3 
On 12/12/22 01:10, Arthur Simchaev wrote:
> Regarding your comment, we can do that, although I don't think we should cover for those FW basic bugs.
> Please let me know that you prefer.

I think this version is good enough. I will post my Reviewed-by.

Thanks,

Bart.
Bart Van Assche Dec. 12, 2022, 7:26 p.m. UTC | #4 
On 12/11/22 05:05, Arthur Simchaev wrote:
> There shouldn't be any restriction of the descriptor size
> (not the descriptor id for that matter) up to QUERY_DESC_MAX_SIZE.
> According to the spec, the caller can use any descriptor size,
> and it is up to the device to return the actual size.
> Therefore there shouldn't be any sizes hardcoded in the kernel,
> nor any need to cache it, hence ufshcd_map_desc_id_to_length function is
> redundant. Always read the descriptors with QUERY_DESC_MAX_SIZE size.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>
diff mbox series

Patch

diff --git a/drivers/ufs/core/ufs_bsg.c b/drivers/ufs/core/ufs_bsg.c
index 7eec38c..dc441ac 100644
--- a/drivers/ufs/core/ufs_bsg.c
+++ b/drivers/ufs/core/ufs_bsg.c
@@ -16,7 +16,6 @@  static int ufs_bsg_get_query_desc_size(struct ufs_hba *hba, int *desc_len,
 				       struct utp_upiu_query *qr)
 {
 	int desc_size = be16_to_cpu(qr->length);
-	int desc_id = qr->idn;
 
 	if (desc_size <= 0)
 		return -EINVAL;
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index b6ef92d3..7f89626 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -3395,12 +3395,6 @@  int ufshcd_read_desc_param(struct ufs_hba *hba,
 	if (desc_id >= QUERY_DESC_IDN_MAX || !param_size)
 		return -EINVAL;
 
-	if (param_offset >= buff_len) {
-		dev_err(hba->dev, "%s: Invalid offset 0x%x in descriptor IDN 0x%x, length 0x%x\n",
-			__func__, param_offset, desc_id, buff_len);
-		return -EINVAL;
-	}
-
 	/* Check whether we need temp memory */
 	if (param_offset != 0 || param_size < buff_len) {
 		desc_buf = kzalloc(buff_len, GFP_KERNEL);
@@ -3413,15 +3407,23 @@  int ufshcd_read_desc_param(struct ufs_hba *hba,
 
 	/* Request for full descriptor */
 	ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_READ_DESC,
-					desc_id, desc_index, 0,
-					desc_buf, &buff_len);
-
+					    desc_id, desc_index, 0,
+					    desc_buf, &buff_len);
 	if (ret) {
 		dev_err(hba->dev, "%s: Failed reading descriptor. desc_id %d, desc_index %d, param_offset %d, ret %d\n",
 			__func__, desc_id, desc_index, param_offset, ret);
 		goto out;
 	}
 
+	/* Update descriptor length */
+	buff_len = desc_buf[QUERY_DESC_LENGTH_OFFSET];
+
+	if (param_offset >= buff_len) {
+		dev_err(hba->dev, "%s: Invalid offset 0x%x in descriptor IDN 0x%x, length 0x%x\n",
+			__func__, param_offset, desc_id, buff_len);
+		return -EINVAL;
+	}
+
 	/* Sanity check */
 	if (desc_buf[QUERY_DESC_DESC_TYPE_OFFSET] != desc_id) {
 		dev_err(hba->dev, "%s: invalid desc_id %d in descriptor header\n",
@@ -3430,9 +3432,6 @@  int ufshcd_read_desc_param(struct ufs_hba *hba,
 		goto out;
 	}
 
-	/* Update descriptor length */
-	buff_len = desc_buf[QUERY_DESC_LENGTH_OFFSET];
-
 	if (is_kmalloc) {
 		/* Make sure we don't copy more data than available */
 		if (param_offset >= buff_len)