lkml.org 
[lkml]   [2020]   [Jan]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/3] scsi: ufs: initialize max_lu_supported while booting
From
Date
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.


\
 
 \ /
  Last update: 2020-01-11 23:43    [W:0.057 / U:0.404 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site