[PATCH v2 0/2] Changes for ufshcd.c

[PATCH v2 0/2] Changes for ufshcd.c

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

v1--v2:
    1. change in ufshcd_lu_init() in  patch 2/2:
        1) Remove duplicate parameter initialization
        2) Move lun parameter initialization before kmalloc()

Bean Huo (2):
  scsi: ufs: core: Remove unnecessary if statement
  scsi: ufs: core: Cleanup ufshcd_slave_alloc()

 drivers/ufs/core/ufshcd-priv.h |   3 -
 drivers/ufs/core/ufshcd.c      | 151 ++++++++++++---------------------
 2 files changed, 54 insertions(+), 100 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] scsi: ufs: core: Remove unnecessary if statement

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

LUs with WB potential support are properly checked in ufshcd_wb_probe()
before calling ufshcd_read_unit_desc_param(), so remove this unnecessary
if-checkup in ufs_is_valid_unit_desc_lun() to match its function definition.

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/ufs/core/ufshcd-priv.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index f68ca33f6ac7..2457b005101a 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -300,9 +300,6 @@ static inline bool ufs_is_valid_unit_desc_lun(struct ufs_dev_info *dev_info,
 		pr_err("Max General LU supported by UFS isn't initialized\n");
 		return false;
 	}
-	/* WB is available only for the logical unit from 0 to 7 */
-	if (param_offset == UNIT_DESC_PARAM_WB_BUF_ALLOC_UNITS)
-		return lun < UFS_UPIU_MAX_WB_LUN_ID;
 	return lun == UFS_UPIU_RPMB_WLUN || (lun < dev_info->max_lu_supported);
 }
 
-- 
2.34.1

RE: [PATCH v2 1/2] scsi: ufs: core: Remove unnecessary if statement

[Daejun Park]

Hi Bean Huo,

I think ufs_is_valid_unit_desc_lun() is also used for wb_buf_alloc_units_show() in ufs-sysfs.c.
So just removing this if-checkup will make different result when check lun value.

Thanks,
Daejun

>From: Bean Huo <beanhuo@micron.com>
>
>LUs with WB potential support are properly checked in ufshcd_wb_probe()
>before calling ufshcd_read_unit_desc_param(), so remove this unnecessary
>if-checkup in ufs_is_valid_unit_desc_lun() to match its function definition.
>
>Signed-off-by: Bean Huo <beanhuo@micron.com>
>---
> drivers/ufs/core/ufshcd-priv.h | 3 ---
> 1 file changed, 3 deletions(-)
>
>diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
>index f68ca33f6ac7..2457b005101a 100644
>--- a/drivers/ufs/core/ufshcd-priv.h
>+++ b/drivers/ufs/core/ufshcd-priv.h
>@@ -300,9 +300,6 @@ static inline bool ufs_is_valid_unit_desc_lun(struct ufs_dev_info *dev_info,
>                 pr_err("Max General LU supported by UFS isn't initialized\n");
>                 return false;
>         }
>-        /* WB is available only for the logical unit from 0 to 7 */
>-        if (param_offset == UNIT_DESC_PARAM_WB_BUF_ALLOC_UNITS)
>-                return lun < UFS_UPIU_MAX_WB_LUN_ID;
>         return lun == UFS_UPIU_RPMB_WLUN || (lun < dev_info->max_lu_supported);
> }
> 
>-- 
>2.34.1

Re: [PATCH v2 1/2] scsi: ufs: core: Remove unnecessary if statement

[Bean Huo]

On Tue, 2022-10-11 at 11:21 +0900, Daejun Park wrote:
> Hi Bean Huo,
> 
> I think ufs_is_valid_unit_desc_lun() is also used for
> wb_buf_alloc_units_show() in ufs-sysfs.c.
> So just removing this if-checkup will make different result when
> check lun value.
> 

Hi Daejun,

Thanks for your review on the patch. Yes, I understood what you mean.
But I don't think that's the problem. Without this patch, access on
sysfs node "wb_shared_alloc_units" would get "invalid argument", while
with this patch sysfs would return 00. According to the UFS
specification:

"If this value is ‘0’, then the WriteBooster is not supported for this
LU. The Logical unit among LU0 ~ LU7 can be configured for WriteBooster
Buffer. Otherwise, whole WriteBooster Buffer configuration in this
device is invalid."

Per my understanding,  with this patch,  there is still no miss-
explanation of this sysfs node. The key purpose of this patch is to
remove any nonsense logical during the booting stage.

please have a think my comments. thanks.


Kind regards,
Bean


> Thanks,
> Daejun
> 
> > From: Bean Huo <beanhuo@micron.com>
> > 
> > LUs with WB potential support are properly checked in ufshcd_wb_pro
> > be()
> > before calling ufshcd_read_unit_desc_param(), so remove this unnece
> > ssary
> > if-
> > checkup in ufs_is_valid_unit_desc_lun() to match its function defin
> > ition.

RE: [PATCH v2 1/2] scsi: ufs: core: Remove unnecessary if statement

[Daejun Park]

Hi Bean,

> LUs with WB potential support are properly checked in ufshcd_wb_probe()
> before calling ufshcd_read_unit_desc_param(), so remove this unnecessary
> if-checkup in ufs_is_valid_unit_desc_lun() to match its function definition.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/ufs/core/ufshcd-priv.h | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
> index f68ca33f6ac7..2457b005101a 100644
> --- a/drivers/ufs/core/ufshcd-priv.h
> +++ b/drivers/ufs/core/ufshcd-priv.h
> @@ -300,9 +300,6 @@ static inline bool ufs_is_valid_unit_desc_lun(struct ufs_dev_info *dev_info,
>                  pr_err("Max General LU supported by UFS isn't initialized\n");
>                  return false;
>          }
> -        /* WB is available only for the logical unit from 0 to 7 */
> -        if (param_offset == UNIT_DESC_PARAM_WB_BUF_ALLOC_UNITS)

Then, there is no requirement for "param_offset" argument.

Thanks,
Daejun

Re: [PATCH v2 1/2] scsi: ufs: core: Remove unnecessary if statement

[Bart Van Assche]

On 10/10/22 02:29, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> LUs with WB potential support are properly checked in ufshcd_wb_probe()
> before calling ufshcd_read_unit_desc_param(), so remove this unnecessary
> if-checkup in ufs_is_valid_unit_desc_lun() to match its function definition.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>   drivers/ufs/core/ufshcd-priv.h | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
> index f68ca33f6ac7..2457b005101a 100644
> --- a/drivers/ufs/core/ufshcd-priv.h
> +++ b/drivers/ufs/core/ufshcd-priv.h
> @@ -300,9 +300,6 @@ static inline bool ufs_is_valid_unit_desc_lun(struct ufs_dev_info *dev_info,
>   		pr_err("Max General LU supported by UFS isn't initialized\n");
>   		return false;
>   	}
> -	/* WB is available only for the logical unit from 0 to 7 */
> -	if (param_offset == UNIT_DESC_PARAM_WB_BUF_ALLOC_UNITS)
> -		return lun < UFS_UPIU_MAX_WB_LUN_ID;
>   	return lun == UFS_UPIU_RPMB_WLUN || (lun < dev_info->max_lu_supported);
>   }

Hi Bean,

I think the above patch reintroduces the stack overflow issue fixed by
commit a2fca52ee640 ("scsi: ufs: WB is only available on LUN #0 to #7").

How about reverting commit a2fca52ee640 and fixing the stack overflow
issue in another way than by modifying ufs_is_valid_unit_desc_lun()?

Thanks,

Bart.

Re: [PATCH v2 1/2] scsi: ufs: core: Remove unnecessary if statement

[Bean Huo]

On Fri, 2022-10-14 at 11:37 -0700, Bart Van Assche wrote:
> > @@ -300,9 +300,6 @@ static inline bool
> > ufs_is_valid_unit_desc_lun(struct ufs_dev_info *dev_info,
> >                 pr_err("Max General LU supported by UFS isn't
> > initialized\n");
> >                 return false;
> >         }
> > -       /* WB is available only for the logical unit from 0 to 7 */
> > -       if (param_offset == UNIT_DESC_PARAM_WB_BUF_ALLOC_UNITS)
> > -               return lun < UFS_UPIU_MAX_WB_LUN_ID;
> >         return lun == UFS_UPIU_RPMB_WLUN || (lun < dev_info-
> > >max_lu_supported);
> >    }
> 
> Hi Bean,
> 
> I think the above patch reintroduces the stack overflow issue fixed
> by
> commit a2fca52ee640 ("scsi: ufs: WB is only available on LUN #0 to
> #7").
> 
> How about reverting commit a2fca52ee640 and fixing the stack overflow
> issue in another way than by modifying ufs_is_valid_unit_desc_lun()?
> 
> Thanks,
> 
> Bart

Hi Bart, 

I knew that fix, it was because the user tried to poll
dLUNumWriteBoosterBufferAllocUnits from RPMB LU, as you know, RPMB
doesn't support WB, but the root cause is that we don't separate normal
logical unit descriptors and RPMB unit descriptor when we create sysfs
group,


in ufshcd_driver_template {

...
.sdev_groups = ufshcd_driver_groups,

}



ufshcd_driver_groups {
...
&ufs_sysfs_unit_descriptor_group,
...
}


so all the logical units will have the unified unit descriptor sysfs
node. This is wrong.  

Another problem is that Boot and device LU don't have unit descriptors,
but we still create a unit descriptor sysfs node group for boot and
device LU.

I am working on the Advanced RPMB, and trying to seperate normal unit
descriptor and RPMB unit descriptor, will let you know if it is
possible. or if you know the solution, please let me know, thanks.


Kind regards,
Bean

Re: [PATCH v2 1/2] scsi: ufs: core: Remove unnecessary if statement

[Bart Van Assche]

On 10/14/22 12:20, Bean Huo wrote:
> I am working on the Advanced RPMB, and trying to seperate normal unit
> descriptor and RPMB unit descriptor, will let you know if it is
> possible. or if you know the solution, please let me know, thanks.

Hi Bean,

How about setting .is_visible member of struct attribute_group in the UFS
driver and letting that callback decide which sysfs attributes are visible
depending on the logical unit type?

Thanks,

Bart.

Re: [PATCH v2 1/2] scsi: ufs: core: Remove unnecessary if statement

[Bean Huo]

On Fri, 2022-10-14 at 13:41 -0700, Bart Van Assche wrote:
> On 10/14/22 12:20, Bean Huo wrote:
> > I am working on the Advanced RPMB, and trying to seperate normal
> > unit
> > descriptor and RPMB unit descriptor, will let you know if it is
> > possible. or if you know the solution, please let me know, thanks.
> 
> Hi Bean,
> 
> How about setting .is_visible member of struct attribute_group in the
> UFS
> driver and letting that callback decide which sysfs attributes are
> visible
> depending on the logical unit type?
> 
> Thanks,
> 
> Bart.

Thanks, it looks like what I expected, I'll verify it further.

Kind regards,
Bean

Re: [PATCH v2 1/2] scsi: ufs: core: Remove unnecessary if statement

[Bean Huo]

On Fri, 2022-10-14 at 11:37 -0700, Bart Van Assche wrote:
> >                 pr_err("Max General LU supported by UFS isn't
> > initialized\n");
> >                 return false;
> >         }
> > -       /* WB is available only for the logical unit from 0 to 7 */
> > -       if (param_offset == UNIT_DESC_PARAM_WB_BUF_ALLOC_UNITS)
> > -               return lun < UFS_UPIU_MAX_WB_LUN_ID;
> >         return lun == UFS_UPIU_RPMB_WLUN || (lun < dev_info-
> > >max_lu_supported);
> >    }
> 
> Hi Bean,
> 
> I think the above patch reintroduces the stack overflow issue fixed
> by
> commit a2fca52ee640 ("scsi: ufs: WB is only available on LUN #0 to
> #7").
> 
> How about reverting commit a2fca52ee640 and fixing the stack overflow
> issue in another way than by modifying ufs_is_valid_unit_desc_lun()?
> 
> Thanks,
> 
> Bart.

Hi Bart, 

I double-checked the changelog and the stack overflow issue was double
fixed by your commit:

commit d3d9c4570285 ("scsi: ufs: Fix memory corruption by
ufshcd_read_desc_param()"),


For example, if the user wants to read wb_buf_alloc_units in the RPMB
unit descriptor,

parameter offset = 41, parameter size = 4,
buff_len = 45;

After ufshcd_query_descriptor_retry(), buff_len will be updated to 35.

param_offset > buff_len, then -EINVAL will be returned.

So we can safely remove this check, and if you still have concerns, I
can verify when I get back to the office.

Kind regards,
Bean

Re: [PATCH v2 1/2] scsi: ufs: core: Remove unnecessary if statement

[Bart Van Assche]

On 10/14/22 13:30, Bean Huo wrote:
> I double-checked the changelog and the stack overflow issue was double
> fixed by your commit:
> 
> commit d3d9c4570285 ("scsi: ufs: Fix memory corruption by
> ufshcd_read_desc_param()"),
> 
> For example, if the user wants to read wb_buf_alloc_units in the RPMB
> unit descriptor,
> 
> parameter offset = 41, parameter size = 4,
> buff_len = 45;
> 
> After ufshcd_query_descriptor_retry(), buff_len will be updated to 35.
> 
> param_offset > buff_len, then -EINVAL will be returned.
> 
> So we can safely remove this check, and if you still have concerns, I
> can verify when I get back to the office.

Hi Bean,

Thank you for having looked this up. I agree with the above.

Bart.

[PATCH v2 2/2] scsi: ufs: core: Cleanup ufshcd_slave_alloc()

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Combine ufshcd_get_lu_power_on_wp_status() and ufshcd_set_queue_depth()
into one single ufshcd_lu_init(), so that we only need to read the LUN
descriptor once to replace the original twice.

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/ufs/core/ufshcd.c | 151 ++++++++++++++------------------------
 1 file changed, 54 insertions(+), 97 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 7c15cbc737b4..0bf1f99b26bb 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -4862,100 +4862,6 @@ static int ufshcd_verify_dev_init(struct ufs_hba *hba)
 	return err;
 }
 
-/**
- * ufshcd_set_queue_depth - set lun queue depth
- * @sdev: pointer to SCSI device
- *
- * Read bLUQueueDepth value and activate scsi tagged command
- * queueing. For WLUN, queue depth is set to 1. For best-effort
- * cases (bLUQueueDepth = 0) the queue depth is set to a maximum
- * value that host can queue.
- */
-static void ufshcd_set_queue_depth(struct scsi_device *sdev)
-{
-	int ret = 0;
-	u8 lun_qdepth;
-	struct ufs_hba *hba;
-
-	hba = shost_priv(sdev->host);
-
-	lun_qdepth = hba->nutrs;
-	ret = ufshcd_read_unit_desc_param(hba,
-					  ufshcd_scsi_to_upiu_lun(sdev->lun),
-					  UNIT_DESC_PARAM_LU_Q_DEPTH,
-					  &lun_qdepth,
-					  sizeof(lun_qdepth));
-
-	/* Some WLUN doesn't support unit descriptor */
-	if (ret == -EOPNOTSUPP)
-		lun_qdepth = 1;
-	else if (!lun_qdepth)
-		/* eventually, we can figure out the real queue depth */
-		lun_qdepth = hba->nutrs;
-	else
-		lun_qdepth = min_t(int, lun_qdepth, hba->nutrs);
-
-	dev_dbg(hba->dev, "%s: activate tcq with queue depth %d\n",
-			__func__, lun_qdepth);
-	scsi_change_queue_depth(sdev, lun_qdepth);
-}
-
-/*
- * ufshcd_get_lu_wp - returns the "b_lu_write_protect" from UNIT DESCRIPTOR
- * @hba: per-adapter instance
- * @lun: UFS device lun id
- * @b_lu_write_protect: pointer to buffer to hold the LU's write protect info
- *
- * Returns 0 in case of success and b_lu_write_protect status would be returned
- * @b_lu_write_protect parameter.
- * Returns -ENOTSUPP if reading b_lu_write_protect is not supported.
- * Returns -EINVAL in case of invalid parameters passed to this function.
- */
-static int ufshcd_get_lu_wp(struct ufs_hba *hba,
-			    u8 lun,
-			    u8 *b_lu_write_protect)
-{
-	int ret;
-
-	if (!b_lu_write_protect)
-		ret = -EINVAL;
-	/*
-	 * According to UFS device spec, RPMB LU can't be write
-	 * protected so skip reading bLUWriteProtect parameter for
-	 * it. For other W-LUs, UNIT DESCRIPTOR is not available.
-	 */
-	else if (lun >= hba->dev_info.max_lu_supported)
-		ret = -ENOTSUPP;
-	else
-		ret = ufshcd_read_unit_desc_param(hba,
-					  lun,
-					  UNIT_DESC_PARAM_LU_WR_PROTECT,
-					  b_lu_write_protect,
-					  sizeof(*b_lu_write_protect));
-	return ret;
-}
-
-/**
- * ufshcd_get_lu_power_on_wp_status - get LU's power on write protect
- * status
- * @hba: per-adapter instance
- * @sdev: pointer to SCSI device
- *
- */
-static inline void ufshcd_get_lu_power_on_wp_status(struct ufs_hba *hba,
-						    const struct scsi_device *sdev)
-{
-	if (hba->dev_info.f_power_on_wp_en &&
-	    !hba->dev_info.is_lu_power_on_wp) {
-		u8 b_lu_write_protect;
-
-		if (!ufshcd_get_lu_wp(hba, ufshcd_scsi_to_upiu_lun(sdev->lun),
-				      &b_lu_write_protect) &&
-		    (b_lu_write_protect == UFS_LU_POWER_ON_WP))
-			hba->dev_info.is_lu_power_on_wp = true;
-	}
-}
-
 /**
  * ufshcd_setup_links - associate link b/w device wlun and other luns
  * @sdev: pointer to SCSI device
@@ -4993,6 +4899,59 @@ static void ufshcd_setup_links(struct ufs_hba *hba, struct scsi_device *sdev)
 	}
 }
 
+/**
+ * ufshcd_lu_power_on_wp_init - Initialize LU's power on write protect state
+ * @hba: per-adapter instance
+ * @sdev: pointer to SCSI device
+ * @b_lu_write_protect: bLUWriteProtect value read from LU descriptor
+ */
+static inline void ufshcd_lu_power_on_wp_init(struct ufs_hba *hba, const struct scsi_device *sdev,
+					      u8 b_lu_write_protect)
+{
+	if (hba->dev_info.f_power_on_wp_en && !hba->dev_info.is_lu_power_on_wp &&
+	    b_lu_write_protect == UFS_LU_POWER_ON_WP)
+		hba->dev_info.is_lu_power_on_wp = true;
+}
+
+static void ufshcd_lu_init(struct ufs_hba *hba, struct scsi_device *sdev)
+{
+	int ret;
+	int len;
+	u8 lun;
+	u8 lun_qdepth;
+	u8 *desc_buf;
+
+	lun_qdepth = hba->nutrs;
+	lun = ufshcd_scsi_to_upiu_lun(sdev->lun);
+	len = hba->desc_size[QUERY_DESC_IDN_UNIT];
+
+	desc_buf = kmalloc(len, GFP_KERNEL);
+	if (!desc_buf)
+		goto set_qdepth;
+
+	ret = ufshcd_read_unit_desc_param(hba, lun, 0, desc_buf, len);
+	if (ret == -EOPNOTSUPP)
+		/* If LU doesn't support unit descriptor, its queue depth is set to 1 */
+		lun_qdepth = 1;
+	else if (desc_buf[UNIT_DESC_PARAM_LU_Q_DEPTH])
+		lun_qdepth = min_t(int, desc_buf[UNIT_DESC_PARAM_LU_Q_DEPTH], hba->nutrs);
+	/*
+	 * According to UFS device spec, The write protection mode is only supported by normal LU,
+	 * not supported by WLUN.
+	 */
+	if (!ret && lun < hba->dev_info.max_lu_supported)
+		ufshcd_lu_power_on_wp_init(hba, sdev, desc_buf[UNIT_DESC_PARAM_LU_WR_PROTECT]);
+
+	kfree(desc_buf);
+set_qdepth:
+	/*
+	 * For WLUNs that don't support unit descriptor, queue depth is set to 1. For LUs whose
+	 * bLUQueueDepth == 0, the queue depth is set to a maximum value that host can queue.
+	 */
+	dev_dbg(hba->dev, "Set LU %x queue depth %d\n", lun, lun_qdepth);
+	scsi_change_queue_depth(sdev, lun_qdepth);
+}
+
 /**
  * ufshcd_slave_alloc - handle initial SCSI device configurations
  * @sdev: pointer to SCSI device
@@ -5020,9 +4979,7 @@ static int ufshcd_slave_alloc(struct scsi_device *sdev)
 	/* WRITE_SAME command is not supported */
 	sdev->no_write_same = 1;
 
-	ufshcd_set_queue_depth(sdev);
-
-	ufshcd_get_lu_power_on_wp_status(hba, sdev);
+	ufshcd_lu_init(hba, sdev);
 
 	ufshcd_setup_links(hba, sdev);
 
-- 
2.34.1

Re: [PATCH v2 2/2] scsi: ufs: core: Cleanup ufshcd_slave_alloc()

[Bart Van Assche]

On 10/10/22 02:29, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> Combine ufshcd_get_lu_power_on_wp_status() and ufshcd_set_queue_depth()
> into one single ufshcd_lu_init(), so that we only need to read the LUN
> descriptor once to replace the original twice.

The following part can probably be left out from the patch description 
without reducing clarity: " to replace the original twice".

> +/**
> + * ufshcd_lu_power_on_wp_init - Initialize LU's power on write protect state
> + * @hba: per-adapter instance
> + * @sdev: pointer to SCSI device
> + * @b_lu_write_protect: bLUWriteProtect value read from LU descriptor
> + */
> +static inline void ufshcd_lu_power_on_wp_init(struct ufs_hba *hba, const struct scsi_device *sdev,
> +					      u8 b_lu_write_protect)
> +{
> +	if (hba->dev_info.f_power_on_wp_en && !hba->dev_info.is_lu_power_on_wp &&
> +	    b_lu_write_protect == UFS_LU_POWER_ON_WP)
> +		hba->dev_info.is_lu_power_on_wp = true;
> +}

The body of this function is only three lines long and this function is 
only called once. Are you sure that you want a separate function instead 
of inlining this function in its only caller?

> +static void ufshcd_lu_init(struct ufs_hba *hba, struct scsi_device *sdev)
> +{
> +	int ret;
> +	int len;
> +	u8 lun;
> +	u8 lun_qdepth;
> +	u8 *desc_buf;

Most kernel developers these days order local variable declarations from 
longest to shortest line ("reverse Christmas tree").

> +	lun_qdepth = hba->nutrs;
> +	lun = ufshcd_scsi_to_upiu_lun(sdev->lun);
> +	len = hba->desc_size[QUERY_DESC_IDN_UNIT];
> +
> +	desc_buf = kmalloc(len, GFP_KERNEL);
> +	if (!desc_buf)
> +		goto set_qdepth;
> +
> +	ret = ufshcd_read_unit_desc_param(hba, lun, 0, desc_buf, len);
> +	if (ret == -EOPNOTSUPP)
> +		/* If LU doesn't support unit descriptor, its queue depth is set to 1 */
> +		lun_qdepth = 1;
> +	else if (desc_buf[UNIT_DESC_PARAM_LU_Q_DEPTH])
> +		lun_qdepth = min_t(int, desc_buf[UNIT_DESC_PARAM_LU_Q_DEPTH], hba->nutrs);

ufshcd_read_unit_desc_param() can return fewer bytes than requested. How 
about modifying ufshcd_read_unit_desc_param() such that it returns the 
number of bytes that has been copied and using that return value above 
to check whether at least UNIT_DESC_PARAM_LU_Q_DEPTH bytes have been 
initialized in desc_buf?

> +	/*
> +	 * According to UFS device spec, The write protection mode is only supported by normal LU,
> +	 * not supported by WLUN.
> +	 */
> +	if (!ret && lun < hba->dev_info.max_lu_supported)
> +		ufshcd_lu_power_on_wp_init(hba, sdev, desc_buf[UNIT_DESC_PARAM_LU_WR_PROTECT]);

Please insert an if (ret < 0) check after the 
ufshcd_read_unit_desc_param() call and jump to the kfree() statement if 
ret < 0 instead of checking several times whether or not ret < 0.

Thanks,

Bart.

Re: [PATCH v2 2/2] scsi: ufs: core: Cleanup ufshcd_slave_alloc()

[Bean Huo]

Hi Bart,

I took all your suggestions and will send new version patch tomorrow
when the autotest is complete.

thanks,
Bean

On Fri, 2022-10-14 at 14:05 -0700, Bart Van Assche wrote:
> On 10/10/22 02:29, Bean Huo wrote:
> > From: Bean Huo <beanhuo@micron.com>
> > 
> > Combine ufshcd_get_lu_power_on_wp_status() and
> > ufshcd_set_queue_depth()
> > into one single ufshcd_lu_init(), so that we only need to read the
> > LUN
> > descriptor once to replace the original twice.
> 
> The following part can probably be left out from the patch
> description 
> without reducing clarity: " to replace the original twice".
> 
> > +/**
> > + * ufshcd_lu_power_on_wp_init - Initialize LU's power on write
> > protect state
> > + * @hba: per-adapter instance
> > + * @sdev: pointer to SCSI device
> > + * @b_lu_write_protect: bLUWriteProtect value read from LU
> > descriptor
> > + */
> > +static inline void ufshcd_lu_power_on_wp_init(struct ufs_hba *hba,
> > const struct scsi_device *sdev,
> > +                                             u8
> > b_lu_write_protect)
> > +{
> > +       if (hba->dev_info.f_power_on_wp_en && !hba-
> > >dev_info.is_lu_power_on_wp &&
> > +           b_lu_write_protect == UFS_LU_POWER_ON_WP)
> > +               hba->dev_info.is_lu_power_on_wp = true;
> > +}
> 
> The body of this function is only three lines long and this function
> is 
> only called once. Are you sure that you want a separate function
> instead 
> of inlining this function in its only caller?
> 
> > +static void ufshcd_lu_init(struct ufs_hba *hba, struct scsi_device
> > *sdev)
> > +{
> > +       int ret;
> > +       int len;
> > +       u8 lun;
> > +       u8 lun_qdepth;
> > +       u8 *desc_buf;
> 
> Most kernel developers these days order local variable declarations
> from 
> longest to shortest line ("reverse Christmas tree").
> 
> > +       lun_qdepth = hba->nutrs;
> > +       lun = ufshcd_scsi_to_upiu_lun(sdev->lun);
> > +       len = hba->desc_size[QUERY_DESC_IDN_UNIT];
> > +
> > +       desc_buf = kmalloc(len, GFP_KERNEL);
> > +       if (!desc_buf)
> > +               goto set_qdepth;
> > +
> > +       ret = ufshcd_read_unit_desc_param(hba, lun, 0, desc_buf,
> > len);
> > +       if (ret == -EOPNOTSUPP)
> > +               /* If LU doesn't support unit descriptor, its queue
> > depth is set to 1 */
> > +               lun_qdepth = 1;
> > +       else if (desc_buf[UNIT_DESC_PARAM_LU_Q_DEPTH])
> > +               lun_qdepth = min_t(int,
> > desc_buf[UNIT_DESC_PARAM_LU_Q_DEPTH], hba->nutrs);
> 
> ufshcd_read_unit_desc_param() can return fewer bytes than requested.
> How 
> about modifying ufshcd_read_unit_desc_param() such that it returns
> the 
> number of bytes that has been copied and using that return value
> above 
> to check whether at least UNIT_DESC_PARAM_LU_Q_DEPTH bytes have been 
> initialized in desc_buf?
> 
> > +       /*
> > +        * According to UFS device spec, The write protection mode
> > is only supported by normal LU,
> > +        * not supported by WLUN.
> > +        */
> > +       if (!ret && lun < hba->dev_info.max_lu_supported)
> > +               ufshcd_lu_power_on_wp_init(hba, sdev,
> > desc_buf[UNIT_DESC_PARAM_LU_WR_PROTECT]);
> 
> Please insert an if (ret < 0) check after the 
> ufshcd_read_unit_desc_param() call and jump to the kfree() statement
> if 
> ret < 0 instead of checking several times whether or not ret < 0.
> 
> Thanks,
> 
> Bart.