[PATCH v2 0/3] scsi: ufs: cleanup ufs initialization

[PATCH v2 0/3] scsi: ufs: cleanup ufs initialization

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

This patchset is to cleanup UFS descriptor access, and delete some
unnecessary code.

Changelog:

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

Bean Huo (3):
  scsi: ufs: remove max_t in ufs_get_device_desc
  scsi: ufs: delete ufshcd_read_desc()
  scsi: ufs: cleanup ufs initialization path

 drivers/scsi/ufs/ufshcd.c | 166 ++++++++------------------------------
 drivers/scsi/ufs/ufshcd.h |  12 +--
 2 files changed, 34 insertions(+), 144 deletions(-)

-- 
2.17.1

[PATCH v2 1/3] 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: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufshcd.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index aca50ed39844..0f8c7e05df29 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6881,8 +6881,7 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
 	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);
+	buff_len = QUERY_DESC_MAX_SIZE + 1;
 	desc_buf = kmalloc(buff_len, GFP_KERNEL);
 	if (!desc_buf) {
 		err = -ENOMEM;
-- 
2.17.1

[PATCH v2 2/3] 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>
---
 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 0f8c7e05df29..0a95f0a5ab73 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",
@@ -6888,8 +6877,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);
@@ -7170,8 +7159,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 v2 3/3] 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. This patch is to
delete unnecessary reduntant code, remove ufshcd_read_desc_length()
and boost UFS initialization.

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufshcd.c | 138 +++++++-------------------------------
 drivers/scsi/ufs/ufshcd.h |  12 +---
 2 files changed, 26 insertions(+), 124 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 0a95f0a5ab73..c47f4584c0f4 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3052,47 +3052,6 @@ 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
@@ -3101,46 +3060,27 @@ static int ufshcd_read_desc_length(struct ufs_hba *hba,
  *
  * 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)
+int 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:
-		*desc_len = 0;
-		break;
-	default:
+	if (desc_id >= QUERY_DESC_IDN_MAX) {
 		*desc_len = 0;
 		return -EINVAL;
 	}
+
+	*desc_len = hba->desc_size[desc_id];
 	return 0;
 }
 EXPORT_SYMBOL(ufshcd_map_desc_id_to_length);
 
+static void ufshcd_update_desc_length(struct ufs_hba *hba,
+				      enum desc_idn desc_id, int 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
@@ -3209,6 +3149,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 +6608,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 +6726,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 =
@@ -6878,7 +6822,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);
@@ -7108,42 +7052,10 @@ static void ufshcd_clear_dbg_ufs_stats(struct ufs_hba *hba)
 
 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;
+	int i;
 
-	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;
+	for (i = 0; i < QUERY_DESC_IDN_MAX; i++)
+		hba->desc_size[i] = QUERY_DESC_MAX_SIZE;
 }
 
 static int ufshcd_device_geo_params_init(struct ufs_hba *hba)
@@ -7152,7 +7064,7 @@ static int ufshcd_device_geo_params_init(struct ufs_hba *hba)
 	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;
@@ -7253,7 +7165,7 @@ static int ufshcd_device_params_init(struct ufs_hba *hba)
 	/* Clear any previous UFS device information */
 	memset(&hba->dev_info, 0, sizeof(hba->dev_info));
 
-	/* Init check for device descriptor sizes */
+	/* Init device descriptor sizes */
 	ufshcd_init_desc_sizes(hba);
 
 	/* Init UFS geometry descriptor related parameters */
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index e3dfb48e669e..b966d9b0eb3d 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;
+	u8 desc_size[QUERY_DESC_IDN_MAX];
 	atomic_t scsi_block_reqs_cnt;
 
 	struct device		bsg_dev;
-- 
2.17.1

RE: [PATCH v2 1/3] scsi: ufs: remove max_t in ufs_get_device_desc

[Avri Altman]

 
> 
> 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: Bean Huo <beanhuo@micron.com>
Acked-by: Avri Altman <avri.altman@wdc.com>

> ---
>  drivers/scsi/ufs/ufshcd.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index aca50ed39844..0f8c7e05df29 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6881,8 +6881,7 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
>         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);
> +       buff_len = QUERY_DESC_MAX_SIZE + 1;
So why the +1?
Originally it was used for the '\0' terminator, which is not needed anymore.

RE: [PATCH v2 2/3] scsi: ufs: delete ufshcd_read_desc()

[Avri Altman]

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

Re: [PATCH v2 1/3] scsi: ufs: remove max_t in ufs_get_device_desc

[Bean Huo]

On Thu, 2020-05-28 at 13:41 +0000, Avri Altman wrote:
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -6881,8 +6881,7 @@ static int ufs_get_device_desc(struct ufs_hba
> > *hba)
> >          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);
> > +       buff_len = QUERY_DESC_MAX_SIZE + 1;
> 
> So why the +1?
> Originally it was used for the '\0' terminator, which is not needed
> anymore.

you are correct, now not need +1, I will change it.

thanks,

Bean

Re: [PATCH v2 1/3] scsi: ufs: remove max_t in ufs_get_device_desc

[Bart Van Assche]

On 2020-05-28 04:56, 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: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index aca50ed39844..0f8c7e05df29 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6881,8 +6881,7 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
>  	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);
> +	buff_len = QUERY_DESC_MAX_SIZE + 1;
>  	desc_buf = kmalloc(buff_len, GFP_KERNEL);
>  	if (!desc_buf) {
>  		err = -ENOMEM;

Since the buff_len variable is not changed after its initial assignment,
please remove it entirely.

Thanks,

Bart.

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

[Avri Altman]

Hi,

> 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.
May I suggest one more clarifying sentence to your commit log:
"Instead, we will capture the descriptor size the first time we'll read it."

>This patch is to
> delete unnecessary reduntant code, remove ufshcd_read_desc_length()
typo: redundant

> and boost UFS initialization.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>

> +       if (desc_id >= QUERY_DESC_IDN_MAX) {
>                 *desc_len = 0;
>                 return -EINVAL;
>         }
if (desc_id == QUERY_DESC_IDN_RFU_0 || desc_id == QUERY_DESC_IDN_RFU_1)
	*desc_len = 0;
else
> +
> +       *desc_len = hba->desc_size[desc_id];
>         return 0;
>  }
>  EXPORT_SYMBOL(ufshcd_map_desc_id_to_length);
> 
> +static void ufshcd_update_desc_length(struct ufs_hba *hba,
> +                                     enum desc_idn desc_id, int desc_len)
desc_len is at most 255 so maybe u8?

> +{
> +       if (hba->desc_size[desc_id] == QUERY_DESC_MAX_SIZE &&
> +           desc_id != QUERY_DESC_IDN_STRING)
> +               hba->desc_size[desc_id] = desc_len;
> +}
> +


Thanks,
Avri

Re: [PATCH v2 2/3] scsi: ufs: delete ufshcd_read_desc()

[Bart Van Assche]

On 2020-05-28 04:56, Bean Huo wrote:
> Delete ufshcd_read_desc(). Instead, let caller directly call
> ufshcd_read_desc_param().

Reviewed-by: Bart van Assche <bvanassche@acm.org>

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

[Bart Van Assche]

On 2020-05-28 04:56, Bean Huo wrote:
> At UFS initialization stage, to get the length of the descriptor,
> ufshcd_read_desc_length() being called 6 times. This patch is to
> delete unnecessary reduntant code, remove ufshcd_read_desc_length()
> and boost UFS initialization.

As explained in Documentation/process/submitting-patches.rst, please use
the imperative mood in patch descriptions. In other words, please change
"This patch is to delete" into "Delete". Please also change "reduntant"
into "redundant". Otherwise this patch looks good to me. Hence:

Reviewed-by: Bart van Assche <bvanassche@acm.org>

Re: [PATCH v2 1/3] scsi: ufs: remove max_t in ufs_get_device_desc

[Bean Huo]

On Thu, 2020-05-28 at 07:56 -0700, Bart Van Assche wrote:
> > -     buff_len = max_t(size_t, hba->desc_size.dev_desc,
> > -                      QUERY_DESC_MAX_SIZE + 1);
> > +     buff_len = QUERY_DESC_MAX_SIZE + 1;
> >        desc_buf = kmalloc(buff_len, GFP_KERNEL);
> >        if (!desc_buf) {
> >                err = -ENOMEM;
> 
> Since the buff_len variable is not changed after its initial
> assignment,
> please remove it entirely.
> 
> Thanks,
> 
> Bart.

hi, Bart

Thanks.

do you mean  like this: buff_len = hba->desc_size[id]?

Bean

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

[Bean Huo]

On Thu, 2020-05-28 at 14:58 +0000, Avri Altman wrote:
> Hi,
> 
> > 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.
> 
> May I suggest one more clarifying sentence to your commit log:
> "Instead, we will capture the descriptor size the first time we'll
> read it."
> 
> > This patch is to
> > delete unnecessary reduntant code, remove ufshcd_read_desc_length()
> 
> typo: redundant

fixed.
> 
> > and boost UFS initialization.
> > 
> > Signed-off-by: Bean Huo <beanhuo@micron.com>
> > +       if (desc_id >= QUERY_DESC_IDN_MAX) {
> >                 *desc_len = 0;
> >                 return -EINVAL;
> >         }
> 
> if (desc_id == QUERY_DESC_IDN_RFU_0 || desc_id ==
> QUERY_DESC_IDN_RFU_1)
> 	*desc_len = 0;
> else
> > +
> > +       *desc_len = hba->desc_size[desc_id];
> >         return 0;
> >  }
> >  EXPORT_SYMBOL(ufshcd_map_desc_id_to_length);
> > 
> > +static void ufshcd_update_desc_length(struct ufs_hba *hba,
> > +                                     enum desc_idn desc_id, int
> > desc_len)
> 
> desc_len is at most 255 so maybe u8?
> 
Avri
thanks, it will be changed in next version.


Bean

Re: [PATCH v2 1/3] scsi: ufs: remove max_t in ufs_get_device_desc

[Bart Van Assche]

On 2020-05-28 08:04, Bean Huo wrote:
> do you mean  like this: buff_len = hba->desc_size[id]?

How about the following untested change?

Thanks,

Bart.

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 698e8d20b4ba..e33754c15c2c 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6606,14 +6606,11 @@ static int ufshcd_scsi_add_wlus(struct ufs_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;

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

[Bart Van Assche]

On 2020-05-28 07:58, Avri Altman wrote:
>> From: Bean Huo <beanhuo@micron.com>
>> +static void ufshcd_update_desc_length(struct ufs_hba *hba,
>> +                                     enum desc_idn desc_id, int desc_len)
> desc_len is at most 255 so maybe u8?

At least on x86 using types like 'u8' for function arguments may lead to
suboptimal code because it may cause the compiler to insert a widening
instruction. How about changing 'int desc_len' into 'unsigned desc_len'
instead?

Thanks,

Bart.

Re: [PATCH v2 2/3] scsi: ufs: delete ufshcd_read_desc()

[Stanley Chu]

On Thu, 2020-05-28 at 13:56 +0200, Bean Huo wrote:
> 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>
> ---
>  drivers/scsi/ufs/ufshcd.c | 27 ++++++++-------------------
>  1 file changed, 8 insertions(+), 19 deletions(-)

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

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

[Stanley Chu]

Hi Bean,

On Thu, 2020-05-28 at 13:56 +0200, Bean Huo wrote:
> 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. This patch is to
> delete unnecessary reduntant code, remove ufshcd_read_desc_length()
> and boost UFS initialization.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 138 +++++++-------------------------------
>  drivers/scsi/ufs/ufshcd.h |  12 +---
>  2 files changed, 26 insertions(+), 124 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 0a95f0a5ab73..c47f4584c0f4 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -3052,47 +3052,6 @@ 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
> @@ -3101,46 +3060,27 @@ static int ufshcd_read_desc_length(struct ufs_hba *hba,
>   *
>   * 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)
> +int 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:
> -		*desc_len = 0;
> -		break;
> -	default:
> +	if (desc_id >= QUERY_DESC_IDN_MAX) {
>  		*desc_len = 0;
>  		return -EINVAL;
>  	}
> +
> +	*desc_len = hba->desc_size[desc_id];
>  	return 0;
>  }
>  EXPORT_SYMBOL(ufshcd_map_desc_id_to_length);
>  
> +static void ufshcd_update_desc_length(struct ufs_hba *hba,
> +				      enum desc_idn desc_id, int 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
> @@ -3209,6 +3149,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 +6608,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 +6726,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 =
> @@ -6878,7 +6822,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);
> @@ -7108,42 +7052,10 @@ static void ufshcd_clear_dbg_ufs_stats(struct ufs_hba *hba)
>  
>  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;
> +	int i;
>  
> -	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;
> +	for (i = 0; i < QUERY_DESC_IDN_MAX; i++)
> +		hba->desc_size[i] = QUERY_DESC_MAX_SIZE;
>  }
>  
>  static int ufshcd_device_geo_params_init(struct ufs_hba *hba)
> @@ -7152,7 +7064,7 @@ static int ufshcd_device_geo_params_init(struct ufs_hba *hba)
>  	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;
> @@ -7253,7 +7165,7 @@ static int ufshcd_device_params_init(struct ufs_hba *hba)
>  	/* Clear any previous UFS device information */
>  	memset(&hba->dev_info, 0, sizeof(hba->dev_info));
>  
> -	/* Init check for device descriptor sizes */
> +	/* Init device descriptor sizes */
>  	ufshcd_init_desc_sizes(hba);

Perhaps just put simple code in ufshcd_init_desc_sizes() here and remove
ufshcd_init_desc_sizes()?

>  
>  	/* Init UFS geometry descriptor related parameters */
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index e3dfb48e669e..b966d9b0eb3d 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;
> +	u8 desc_size[QUERY_DESC_IDN_MAX];
>  	atomic_t scsi_block_reqs_cnt;
>  
>  	struct device		bsg_dev;

Except for the above nit, otherwise I really like this patch.

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