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.
