[PATCH v4 0/4] scsi: ufs: cleanup ufs initialization

[PATCH v4 0/4] scsi: ufs: cleanup ufs initialization

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Cleanup UFS descriptor length initialization, and delete unnecessary code.

Changelog:

v2 - v3:
    1. Fix typo in the commit message (Avri Altman & Bart van Assche)
    2. Delete ufshcd_init_desc_sizes() in patch 3/4 (Stanley Chu)
    3. Remove max_t() and buff_len in patch 1/4 (Bart van Assche)
    4. Add patch 4/4

v1 - v2:
    1. Split patch
    2. Fix one compiling WARNING (Reported-by: kbuild test robot <lkp@intel.com>)

Bean Huo (4):
  scsi: ufs: remove max_t in ufs_get_device_desc
  scsi: ufs: delete ufshcd_read_desc()
  scsi: ufs: cleanup ufs initialization path
  scsi: ufs: add compatibility with 3.1 UFS unit descriptor length

 drivers/scsi/ufs/ufs.h     |  11 +-
 drivers/scsi/ufs/ufs_bsg.c |   5 +-
 drivers/scsi/ufs/ufshcd.c  | 200 ++++++++-----------------------------
 drivers/scsi/ufs/ufshcd.h  |  16 +--
 4 files changed, 50 insertions(+), 182 deletions(-)

-- 
2.17.1

[PATCH v4 1/4] scsi: ufs: remove max_t in ufs_get_device_desc

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

For the UFS device, the maximum descriptor size is 255, max_t called
in ufs_get_device_desc() is useless.

Signed-off-by: Bart van Assche <bvanassche@acm.org>
Signed-off-by: Bean Huo <beanhuo@micron.com>
Acked-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/scsi/ufs/ufshcd.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index aca50ed39844..f57acfbf9d60 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6876,14 +6876,11 @@ static void ufs_fixup_device_setup(struct ufs_hba *hba)
 static int ufs_get_device_desc(struct ufs_hba *hba)
 {
 	int err;
-	size_t buff_len;
 	u8 model_index;
 	u8 *desc_buf;
 	struct ufs_dev_info *dev_info = &hba->dev_info;
 
-	buff_len = max_t(size_t, hba->desc_size.dev_desc,
-			 QUERY_DESC_MAX_SIZE + 1);
-	desc_buf = kmalloc(buff_len, GFP_KERNEL);
+	desc_buf = kmalloc(QUERY_DESC_MAX_SIZE, GFP_KERNEL);
 	if (!desc_buf) {
 		err = -ENOMEM;
 		goto out;
-- 
2.17.1

Re: [PATCH v4 1/4] scsi: ufs: remove max_t in ufs_get_device_desc

[Stanley Chu]

On Fri, 2020-05-29 at 18:40 +0200, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> For the UFS device, the maximum descriptor size is 255, max_t called
> in ufs_get_device_desc() is useless.
> 
> Signed-off-by: Bart van Assche <bvanassche@acm.org>
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> Acked-by: Avri Altman <avri.altman@wdc.com>

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>

[PATCH v4 2/4] scsi: ufs: delete ufshcd_read_desc()

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Delete ufshcd_read_desc(). Instead, let caller directly call
ufshcd_read_desc_param().

Signed-off-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Avri Altman <avri.altman@wdc.com>
Reviewed-by: Bart van Assche <bvanassche@acm.org>
Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufshcd.c | 27 ++++++++-------------------
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index f57acfbf9d60..f7e8bfefe3d4 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3221,16 +3221,6 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
 	return ret;
 }
 
-static inline int ufshcd_read_desc(struct ufs_hba *hba,
-				   enum desc_idn desc_id,
-				   int desc_index,
-				   void *buf,
-				   u32 size)
-{
-	return ufshcd_read_desc_param(hba, desc_id, desc_index, 0, buf, size);
-}
-
-
 /**
  * struct uc_string_id - unicode string
  *
@@ -3278,9 +3268,8 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, u8 desc_index,
 	if (!uc_str)
 		return -ENOMEM;
 
-	ret = ufshcd_read_desc(hba, QUERY_DESC_IDN_STRING,
-			       desc_index, uc_str,
-			       QUERY_DESC_MAX_SIZE);
+	ret = ufshcd_read_desc_param(hba, QUERY_DESC_IDN_STRING, desc_index, 0,
+				     (u8 *)uc_str, QUERY_DESC_MAX_SIZE);
 	if (ret < 0) {
 		dev_err(hba->dev, "Reading String Desc failed after %d retries. err = %d\n",
 			QUERY_REQ_RETRIES, ret);
@@ -6684,8 +6673,8 @@ static void ufshcd_set_active_icc_lvl(struct ufs_hba *hba)
 	if (!desc_buf)
 		return;
 
-	ret = ufshcd_read_desc(hba, QUERY_DESC_IDN_POWER, 0,
-			desc_buf, buff_len);
+	ret = ufshcd_read_desc_param(hba, QUERY_DESC_IDN_POWER, 0, 0,
+				     desc_buf, buff_len);
 	if (ret) {
 		dev_err(hba->dev,
 			"%s: Failed reading power descriptor.len = %d ret = %d",
@@ -6886,8 +6875,8 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
 		goto out;
 	}
 
-	err = ufshcd_read_desc(hba, QUERY_DESC_IDN_DEVICE, 0, desc_buf,
-			hba->desc_size.dev_desc);
+	err = ufshcd_read_desc_param(hba, QUERY_DESC_IDN_DEVICE, 0, 0, desc_buf,
+				     hba->desc_size.dev_desc);
 	if (err) {
 		dev_err(hba->dev, "%s: Failed reading Device Desc. err = %d\n",
 			__func__, err);
@@ -7168,8 +7157,8 @@ static int ufshcd_device_geo_params_init(struct ufs_hba *hba)
 		goto out;
 	}
 
-	err = ufshcd_read_desc(hba, QUERY_DESC_IDN_GEOMETRY, 0,
-			desc_buf, buff_len);
+	err = ufshcd_read_desc_param(hba, QUERY_DESC_IDN_GEOMETRY, 0, 0,
+				     desc_buf, buff_len);
 	if (err) {
 		dev_err(hba->dev, "%s: Failed reading Geometry Desc. err = %d\n",
 				__func__, err);
-- 
2.17.1

[PATCH v4 3/4] scsi: ufs: cleanup ufs initialization path

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

At UFS initialization stage, to get the length of the descriptor,
ufshcd_read_desc_length() being called 6 times. Instead, we will
capture the descriptor size the first time  we'll read it.

Delete unnecessary redundant code, remove ufshcd_read_desc_length(),
ufshcd_init_desc_sizes(), and boost UFS initialization.

Signed-off-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Bart van Assche <bvanassche@acm.org>
Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufs.h     |  10 ---
 drivers/scsi/ufs/ufs_bsg.c |   5 +-
 drivers/scsi/ufs/ufshcd.c  | 165 +++++++------------------------------
 drivers/scsi/ufs/ufshcd.h  |  16 +---
 4 files changed, 36 insertions(+), 160 deletions(-)

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index fadba3a3bbcd..6548ef102eb9 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -200,16 +200,6 @@ enum desc_header_offset {
 	QUERY_DESC_DESC_TYPE_OFFSET	= 0x01,
 };
 
-enum ufs_desc_def_size {
-	QUERY_DESC_DEVICE_DEF_SIZE		= 0x59,
-	QUERY_DESC_CONFIGURATION_DEF_SIZE	= 0x90,
-	QUERY_DESC_UNIT_DEF_SIZE		= 0x2D,
-	QUERY_DESC_INTERCONNECT_DEF_SIZE	= 0x06,
-	QUERY_DESC_GEOMETRY_DEF_SIZE		= 0x48,
-	QUERY_DESC_POWER_DEF_SIZE		= 0x62,
-	QUERY_DESC_HEALTH_DEF_SIZE		= 0x25,
-};
-
 /* Unit descriptor parameters offsets in bytes*/
 enum unit_desc_param {
 	UNIT_DESC_PARAM_LEN			= 0x0,
diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c
index 53dd87628cbe..27f54615ee84 100644
--- a/drivers/scsi/ufs/ufs_bsg.c
+++ b/drivers/scsi/ufs/ufs_bsg.c
@@ -11,13 +11,12 @@ static int ufs_bsg_get_query_desc_size(struct ufs_hba *hba, int *desc_len,
 {
 	int desc_size = be16_to_cpu(qr->length);
 	int desc_id = qr->idn;
-	int ret;
 
 	if (desc_size <= 0)
 		return -EINVAL;
 
-	ret = ufshcd_map_desc_id_to_length(hba, desc_id, desc_len);
-	if (ret || !*desc_len)
+	ufshcd_map_desc_id_to_length(hba, desc_id, desc_len);
+	if (!*desc_len)
 		return -EINVAL;
 
 	*desc_len = min_t(int, *desc_len, desc_size);
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index f7e8bfefe3d4..951e52babf65 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3052,95 +3052,32 @@ int ufshcd_query_descriptor_retry(struct ufs_hba *hba,
 	return err;
 }
 
-/**
- * ufshcd_read_desc_length - read the specified descriptor length from header
- * @hba: Pointer to adapter instance
- * @desc_id: descriptor idn value
- * @desc_index: descriptor index
- * @desc_length: pointer to variable to read the length of descriptor
- *
- * Return 0 in case of success, non-zero otherwise
- */
-static int ufshcd_read_desc_length(struct ufs_hba *hba,
-	enum desc_idn desc_id,
-	int desc_index,
-	int *desc_length)
-{
-	int ret;
-	u8 header[QUERY_DESC_HDR_SIZE];
-	int header_len = QUERY_DESC_HDR_SIZE;
-
-	if (desc_id >= QUERY_DESC_IDN_MAX)
-		return -EINVAL;
-
-	ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_READ_DESC,
-					desc_id, desc_index, 0, header,
-					&header_len);
-
-	if (ret) {
-		dev_err(hba->dev, "%s: Failed to get descriptor header id %d",
-			__func__, desc_id);
-		return ret;
-	} else if (desc_id != header[QUERY_DESC_DESC_TYPE_OFFSET]) {
-		dev_warn(hba->dev, "%s: descriptor header id %d and desc_id %d mismatch",
-			__func__, header[QUERY_DESC_DESC_TYPE_OFFSET],
-			desc_id);
-		ret = -EINVAL;
-	}
-
-	*desc_length = header[QUERY_DESC_LENGTH_OFFSET];
-	return ret;
-
-}
-
 /**
  * ufshcd_map_desc_id_to_length - map descriptor IDN to its length
  * @hba: Pointer to adapter instance
  * @desc_id: descriptor idn value
  * @desc_len: mapped desc length (out)
  *
- * Return 0 in case of success, non-zero otherwise
  */
-int ufshcd_map_desc_id_to_length(struct ufs_hba *hba,
-	enum desc_idn desc_id, int *desc_len)
+void ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn desc_id,
+				  int *desc_len)
 {
-	switch (desc_id) {
-	case QUERY_DESC_IDN_DEVICE:
-		*desc_len = hba->desc_size.dev_desc;
-		break;
-	case QUERY_DESC_IDN_POWER:
-		*desc_len = hba->desc_size.pwr_desc;
-		break;
-	case QUERY_DESC_IDN_GEOMETRY:
-		*desc_len = hba->desc_size.geom_desc;
-		break;
-	case QUERY_DESC_IDN_CONFIGURATION:
-		*desc_len = hba->desc_size.conf_desc;
-		break;
-	case QUERY_DESC_IDN_UNIT:
-		*desc_len = hba->desc_size.unit_desc;
-		break;
-	case QUERY_DESC_IDN_INTERCONNECT:
-		*desc_len = hba->desc_size.interc_desc;
-		break;
-	case QUERY_DESC_IDN_STRING:
-		*desc_len = QUERY_DESC_MAX_SIZE;
-		break;
-	case QUERY_DESC_IDN_HEALTH:
-		*desc_len = hba->desc_size.hlth_desc;
-		break;
-	case QUERY_DESC_IDN_RFU_0:
-	case QUERY_DESC_IDN_RFU_1:
+	if (desc_id == QUERY_DESC_IDN_RFU_0 || desc_id == QUERY_DESC_IDN_RFU_1)
 		*desc_len = 0;
-		break;
-	default:
-		*desc_len = 0;
-		return -EINVAL;
-	}
-	return 0;
+	else
+		*desc_len = hba->desc_size[desc_id];
 }
 EXPORT_SYMBOL(ufshcd_map_desc_id_to_length);
 
+static void ufshcd_update_desc_length(struct ufs_hba *hba,
+				      enum desc_idn desc_id,
+				      unsigned char desc_len)
+{
+	if (hba->desc_size[desc_id] == QUERY_DESC_MAX_SIZE &&
+	    desc_id != QUERY_DESC_IDN_STRING)
+		hba->desc_size[desc_id] = desc_len;
+}
+
 /**
  * ufshcd_read_desc_param - read the specified descriptor parameter
  * @hba: Pointer to adapter instance
@@ -3168,16 +3105,11 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
 	if (desc_id >= QUERY_DESC_IDN_MAX || !param_size)
 		return -EINVAL;
 
-	/* Get the max length of descriptor from structure filled up at probe
-	 * time.
-	 */
-	ret = ufshcd_map_desc_id_to_length(hba, desc_id, &buff_len);
-
-	/* Sanity checks */
-	if (ret || !buff_len) {
-		dev_err(hba->dev, "%s: Failed to get full descriptor length",
-			__func__);
-		return ret;
+	/* Get the length of descriptor */
+	ufshcd_map_desc_id_to_length(hba, desc_id, &buff_len);
+	if (!buff_len) {
+		dev_err(hba->dev, "%s: Failed to get desc length", __func__);
+		return -EINVAL;
 	}
 
 	/* Check whether we need temp memory */
@@ -3209,6 +3141,9 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
 		goto out;
 	}
 
+	ufshcd_update_desc_length(hba, desc_id,
+				  desc_buf[QUERY_DESC_LENGTH_OFFSET]);
+
 	/* Check wherher we will not copy more data, than available */
 	if (is_kmalloc && param_size > buff_len)
 		param_size = buff_len;
@@ -6665,7 +6600,7 @@ static u32 ufshcd_find_max_sup_active_icc_level(struct ufs_hba *hba,
 static void ufshcd_set_active_icc_lvl(struct ufs_hba *hba)
 {
 	int ret;
-	int buff_len = hba->desc_size.pwr_desc;
+	int buff_len = hba->desc_size[QUERY_DESC_IDN_POWER];
 	u8 *desc_buf;
 	u32 icc_level;
 
@@ -6783,7 +6718,8 @@ static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf)
 	if (!ufshcd_is_wb_allowed(hba))
 		return;
 
-	if (hba->desc_size.dev_desc < DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 4)
+	if (hba->desc_size[QUERY_DESC_IDN_DEVICE] <
+	    DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 4)
 		goto wb_disabled;
 
 	hba->dev_info.d_ext_ufs_feature_sup =
@@ -6876,7 +6812,7 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
 	}
 
 	err = ufshcd_read_desc_param(hba, QUERY_DESC_IDN_DEVICE, 0, 0, desc_buf,
-				     hba->desc_size.dev_desc);
+				     hba->desc_size[QUERY_DESC_IDN_DEVICE]);
 	if (err) {
 		dev_err(hba->dev, "%s: Failed reading Device Desc. err = %d\n",
 			__func__, err);
@@ -7104,53 +7040,13 @@ static void ufshcd_clear_dbg_ufs_stats(struct ufs_hba *hba)
 	hba->req_abort_count = 0;
 }
 
-static void ufshcd_init_desc_sizes(struct ufs_hba *hba)
-{
-	int err;
-
-	err = ufshcd_read_desc_length(hba, QUERY_DESC_IDN_DEVICE, 0,
-		&hba->desc_size.dev_desc);
-	if (err)
-		hba->desc_size.dev_desc = QUERY_DESC_DEVICE_DEF_SIZE;
-
-	err = ufshcd_read_desc_length(hba, QUERY_DESC_IDN_POWER, 0,
-		&hba->desc_size.pwr_desc);
-	if (err)
-		hba->desc_size.pwr_desc = QUERY_DESC_POWER_DEF_SIZE;
-
-	err = ufshcd_read_desc_length(hba, QUERY_DESC_IDN_INTERCONNECT, 0,
-		&hba->desc_size.interc_desc);
-	if (err)
-		hba->desc_size.interc_desc = QUERY_DESC_INTERCONNECT_DEF_SIZE;
-
-	err = ufshcd_read_desc_length(hba, QUERY_DESC_IDN_CONFIGURATION, 0,
-		&hba->desc_size.conf_desc);
-	if (err)
-		hba->desc_size.conf_desc = QUERY_DESC_CONFIGURATION_DEF_SIZE;
-
-	err = ufshcd_read_desc_length(hba, QUERY_DESC_IDN_UNIT, 0,
-		&hba->desc_size.unit_desc);
-	if (err)
-		hba->desc_size.unit_desc = QUERY_DESC_UNIT_DEF_SIZE;
-
-	err = ufshcd_read_desc_length(hba, QUERY_DESC_IDN_GEOMETRY, 0,
-		&hba->desc_size.geom_desc);
-	if (err)
-		hba->desc_size.geom_desc = QUERY_DESC_GEOMETRY_DEF_SIZE;
-
-	err = ufshcd_read_desc_length(hba, QUERY_DESC_IDN_HEALTH, 0,
-		&hba->desc_size.hlth_desc);
-	if (err)
-		hba->desc_size.hlth_desc = QUERY_DESC_HEALTH_DEF_SIZE;
-}
-
 static int ufshcd_device_geo_params_init(struct ufs_hba *hba)
 {
 	int err;
 	size_t buff_len;
 	u8 *desc_buf;
 
-	buff_len = hba->desc_size.geom_desc;
+	buff_len = hba->desc_size[QUERY_DESC_IDN_GEOMETRY];
 	desc_buf = kmalloc(buff_len, GFP_KERNEL);
 	if (!desc_buf) {
 		err = -ENOMEM;
@@ -7246,13 +7142,14 @@ static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba)
 static int ufshcd_device_params_init(struct ufs_hba *hba)
 {
 	bool flag;
-	int ret;
+	int ret, i;
 
 	/* Clear any previous UFS device information */
 	memset(&hba->dev_info, 0, sizeof(hba->dev_info));
 
-	/* Init check for device descriptor sizes */
-	ufshcd_init_desc_sizes(hba);
+	/* Init device descriptor sizes */
+	for (i = 0; i < QUERY_DESC_IDN_MAX; i++)
+		hba->desc_size[i] = QUERY_DESC_MAX_SIZE;
 
 	/* Init UFS geometry descriptor related parameters */
 	ret = ufshcd_device_geo_params_init(hba);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index e3dfb48e669e..5ea090d82ddc 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -236,16 +236,6 @@ struct ufs_dev_cmd {
 	struct ufs_query query;
 };
 
-struct ufs_desc_size {
-	int dev_desc;
-	int pwr_desc;
-	int geom_desc;
-	int interc_desc;
-	int unit_desc;
-	int conf_desc;
-	int hlth_desc;
-};
-
 /**
  * struct ufs_clk_info - UFS clock related info
  * @list: list headed by hba->clk_list_head
@@ -738,7 +728,7 @@ struct ufs_hba {
 	bool is_urgent_bkops_lvl_checked;
 
 	struct rw_semaphore clk_scaling_lock;
-	struct ufs_desc_size desc_size;
+	unsigned char desc_size[QUERY_DESC_IDN_MAX];
 	atomic_t scsi_block_reqs_cnt;
 
 	struct device		bsg_dev;
@@ -975,8 +965,8 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, u8 desc_index,
 int ufshcd_hold(struct ufs_hba *hba, bool async);
 void ufshcd_release(struct ufs_hba *hba);
 
-int ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn desc_id,
-	int *desc_length);
+void ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn desc_id,
+				  int *desc_length);
 
 u32 ufshcd_get_local_unipro_ver(struct ufs_hba *hba);
 
-- 
2.17.1

RE: [PATCH v4 3/4] scsi: ufs: cleanup ufs initialization path

[Avri Altman]

> -       case QUERY_DESC_IDN_RFU_0:
> -       case QUERY_DESC_IDN_RFU_1:
You forgot to check that desc_id < QUERY_DESC_IDN_MAX

> +       if (desc_id == QUERY_DESC_IDN_RFU_0 || desc_id ==
> QUERY_DESC_IDN_RFU_1)
>                 *desc_len = 0;
> -               break;
> -       default:
> -               *desc_len = 0;
> -               return -EINVAL;
> -       }
> -       return 0;
> +       else
> +               *desc_len = hba->desc_size[desc_id];
>  }
>  EXPORT_SYMBOL(ufshcd_map_desc_id_to_length);
> 
> +static void ufshcd_update_desc_length(struct ufs_hba *hba,
> +                                     enum desc_idn desc_id,
> +                                     unsigned char desc_len)
> +{
> +       if (hba->desc_size[desc_id] == QUERY_DESC_MAX_SIZE &&
> +           desc_id != QUERY_DESC_IDN_STRING)
> +               hba->desc_size[desc_id] = desc_len;
> +}
> +
>  /**
>   * ufshcd_read_desc_param - read the specified descriptor parameter
>   * @hba: Pointer to adapter instance
> @@ -3168,16 +3105,11 @@ int ufshcd_read_desc_param(struct ufs_hba
> *hba,
>         if (desc_id >= QUERY_DESC_IDN_MAX || !param_size)
>                 return -EINVAL;
> 
> -       /* Get the max length of descriptor from structure filled up at probe
> -        * time.
> -        */
> -       ret = ufshcd_map_desc_id_to_length(hba, desc_id, &buff_len);
> -
> -       /* Sanity checks */
> -       if (ret || !buff_len) {
> -               dev_err(hba->dev, "%s: Failed to get full descriptor length",
> -                       __func__);
> -               return ret;
> +       /* Get the length of descriptor */
> +       ufshcd_map_desc_id_to_length(hba, desc_id, &buff_len);
> +       if (!buff_len) {
> +               dev_err(hba->dev, "%s: Failed to get desc length", __func__);
> +               return -EINVAL;
>         }
> 
>         /* Check whether we need temp memory */
The first time we are reading the descriptor, we no longer can rely on its true size.
So for this check, buff_len is 256 and kmalloc will always happen. 
Do you think that this check is still relevant?

/* Check whether we need temp memory */
        if (param_offset != 0 || param_size < buff_len) {
                desc_buf = kmalloc(buff_len, GFP_KERNEL);
                if (!desc_buf)
                        return -ENOMEM;
        } else {
                desc_buf = param_read_buf;
                is_kmalloc = false;
        }


> @@ -3209,6 +3141,9 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
>                 goto out;
>         }
> 
> +       ufshcd_update_desc_length(hba, desc_id,
> +                                 desc_buf[QUERY_DESC_LENGTH_OFFSET]);
> +
>         /* Check wherher we will not copy more data, than available */
>         if (is_kmalloc && param_size > buff_len)
>                 param_size = buff_len;
And here, we might want to update buff_len to hold the true descriptor size,
Before checking the copy-back buffer.

Thanks,
Avri

Re: [PATCH v4 3/4] scsi: ufs: cleanup ufs initialization path

[Bean Huo]

Hi Avri
thanks your review.

On Sat, 2020-05-30 at 06:37 +0000, Avri Altman wrote:
> > -       case QUERY_DESC_IDN_RFU_0:
> > -       case QUERY_DESC_IDN_RFU_1:
> 
> You forgot to check that desc_id < QUERY_DESC_IDN_MAX

I deleted it since I saw the caller has checked this.
I will add back.

>  
> 
> > +       if (desc_id == QUERY_DESC_IDN_RFU_0 || desc_id ==
> > QUERY_DESC_IDN_RFU_1)
> >                 *desc_len = 0;
> > -               break;
> > -       default:
> > -               *desc_len = 0;
> > -               return -EINVAL;
> > -       }
> > -       return 0;
> > +       else
> > +               *desc_len = hba->desc_size[desc_id];
> >  }
> >  EXPORT_SYMBOL(ufshcd_map_desc_id_to_length);
> > 
> > +static void ufshcd_update_desc_length(struct ufs_hba *hba,
> > +                                     enum desc_idn desc_id,
> > +                                     unsigned char desc_len)
> > +{
> > +       if (hba->desc_size[desc_id] == QUERY_DESC_MAX_SIZE &&
> > +           desc_id != QUERY_DESC_IDN_STRING)
> > +               hba->desc_size[desc_id] = desc_len;
> > +}
> > +
> >  /**
> >   * ufshcd_read_desc_param - read the specified descriptor
> > parameter
> >   * @hba: Pointer to adapter instance
> > @@ -3168,16 +3105,11 @@ int ufshcd_read_desc_param(struct ufs_hba
> > *hba,
> >         if (desc_id >= QUERY_DESC_IDN_MAX || !param_size)
> >                 return -EINVAL;
> > 
> > -       /* Get the max length of descriptor from structure filled
> > up at probe
> > -        * time.
> > -        */
> > -       ret = ufshcd_map_desc_id_to_length(hba, desc_id,
> > &buff_len);
> > -
> > -       /* Sanity checks */
> > -       if (ret || !buff_len) {
> > -               dev_err(hba->dev, "%s: Failed to get full
> > descriptor length",
> > -                       __func__);
> > -               return ret;
> > +       /* Get the length of descriptor */
> > +       ufshcd_map_desc_id_to_length(hba, desc_id, &buff_len);
> > +       if (!buff_len) {
> > +               dev_err(hba->dev, "%s: Failed to get desc length",
> > __func__);
> > +               return -EINVAL;
> >         }
> > 
> >         /* Check whether we need temp memory */
> 
> The first time we are reading the descriptor, we no longer can rely
> on its true size.
> So for this check, buff_len is 256 and kmalloc will always happen. 
> Do you think that this check is still relevant?

you are right, I will delete this redundat code.

> 
> /* Check whether we need temp memory */
>         if (param_offset != 0 || param_size < buff_len) {
>                 desc_buf = kmalloc(buff_len, GFP_KERNEL);
>                 if (!desc_buf)
>                         return -ENOMEM;
>         } else {
>                 desc_buf = param_read_buf;
>                 is_kmalloc = false;
>         }
> 
> 
> > @@ -3209,6 +3141,9 @@ int ufshcd_read_desc_param(struct ufs_hba
> > *hba,
> >                 goto out;
> >         }
> > 
> > +       ufshcd_update_desc_length(hba, desc_id,
> > +                                 desc_buf[QUERY_DESC_LENGTH_OFFSET
> > ]);
> > +
> >         /* Check wherher we will not copy more data, than available
> > */
> >         if (is_kmalloc && param_size > buff_len)
> >                 param_size = buff_len;
> 
> And here, we might want to update buff_len to hold the true
> descriptor size,
> Before checking the copy-back buffer.
> 

yes, buff_len should be updated here. will change it.

thanks 
Bean

Re: [PATCH v4 3/4] scsi: ufs: cleanup ufs initialization path

[Bean Huo]

On Sat, 2020-05-30 at 06:37 +0000, Avri Altman wrote:
> > +       /* Get the length of descriptor */
> > +       ufshcd_map_desc_id_to_length(hba, desc_id, &buff_len);
> > +       if (!buff_len) {
> > +               dev_err(hba->dev, "%s: Failed to get desc length",
> > __func__);
> > +               return -EINVAL;
> >          }
> > 
> >          /* Check whether we need temp memory */
> 
> The first time we are reading the descriptor, we no longer can rely
> on its true size.
> So for this check, buff_len is 256 and kmalloc will always happen. 
> Do you think that this check is still relevant?
> 
> /* Check whether we need temp memory */
>         if (param_offset != 0 || param_size < buff_len) {
>                 desc_buf = kmalloc(buff_len, GFP_KERNEL);
>                 if (!desc_buf)
>                         return -ENOMEM;
>         } else {
>                 desc_buf = param_read_buf;
>                 is_kmalloc = false;
>         }

Avri
I found this checkup is still needed since LU descriptor read will
multiple enter this function. so I didn't delete it in the new version
patch.

thanks,
Bean

[PATCH v4 4/4] scsi: ufs: add compatibility with 3.1 UFS unit descriptor length

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

For UFS 3.1, the normal unit descriptor is 10 bytes larger
than the RPMB unit, however, both descriptors share the same
desc_idn, to cover both unit descriptors with one length, we
choose the normal unit descriptor length by desc_index.

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufs.h    |  1 +
 drivers/scsi/ufs/ufshcd.c | 11 ++++++++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 6548ef102eb9..332ae09e6238 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -63,6 +63,7 @@
 #define UFS_UPIU_MAX_UNIT_NUM_ID	0x7F
 #define UFS_MAX_LUNS		(SCSI_W_LUN_BASE + UFS_UPIU_MAX_UNIT_NUM_ID)
 #define UFS_UPIU_WLUN_ID	(1 << 7)
+#define UFS_RPMB_UNIT		0xC4
 
 /* WriteBooster buffer is available only for the logical unit from 0 to 7 */
 #define UFS_UPIU_MAX_WB_LUN_ID	8
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 951e52babf65..3cdc585d0095 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3070,11 +3070,16 @@ void ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn desc_id,
 EXPORT_SYMBOL(ufshcd_map_desc_id_to_length);
 
 static void ufshcd_update_desc_length(struct ufs_hba *hba,
-				      enum desc_idn desc_id,
+				      enum desc_idn desc_id, int desc_index,
 				      unsigned char desc_len)
 {
 	if (hba->desc_size[desc_id] == QUERY_DESC_MAX_SIZE &&
-	    desc_id != QUERY_DESC_IDN_STRING)
+	    desc_id != QUERY_DESC_IDN_STRING && desc_index != UFS_RPMB_UNIT)
+		/* For UFS 3.1, the normal unit descriptor is 10 bytes larger
+		 * than the RPMB unit, however, both descriptors share the same
+		 * desc_idn, to cover both unit descriptors with one length, we
+		 * choose the normal unit descriptor length by desc_index.
+		 */
 		hba->desc_size[desc_id] = desc_len;
 }
 
@@ -3141,7 +3146,7 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
 		goto out;
 	}
 
-	ufshcd_update_desc_length(hba, desc_id,
+	ufshcd_update_desc_length(hba, desc_id, desc_index,
 				  desc_buf[QUERY_DESC_LENGTH_OFFSET]);
 
 	/* Check wherher we will not copy more data, than available */
-- 
2.17.1

RE: [PATCH v4 4/4] scsi: ufs: add compatibility with 3.1 UFS unit descriptor length

[Avri Altman]

 
> 
> From: Bean Huo <beanhuo@micron.com>
> 
> For UFS 3.1, the normal unit descriptor is 10 bytes larger
> than the RPMB unit, however, both descriptors share the same
> desc_idn, to cover both unit descriptors with one length, we
> choose the normal unit descriptor length by desc_index.
This is not what your code is doing.
For RPMB - desc size will not be 0x2d but remain 256.

Your strategy is still correct IMO - if you assign the larger size,
The device will not reply with error, but with the proper buffer.

You can also rely that reading the rpmb unit descriptor will not happen before
Reading regular luns, because this is happening in the first slave_alloc.
 
Hence, I think you can drop the extra if, and just add the comment.

Thanks,
Avri 

> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/scsi/ufs/ufs.h    |  1 +
>  drivers/scsi/ufs/ufshcd.c | 11 ++++++++---
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index 6548ef102eb9..332ae09e6238 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -63,6 +63,7 @@
>  #define UFS_UPIU_MAX_UNIT_NUM_ID       0x7F
>  #define UFS_MAX_LUNS           (SCSI_W_LUN_BASE +
> UFS_UPIU_MAX_UNIT_NUM_ID)
>  #define UFS_UPIU_WLUN_ID       (1 << 7)
> +#define UFS_RPMB_UNIT          0xC4
> 
>  /* WriteBooster buffer is available only for the logical unit from 0 to 7 */
>  #define UFS_UPIU_MAX_WB_LUN_ID 8
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 951e52babf65..3cdc585d0095 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -3070,11 +3070,16 @@ void ufshcd_map_desc_id_to_length(struct
> ufs_hba *hba, enum desc_idn desc_id,
>  EXPORT_SYMBOL(ufshcd_map_desc_id_to_length);
> 
>  static void ufshcd_update_desc_length(struct ufs_hba *hba,
> -                                     enum desc_idn desc_id,
> +                                     enum desc_idn desc_id, int desc_index,
>                                       unsigned char desc_len)
>  {
>         if (hba->desc_size[desc_id] == QUERY_DESC_MAX_SIZE &&
> -           desc_id != QUERY_DESC_IDN_STRING)
> +           desc_id != QUERY_DESC_IDN_STRING && desc_index !=
> UFS_RPMB_UNIT)
> +               /* For UFS 3.1, the normal unit descriptor is 10 bytes larger
> +                * than the RPMB unit, however, both descriptors share the same
> +                * desc_idn, to cover both unit descriptors with one length, we
> +                * choose the normal unit descriptor length by desc_index.
> +                */
>                 hba->desc_size[desc_id] = desc_len;
>  }
> 
> @@ -3141,7 +3146,7 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
>                 goto out;
>         }
> 
> -       ufshcd_update_desc_length(hba, desc_id,
> +       ufshcd_update_desc_length(hba, desc_id, desc_index,
>                                   desc_buf[QUERY_DESC_LENGTH_OFFSET]);
> 
>         /* Check wherher we will not copy more data, than available */
> --
> 2.17.1

Re: [PATCH v4 4/4] scsi: ufs: add compatibility with 3.1 UFS unit descriptor length

[Bean Huo]

Avri,


On Sat, 2020-05-30 at 06:56 +0000, Avri Altman wrote:
>  
> > 
> > From: Bean Huo <beanhuo@micron.com>
> > 
> > For UFS 3.1, the normal unit descriptor is 10 bytes larger
> > than the RPMB unit, however, both descriptors share the same
> > desc_idn, to cover both unit descriptors with one length, we
> > choose the normal unit descriptor length by desc_index.
> 
> This is not what your code is doing.
> For RPMB - desc size will not be 0x2d but remain 256.

sorry, I'm afraid I didn't quite get your point here.
would you go over it again in detail please?


> 
> Your strategy is still correct IMO - if you assign the larger size,
> The device will not reply with error, but with the proper buffer.
> 
> You can also rely that reading the rpmb unit descriptor will not
> happen before
> Reading regular luns, because this is happening in the first
> slave_alloc.
>  

On my side, I saw the Well-know LU descriptor is read before
regulaer/normal LU descritptor. see ufshcd_add_lus();

I did further debug to verify my patch, and the unit descriptor read
sequence:

1. read RPMB descriptor: desc_id 2, desc_index 0xc4
2. read LU 0 descriptor: desc_id 2, desc_index 0
3. read LU 0 descriptor: desc_id 2, desc_index 1
4. read LU 0 descriptor: desc_id 2, desc_index 2
5. read LU 0 descriptor: desc_id 2, desc_index 4
....

> Hence, I think you can drop the extra if, and just add the comment.
> 

so, this 'if' is still needed. otherwise, LU descriptor length will be
initialized as 0x23(RPMB descriptor length).

if I am wrong, please correct me.
thanks,

Bean

RE: [PATCH v4 4/4] scsi: ufs: add compatibility with 3.1 UFS unit descriptor length

[Avri Altman]

 
> Avri,
> 
> 
> On Sat, 2020-05-30 at 06:56 +0000, Avri Altman wrote:
> >
> > >
> > > From: Bean Huo <beanhuo@micron.com>
> > >
> > > For UFS 3.1, the normal unit descriptor is 10 bytes larger
> > > than the RPMB unit, however, both descriptors share the same
> > > desc_idn, to cover both unit descriptors with one length, we
> > > choose the normal unit descriptor length by desc_index.
> >
> > This is not what your code is doing.
> > For RPMB - desc size will not be 0x2d but remain 256.
> 
> sorry, I'm afraid I didn't quite get your point here.
> would you go over it again in detail please?
> 
> 
> >
> > Your strategy is still correct IMO - if you assign the larger size,
> > The device will not reply with error, but with the proper buffer.
> >
> > You can also rely that reading the rpmb unit descriptor will not
> > happen before
> > Reading regular luns, because this is happening in the first
> > slave_alloc.
> >
> 
> On my side, I saw the Well-know LU descriptor is read before
> regulaer/normal LU descritptor. see ufshcd_add_lus();
> 
> I did further debug to verify my patch, and the unit descriptor read
> sequence:
> 
> 1. read RPMB descriptor: desc_id 2, desc_index 0xc4
> 2. read LU 0 descriptor: desc_id 2, desc_index 0
> 3. read LU 0 descriptor: desc_id 2, desc_index 1
> 4. read LU 0 descriptor: desc_id 2, desc_index 2
> 5. read LU 0 descriptor: desc_id 2, desc_index 4
> ....
> 
> > Hence, I think you can drop the extra if, and just add the comment.
> >
> 
> so, this 'if' is still needed. otherwise, LU descriptor length will be
> initialized as 0x23(RPMB descriptor length).
> 
Ahha, ok.  I see your point now,
Fair enough.

Reviewed-by: Avri Altman <avri.altman@wdc.com>