On 10/18/22 11:16, Bean Huo wrote:
> +static void ufshcd_lu_init(struct ufs_hba *hba, struct scsi_device *sdev)
> +{
> + u8 lun_qdepth;
> + u8 *desc_buf;
> + int ret;
> + int len;
> + u8 lun;
> +
> + lun_qdepth = hba->nutrs;
> + lun = ufshcd_scsi_to_upiu_lun(sdev->lun);
> + len = hba->desc_size[QUERY_DESC_IDN_UNIT];
Isn't the preferred style for kernel code to combine the above
declarations and assignments (but not memory allocation calls)?
> + desc_buf = kzalloc(len, GFP_KERNEL);
> + if (!desc_buf)
> + goto set_qdepth;
> +
> + ret = ufshcd_read_unit_desc_param(hba, lun, 0, desc_buf, len);
> + if (ret < 0) {
> + if (ret == -EOPNOTSUPP)
> + /* If LU doesn't support unit descriptor, its queue depth is set to 1 */
> + lun_qdepth = 1;
> + kfree(desc_buf);
> + goto set_qdepth;
> + }
> +
> + if (desc_buf[UNIT_DESC_PARAM_LU_Q_DEPTH])
> + /*
> + * In per-LU queueing architecture, bLUQueueDepth will not be 0, then we will
> + * use the smaller between UFSHCI CAP.NUTRS and UFS LU bLUQueueDepth
> + */
> + lun_qdepth = min_t(int, desc_buf[UNIT_DESC_PARAM_LU_Q_DEPTH], hba->nutrs);
Should a test be added that verifies that UNIT_DESC_PARAM_LU_Q_DEPTH < len?
Additionally, please use braces ({}) around multi-line if-statement bodies.
> + /*
> + * According to UFS device specification, the write protection mode is only supported by
> + * normal LU, not supported by WLUN.
> + */
> + if (hba->dev_info.f_power_on_wp_en && lun < hba->dev_info.max_lu_supported &&
> + !hba->dev_info.is_lu_power_on_wp &&
> + desc_buf[UNIT_DESC_PARAM_LU_WR_PROTECT] == UFS_LU_POWER_ON_WP)
> + hba->dev_info.is_lu_power_on_wp = true;
Also here, should the following test be added:
UNIT_DESC_PARAM_LU_WR_PROTECT < len?
Otherwise this patch looks good to me.
Thanks,
Bart.
On Tue, 2022-10-18 at 11:27 -0700, Bart Van Assche wrote:
> Should a test be added that verifies that UNIT_DESC_PARAM_LU_Q_DEPTH
> < len?
>
> Additionally, please use braces ({}) around multi-line if-statement
> bodies.
>
> > + /*
> > + * According to UFS device specification, the write
> > protection mode is only supported by
> > + * normal LU, not supported by WLUN.
> > + */
> > + if (hba->dev_info.f_power_on_wp_en && lun < hba-
> > >dev_info.max_lu_supported &&
> > + !hba->dev_info.is_lu_power_on_wp &&
> > + desc_buf[UNIT_DESC_PARAM_LU_WR_PROTECT] ==
> > UFS_LU_POWER_ON_WP)
> > + hba->dev_info.is_lu_power_on_wp = true;
>
> Also here, should the following test be added:
> UNIT_DESC_PARAM_LU_WR_PROTECT < len?
>
> Otherwise this patch looks good to me.
Bart,
len is already the LU descriptor size. UNIT_DESC_PARAM_LU_Q_DEPTH (0x6)
and UNIT_DESC_PARAM_LU_WR_PROTECT(0x5) are very basic parameters in the
LU descriptor, no matter what version of UFS is, there are these two
parameters in the descriptor. So the return value of
ufshcd_read_unit_desc_param() will be -EOPNOTSUPP, -EINVAL, -ENOMEM or
0 (successful read of LU descriptor). the checkup of "< len" is not
neccessary.
Kind regards,
Bean