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