RE: [EXT] Re: [PATCH 2/3] scsi: ufs: initialize max_lu_supported while booting

[Bean Huo (beanhuo)]

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