[PATCH v1 0/3] use UFS device indicated maximum LU number

[PATCH v1 0/3] use UFS device indicated maximum LU number

[Bean Huo]

According to Jedec standard UFS 3.0 and UFS 2.1 Spec, Maximum
number of logical units supported by the UFS device is specified
by parameter bMaxNumberLU in Geometry Descriptor. This series
of patches is to delete macro definition UFS_UPIU_MAX_GENERAL_LUN,
and switch to use indicated value in descriptor instead.


Bean Huo (3):
  scsi: ufs: add max_lu_supported in struct ufs_dev_info
  scsi: ufs: initialize max_lu_supported while booting
  scsi: ufs: use UFS device indicated maximum LU number

 drivers/scsi/ufs/ufs-sysfs.c |  2 +-
 drivers/scsi/ufs/ufs.h       | 14 +++++++---
 drivers/scsi/ufs/ufshcd.c    | 51 +++++++++++++++++++++++++++++++++---
 3 files changed, 60 insertions(+), 7 deletions(-)

-- 
2.17.1

[PATCH 1/3] scsi: ufs: add max_lu_supported in struct ufs_dev_info

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Add one new parameter max_lu_supported in struct ufs_dev_info,
which will be used to express exactly how many general LUs being
supported by UFS device.

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufs.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index c89f21698629..5ca7ea4f223e 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -530,6 +530,8 @@ struct ufs_dev_info {
 	bool f_power_on_wp_en;
 	/* Keeps information if any of the LU is power on write protected */
 	bool is_lu_power_on_wp;
+	/* Maximum number of general LU supported by the UFS device */
+	u8 max_lu_supported;
 };
 
 #define MAX_MODEL_LEN 16
-- 
2.17.1

Re: [PATCH 1/3] scsi: ufs: add max_lu_supported in struct ufs_dev_info

[Bart Van Assche]

On 2020-01-10 10:36, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> Add one new parameter max_lu_supported in struct ufs_dev_info,
> which will be used to express exactly how many general LUs being
> supported by UFS device.

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

Re: [PATCH 1/3] scsi: ufs: add max_lu_supported in struct ufs_dev_info

[asutoshd]

On 2020-01-10 10:36, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> Add one new parameter max_lu_supported in struct ufs_dev_info,
> which will be used to express exactly how many general LUs being
> supported by UFS device.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/scsi/ufs/ufs.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index c89f21698629..5ca7ea4f223e 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -530,6 +530,8 @@ struct ufs_dev_info {
>  	bool f_power_on_wp_en;
>  	/* Keeps information if any of the LU is power on write protected */
>  	bool is_lu_power_on_wp;
> +	/* Maximum number of general LU supported by the UFS device */
> +	u8 max_lu_supported;
>  };
> 
>  #define MAX_MODEL_LEN 16

Looks good to me.
Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>

[PATCH 2/3] scsi: ufs: initialize max_lu_supported while booting

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

This patch is to add a new function ufshcd_init_device_param() for
initialization of  UFS device related parameters which are used by
driver. In this version patch, there is only dev_info.max_lu_supported
being initialized.

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufshcd.c | 47 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 46 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 1b97f2dc0b63..a297fe55e36a 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3158,6 +3158,11 @@ static int ufshcd_read_device_desc(struct ufs_hba *hba, u8 *buf, u32 size)
 	return ufshcd_read_desc(hba, QUERY_DESC_IDN_DEVICE, 0, buf, size);
 }
 
+static int ufshcd_read_geometry_desc(struct ufs_hba *hba, u8 *buf, u32 size)
+{
+	return ufshcd_read_desc(hba, QUERY_DESC_IDN_GEOMETRY, 0, buf, size);
+}
+
 /**
  * struct uc_string_id - unicode string
  *
@@ -6827,6 +6832,37 @@ static void ufshcd_clear_dbg_ufs_stats(struct ufs_hba *hba)
 	hba->req_abort_count = 0;
 }
 
+static int ufshcd_init_device_param(struct ufs_hba *hba)
+{
+	int err;
+	size_t buff_len;
+	u8 *desc_buf;
+
+	buff_len = QUERY_DESC_MAX_SIZE;
+	desc_buf = kmalloc(buff_len, GFP_KERNEL);
+	if (!desc_buf) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	err = ufshcd_read_geometry_desc(hba, desc_buf,
+			hba->desc_size.geom_desc);
+	if (err) {
+		dev_err(hba->dev, "%s: Failed reading Geometry Desc. err = %d\n",
+			__func__, err);
+		goto out;
+	}
+
+	if (desc_buf[GEOMETRY_DESC_PARAM_MAX_NUM_LUN] == 1)
+		hba->dev_info.max_lu_supported = 32;
+	else if (desc_buf[GEOMETRY_DESC_PARAM_MAX_NUM_LUN] == 0)
+		hba->dev_info.max_lu_supported = 8;
+
+out:
+	kfree(desc_buf);
+	return err;
+}
+
 static void ufshcd_init_desc_sizes(struct ufs_hba *hba)
 {
 	int err;
@@ -7016,13 +7052,22 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
 
 	/*
 	 * If we are in error handling context or in power management callbacks
-	 * context, no need to scan the host
+	 * context, no need to scan the host and to reinitialize the parameters
 	 */
 	if (!ufshcd_eh_in_progress(hba) && !hba->pm_op_in_progress) {
 		bool flag;
 
 		/* clear any previous UFS device information */
 		memset(&hba->dev_info, 0, sizeof(hba->dev_info));
+		/* Init parameters according to UFS relevant descriptors */
+		ret = ufshcd_init_device_param(hba);
+		if (ret) {
+			dev_err(hba->dev,
+				"%s: Init of device param failed. err = %d\n",
+				__func__, ret);
+			goto out;
+		}
+
 		if (!ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_READ_FLAG,
 				QUERY_FLAG_IDN_PWR_ON_WPE, &flag))
 			hba->dev_info.f_power_on_wp_en = flag;
-- 
2.17.1

Re: [PATCH 2/3] scsi: ufs: initialize max_lu_supported while booting

[Bart Van Assche]

On 2020-01-10 10:36, Bean Huo wrote:
> +static int ufshcd_read_geometry_desc(struct ufs_hba *hba, u8 *buf, u32 size)
> +{
> +	return ufshcd_read_desc(hba, QUERY_DESC_IDN_GEOMETRY, 0, buf, size);
> +}

The declaration of this function is longer than its body. Do we really
need this function? Has it been considered to inline this function into
its caller?

> +static int ufshcd_init_device_param(struct ufs_hba *hba)
> +{
> +	int err;
> +	size_t buff_len;
> +	u8 *desc_buf;
> +
> +	buff_len = QUERY_DESC_MAX_SIZE;
> +	desc_buf = kmalloc(buff_len, GFP_KERNEL);
> +	if (!desc_buf) {
> +		err = -ENOMEM;
> +		goto out;
> +	}

Has it been considered to use hba->desc_size.geom_desc instead of
QUERY_DESC_MAX_SIZE?

> +	err = ufshcd_read_geometry_desc(hba, desc_buf,
> +			hba->desc_size.geom_desc);
> +	if (err) {
> +		dev_err(hba->dev, "%s: Failed reading Geometry Desc. err = %d\n",
> +			__func__, err);
> +		goto out;
> +	}
> +
> +	if (desc_buf[GEOMETRY_DESC_PARAM_MAX_NUM_LUN] == 1)
> +		hba->dev_info.max_lu_supported = 32;
> +	else if (desc_buf[GEOMETRY_DESC_PARAM_MAX_NUM_LUN] == 0)
> +		hba->dev_info.max_lu_supported = 8;

Can it happen that GEOMETRY_DESC_PARAM_MAX_NUM_LUN >=
hba->desc_size.geom_desc and hence that the above code reads
uninitialized data?

> @@ -7016,13 +7052,22 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
>  
>  	/*
>  	 * If we are in error handling context or in power management callbacks
> -	 * context, no need to scan the host
> +	 * context, no need to scan the host and to reinitialize the parameters
>  	 */
>  	if (!ufshcd_eh_in_progress(hba) && !hba->pm_op_in_progress) {
>  		bool flag;
>  
>  		/* clear any previous UFS device information */
>  		memset(&hba->dev_info, 0, sizeof(hba->dev_info));
> +		/* Init parameters according to UFS relevant descriptors */
> +		ret = ufshcd_init_device_param(hba);
> +		if (ret) {
> +			dev_err(hba->dev,
> +				"%s: Init of device param failed. err = %d\n",
> +				__func__, ret);
> +			goto out;
> +		}
> +
>  		if (!ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_READ_FLAG,
>  				QUERY_FLAG_IDN_PWR_ON_WPE, &flag))
>  			hba->dev_info.f_power_on_wp_en = flag;

The context check in ufshcd_probe_hba() looks ugly to me. Has it been
considered to move all code that is controlled by the if-statement with
the context check into ufshcd_async_scan()?

Thanks,

Bart.

RE: [PATCH 2/3] scsi: ufs: initialize max_lu_supported while booting

[Avri Altman]

 
> >               memset(&hba->dev_info, 0, sizeof(hba->dev_info));
> > +             /* Init parameters according to UFS relevant descriptors */
> > +             ret = ufshcd_init_device_param(hba);
> > +             if (ret) {
> > +                     dev_err(hba->dev,
> > +                             "%s: Init of device param failed. err = %d\n",
> > +                             __func__, ret);
> > +                     goto out;
> > +             }
> > +
> >               if (!ufshcd_query_flag_retry(hba,
> UPIU_QUERY_OPCODE_READ_FLAG,
> >                               QUERY_FLAG_IDN_PWR_ON_WPE, &flag))
> >                       hba->dev_info.f_power_on_wp_en = flag;
> 
> The context check in ufshcd_probe_hba() looks ugly to me. Has it been
> considered to move all code that is controlled by the if-statement with the
> context check into ufshcd_async_scan()?
As part of ufshcd_probe_hba we also read the device descriptor,
Which is, by spec, an optional stage of the boot process, right after the unipro boot sequence.
Might want to consider moving the call there, as an integral phase of obtaining device info.

Thanks,
Avri

> 
> Thanks,
> 
> Bart.
> 

RE: [EXT] Re: [PATCH 2/3] scsi: ufs: initialize max_lu_supported while booting

[Bean Huo (beanhuo)]

Hi, Bart

> > +static int ufshcd_read_geometry_desc(struct ufs_hba *hba, u8 *buf,
> > +u32 size) {
> > +	return ufshcd_read_desc(hba, QUERY_DESC_IDN_GEOMETRY, 0, buf,
> size);
> > +}
> 
> The declaration of this function is longer than its body. Do we really need this
> function? Has it been considered to inline this function into its caller?
>

No, absolutely doesn't need it. ufshcd_read_power_desc() and ufshcd_read_device_desc()
as well. Let me try to simplify them in the next version.
 
> > +static int ufshcd_init_device_param(struct ufs_hba *hba) {
> > +	int err;
> > +	size_t buff_len;
> > +	u8 *desc_buf;
> > +
> > +	buff_len = QUERY_DESC_MAX_SIZE;
> > +	desc_buf = kmalloc(buff_len, GFP_KERNEL);
> > +	if (!desc_buf) {
> > +		err = -ENOMEM;
> > +		goto out;
> > +	}
> 
> Has it been considered to use hba->desc_size.geom_desc instead of
> QUERY_DESC_MAX_SIZE?
>
The reason is that I want all of UFS  device related parameters move to 
ufshcd_init_device_param(), And QUERY_DESC_MAX_SIZE is to compatible
with all of descriptors size. 
 
> > +	err = ufshcd_read_geometry_desc(hba, desc_buf,
> > +			hba->desc_size.geom_desc);
> > +	if (err) {
> > +		dev_err(hba->dev, "%s: Failed reading Geometry Desc. err
> = %d\n",
> > +			__func__, err);
> > +		goto out;
> > +	}
> > +
> > +	if (desc_buf[GEOMETRY_DESC_PARAM_MAX_NUM_LUN] == 1)
> > +		hba->dev_info.max_lu_supported = 32;
> > +	else if (desc_buf[GEOMETRY_DESC_PARAM_MAX_NUM_LUN] == 0)
> > +		hba->dev_info.max_lu_supported = 8;
> 
> Can it happen that GEOMETRY_DESC_PARAM_MAX_NUM_LUN >=
> hba->desc_size.geom_desc and hence that the above code reads
> uninitialized data?
> 
No, GEOMETRY_DESC_PARAM_MAX_NUM_LUN 0x0c is far less than geometry descriptor size.
As for the  UFS 2.0, this field is reserved, it is default 0.  For the UFS 2.1 and UFS 3.0, there are only
two valid value for this filed, either 00h: 8 Logical units, or 01h: 32 Logical units.

> > @@ -7016,13 +7052,22 @@ static int ufshcd_probe_hba(struct ufs_hba
> > *hba)
> >
> >  	/*
> >  	 * If we are in error handling context or in power management callbacks
> > -	 * context, no need to scan the host
> > +	 * context, no need to scan the host and to reinitialize the
> > +parameters
> >  	 */
> >  	if (!ufshcd_eh_in_progress(hba) && !hba->pm_op_in_progress) {
> >  		bool flag;
> >
> >  		/* clear any previous UFS device information */
> >  		memset(&hba->dev_info, 0, sizeof(hba->dev_info));
> > +		/* Init parameters according to UFS relevant descriptors */
> > +		ret = ufshcd_init_device_param(hba);
> > +		if (ret) {
> > +			dev_err(hba->dev,
> > +				"%s: Init of device param failed. err = %d\n",
> > +				__func__, ret);
> > +			goto out;
> > +		}
> > +
> >  		if (!ufshcd_query_flag_retry(hba,
> UPIU_QUERY_OPCODE_READ_FLAG,
> >  				QUERY_FLAG_IDN_PWR_ON_WPE, &flag))
> >  			hba->dev_info.f_power_on_wp_en = flag;
> 
> The context check in ufshcd_probe_hba() looks ugly to me. Has it been
> considered to move all code that is controlled by the if-statement with the
> context check into ufshcd_async_scan()?
> 
I totally agree with you. They should be moved out from ufshcd_probe_hba(), 
And moved to ufshcd_async_scan(). Let me do in the next version. 

Thanks.

//Bean Huo 

Re: [PATCH 2/3] scsi: ufs: initialize max_lu_supported while booting

[asutoshd]

On 2020-01-10 10:36, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> This patch is to add a new function ufshcd_init_device_param() for
> initialization of  UFS device related parameters which are used by
> driver. In this version patch, there is only dev_info.max_lu_supported
> being initialized.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 47 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 1b97f2dc0b63..a297fe55e36a 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -3158,6 +3158,11 @@ static int ufshcd_read_device_desc(struct
> ufs_hba *hba, u8 *buf, u32 size)
>  	return ufshcd_read_desc(hba, QUERY_DESC_IDN_DEVICE, 0, buf, size);
>  }
> 
> +static int ufshcd_read_geometry_desc(struct ufs_hba *hba, u8 *buf, u32 
> size)
> +{
> +	return ufshcd_read_desc(hba, QUERY_DESC_IDN_GEOMETRY, 0, buf, size);
> +}
> +
>  /**
>   * struct uc_string_id - unicode string
>   *
> @@ -6827,6 +6832,37 @@ static void ufshcd_clear_dbg_ufs_stats(struct
> ufs_hba *hba)
>  	hba->req_abort_count = 0;
>  }
> 
> +static int ufshcd_init_device_param(struct ufs_hba *hba)
> +{
> +	int err;
> +	size_t buff_len;
> +	u8 *desc_buf;
> +
> +	buff_len = QUERY_DESC_MAX_SIZE;
> +	desc_buf = kmalloc(buff_len, GFP_KERNEL);
> +	if (!desc_buf) {
> +		err = -ENOMEM;
> +		goto out;
> +	}
> +
> +	err = ufshcd_read_geometry_desc(hba, desc_buf,
> +			hba->desc_size.geom_desc);
> +	if (err) {
> +		dev_err(hba->dev, "%s: Failed reading Geometry Desc. err = %d\n",
> +			__func__, err);
> +		goto out;
> +	}
> +
> +	if (desc_buf[GEOMETRY_DESC_PARAM_MAX_NUM_LUN] == 1)
> +		hba->dev_info.max_lu_supported = 32;
> +	else if (desc_buf[GEOMETRY_DESC_PARAM_MAX_NUM_LUN] == 0)
> +		hba->dev_info.max_lu_supported = 8;
> +
> +out:
> +	kfree(desc_buf);
> +	return err;
> +}
> +
>  static void ufshcd_init_desc_sizes(struct ufs_hba *hba)
>  {
>  	int err;
> @@ -7016,13 +7052,22 @@ static int ufshcd_probe_hba(struct ufs_hba 
> *hba)
> 
>  	/*
>  	 * If we are in error handling context or in power management 
> callbacks
> -	 * context, no need to scan the host
> +	 * context, no need to scan the host and to reinitialize the 
> parameters
>  	 */
>  	if (!ufshcd_eh_in_progress(hba) && !hba->pm_op_in_progress) {
>  		bool flag;
> 
>  		/* clear any previous UFS device information */
>  		memset(&hba->dev_info, 0, sizeof(hba->dev_info));
> +		/* Init parameters according to UFS relevant descriptors */
> +		ret = ufshcd_init_device_param(hba);
> +		if (ret) {
> +			dev_err(hba->dev,
> +				"%s: Init of device param failed. err = %d\n",
> +				__func__, ret);
> +			goto out;
> +		}
> +
>  		if (!ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_READ_FLAG,
>  				QUERY_FLAG_IDN_PWR_ON_WPE, &flag))
>  			hba->dev_info.f_power_on_wp_en = flag;

Looks good to me.
Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>

[PATCH 3/3] scsi: ufs: use UFS device indicated maximum LU number

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

According to Jedec standard UFS 3.0 and UFS 2.1 Spec, Maximum number of logical
units supported by the UFS device is indicated by parameter bMaxNumberLU in
Geometry Descriptor. This patch is to delete current hard code macro definition
of UFS_UPIU_MAX_GENERAL_LUN, and switch to use device indicated number instead.

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

diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index 720be3f64be7..dbdf8b01abed 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -713,7 +713,7 @@ static ssize_t _pname##_show(struct device *dev,			\
 	struct scsi_device *sdev = to_scsi_device(dev);			\
 	struct ufs_hba *hba = shost_priv(sdev->host);			\
 	u8 lun = ufshcd_scsi_to_upiu_lun(sdev->lun);			\
-	if (!ufs_is_valid_unit_desc_lun(lun))				\
+	if (!ufs_is_valid_unit_desc_lun(&hba->dev_info, lun))		\
 		return -EINVAL;						\
 	return ufs_sysfs_read_desc_param(hba, QUERY_DESC_IDN_##_duname,	\
 		lun, _duname##_DESC_PARAM##_puname, buf, _size);	\
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 5ca7ea4f223e..810eeca0de63 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -63,7 +63,6 @@
 #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_UPIU_MAX_GENERAL_LUN	8
 
 /* Well known logical unit id in LUN field of UPIU */
 enum {
@@ -548,12 +547,19 @@ struct ufs_dev_desc {
 
 /**
  * ufs_is_valid_unit_desc_lun - checks if the given LUN has a unit descriptor
+ * @dev_info: pointer of instance of struct ufs_dev_info
  * @lun: LU number to check
  * @return: true if the lun has a matching unit descriptor, false otherwise
  */
-static inline bool ufs_is_valid_unit_desc_lun(u8 lun)
+static inline bool ufs_is_valid_unit_desc_lun(struct ufs_dev_info *dev_info,
+		u8 lun)
 {
-	return lun == UFS_UPIU_RPMB_WLUN || (lun < UFS_UPIU_MAX_GENERAL_LUN);
+	if (!dev_info || !dev_info->max_lu_supported) {
+		pr_err("Max General LU supported by UFS isn't initilized\n");
+		return false;
+	}
+
+	return lun == UFS_UPIU_RPMB_WLUN || (lun < dev_info->max_lu_supported);
 }
 
 #endif /* End of Header */
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index a297fe55e36a..c6ea5d88222d 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3286,7 +3286,7 @@ static inline int ufshcd_read_unit_desc_param(struct ufs_hba *hba,
 	 * Unit descriptors are only available for general purpose LUs (LUN id
 	 * from 0 to 7) and RPMB Well known LU.
 	 */
-	if (!ufs_is_valid_unit_desc_lun(lun))
+	if (!ufs_is_valid_unit_desc_lun(&hba->dev_info, lun))
 		return -EOPNOTSUPP;
 
 	return ufshcd_read_desc_param(hba, QUERY_DESC_IDN_UNIT, lun,
@@ -4540,7 +4540,7 @@ static int ufshcd_get_lu_wp(struct ufs_hba *hba,
 	 * protected so skip reading bLUWriteProtect parameter for
 	 * it. For other W-LUs, UNIT DESCRIPTOR is not available.
 	 */
-	else if (lun >= UFS_UPIU_MAX_GENERAL_LUN)
+	else if (lun >= hba->dev_info.max_lu_supported)
 		ret = -ENOTSUPP;
 	else
 		ret = ufshcd_read_unit_desc_param(hba,
-- 
2.17.1

Re: [PATCH 3/3] scsi: ufs: use UFS device indicated maximum LU number

[Bart Van Assche]

On 2020-01-10 10:36, Bean Huo wrote:
> @@ -548,12 +547,19 @@ struct ufs_dev_desc {
>  
>  /**
>   * ufs_is_valid_unit_desc_lun - checks if the given LUN has a unit descriptor
> + * @dev_info: pointer of instance of struct ufs_dev_info
>   * @lun: LU number to check
>   * @return: true if the lun has a matching unit descriptor, false otherwise
>   */
> -static inline bool ufs_is_valid_unit_desc_lun(u8 lun)
> +static inline bool ufs_is_valid_unit_desc_lun(struct ufs_dev_info *dev_info,
> +		u8 lun)
>  {

Can the dev_info be declared 'const' (const truct ufs_dev_info *dev_info)?

> -	return lun == UFS_UPIU_RPMB_WLUN || (lun < UFS_UPIU_MAX_GENERAL_LUN);
> +	if (!dev_info || !dev_info->max_lu_supported) {
> +		pr_err("Max General LU supported by UFS isn't initilized\n");
                                                              ^^^^^^^^^^
                                                            initialized?
> +		return false;
> +	}
> +
> +	return lun == UFS_UPIU_RPMB_WLUN || (lun < dev_info->max_lu_supported);
>  }

Are the parentheses in the above expression necessary?

Thanks,

Bart.

Re: [PATCH 3/3] scsi: ufs: use UFS device indicated maximum LU number

[asutoshd]

On 2020-01-10 10:36, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> According to Jedec standard UFS 3.0 and UFS 2.1 Spec, Maximum number of 
> logical
> units supported by the UFS device is indicated by parameter 
> bMaxNumberLU in
> Geometry Descriptor. This patch is to delete current hard code macro 
> definition
> of UFS_UPIU_MAX_GENERAL_LUN, and switch to use device indicated number 
> instead.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/scsi/ufs/ufs-sysfs.c |  2 +-
>  drivers/scsi/ufs/ufs.h       | 12 +++++++++---
>  drivers/scsi/ufs/ufshcd.c    |  4 ++--
>  3 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs-sysfs.c 
> b/drivers/scsi/ufs/ufs-sysfs.c
> index 720be3f64be7..dbdf8b01abed 100644
> --- a/drivers/scsi/ufs/ufs-sysfs.c
> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> @@ -713,7 +713,7 @@ static ssize_t _pname##_show(struct device 
> *dev,			\
>  	struct scsi_device *sdev = to_scsi_device(dev);			\
>  	struct ufs_hba *hba = shost_priv(sdev->host);			\
>  	u8 lun = ufshcd_scsi_to_upiu_lun(sdev->lun);			\
> -	if (!ufs_is_valid_unit_desc_lun(lun))				\
> +	if (!ufs_is_valid_unit_desc_lun(&hba->dev_info, lun))		\
>  		return -EINVAL;						\
>  	return ufs_sysfs_read_desc_param(hba, QUERY_DESC_IDN_##_duname,	\
>  		lun, _duname##_DESC_PARAM##_puname, buf, _size);	\
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index 5ca7ea4f223e..810eeca0de63 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -63,7 +63,6 @@
>  #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_UPIU_MAX_GENERAL_LUN	8
> 
>  /* Well known logical unit id in LUN field of UPIU */
>  enum {
> @@ -548,12 +547,19 @@ struct ufs_dev_desc {
> 
>  /**
>   * ufs_is_valid_unit_desc_lun - checks if the given LUN has a unit 
> descriptor
> + * @dev_info: pointer of instance of struct ufs_dev_info
>   * @lun: LU number to check
>   * @return: true if the lun has a matching unit descriptor, false 
> otherwise
>   */
> -static inline bool ufs_is_valid_unit_desc_lun(u8 lun)
> +static inline bool ufs_is_valid_unit_desc_lun(struct ufs_dev_info 
> *dev_info,
> +		u8 lun)
>  {
> -	return lun == UFS_UPIU_RPMB_WLUN || (lun < UFS_UPIU_MAX_GENERAL_LUN);
> +	if (!dev_info || !dev_info->max_lu_supported) {
> +		pr_err("Max General LU supported by UFS isn't initilized\n");
> +		return false;
> +	}
> +
> +	return lun == UFS_UPIU_RPMB_WLUN || (lun < 
> dev_info->max_lu_supported);
>  }
> 
>  #endif /* End of Header */
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index a297fe55e36a..c6ea5d88222d 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -3286,7 +3286,7 @@ static inline int
> ufshcd_read_unit_desc_param(struct ufs_hba *hba,
>  	 * Unit descriptors are only available for general purpose LUs (LUN 
> id
>  	 * from 0 to 7) and RPMB Well known LU.
>  	 */
> -	if (!ufs_is_valid_unit_desc_lun(lun))
> +	if (!ufs_is_valid_unit_desc_lun(&hba->dev_info, lun))
>  		return -EOPNOTSUPP;
> 
>  	return ufshcd_read_desc_param(hba, QUERY_DESC_IDN_UNIT, lun,
> @@ -4540,7 +4540,7 @@ static int ufshcd_get_lu_wp(struct ufs_hba *hba,
>  	 * protected so skip reading bLUWriteProtect parameter for
>  	 * it. For other W-LUs, UNIT DESCRIPTOR is not available.
>  	 */
> -	else if (lun >= UFS_UPIU_MAX_GENERAL_LUN)
> +	else if (lun >= hba->dev_info.max_lu_supported)
>  		ret = -ENOTSUPP;
>  	else
>  		ret = ufshcd_read_unit_desc_param(hba,

Looks good to me.
Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>