diff mbox series

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

Message ID 20200110183606.10102-3-huobean@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series use UFS device indicated maximum LU number | expand

Commit Message

Bean Huo Jan. 10, 2020, 6:36 p.m. UTC
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(-)

Comments

Bart Van Assche Jan. 11, 2020, 10:42 p.m. UTC | #1
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.
Avri Altman Jan. 12, 2020, 8:41 a.m. UTC | #2
> >               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.
>
Bean Huo Jan. 12, 2020, 9:52 a.m. UTC | #3
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
Asutosh Das (asd) Jan. 13, 2020, 6:44 p.m. UTC | #4
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>
diff mbox series

Patch

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;