Re: [PATCH v36 4/4] scsi: ufs: Add HPB 2.0 support

[Bart Van Assche]

On 6/6/21 9:19 PM, Daejun Park wrote:
> -What:		/sys/class/scsi_device/*/device/hpb_sysfs/hit_cnt
> +What:		/sys/class/scsi_device/*/device/hpb_stat_sysfs/hit_cnt
>  Date:		June 2021
>  Contact:	Daejun Park <daejun7.park@xxxxxxxxxxx>
>  Description:	This entry shows the number of reads that changed to HPB read.
>  
>  		The file is read only.

Is it really useful to have a suffix "_sysfs" for a directory that
occurs in sysfs? If not, please leave it out.

Should "hpb_stat" perhaps be renamed into "hpb_stats"? An example of
another directory with statistics is /sys/power/suspend_stats. The name
of that directory also ends in "stats" (plural form).

> +What:		/sys/bus/platform/drivers/ufshcd/*/attributes/max_data_size_hpb_single_cmd
> +Date:		June 2021
> +Contact:	Daejun Park <daejun7.park@xxxxxxxxxxx>
> +Description:	This entry shows the maximum HPB data size for using single HPB
> +		command.
> +
> +		===  ========
> +		00h  4KB
> +		01h  8KB
> +		02h  12KB
> +		...
> +		FFh  1024KB
> +		===  ========
> +
> +		The file is read only.

This is not clear enough. What are the values reported through this
sysfs attribute? Are that perhaps the values 00h .. FFh? Is the software
that reads this attribute perhaps expected to convert this attribute
from hex to int and next convert the int into a size in KB by using the
above lookup table? That seems awkward to me. Please report the maximum
data size directly, either in KB or in bytes.

> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index f99059b31e0a..d902414e4a6f 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -652,6 +652,8 @@ struct ufs_hba_variant_params {
>   * @srgn_size: device reported HPB sub-region size
>   * @slave_conf_cnt: counter to check all lu finished initialization
>   * @hpb_disabled: flag to check if HPB is disabled
> + * @max_hpb_single_cmd: maximum size of single HPB command
> + * @is_legacy: flag to check HPB 1.0
>   */
>  struct ufshpb_dev_info {
>  	int num_lu;
> @@ -659,6 +661,8 @@ struct ufshpb_dev_info {
>  	int srgn_size;
>  	atomic_t slave_conf_cnt;
>  	bool hpb_disabled;
> +	int max_hpb_single_cmd;
> +	bool is_legacy;
>  };
>  #endif

Elsewhere in this patch I see that max_hpb_single_cmd is the value read
from the bMAX_DATA_SIZE_FOR_SINGLE_CMD descriptor (one byte). Does this
mean that the type of 'max_hpb_single_cmd' should be changed into
uint8_t? Additionally, please make it clear in the comment block above
struct ufshpb_dev_info that max_hpb_single_cmd is not a size in bytes.

> +bool ufshpb_is_legacy(struct ufs_hba *hba)
> +{
> +	return hba->ufshpb_dev.is_legacy;
> +}

Please add a comment above this function that explains what 'legacy'
means in the context of HPB.

> +static int ufshpb_execute_umap_req(struct ufshpb_lu *hpb,
> +				   struct ufshpb_req *umap_req,
> +				   struct ufshpb_region *rgn)
> +{
> +	struct request *req;
> +	struct scsi_request *rq;
> +
> +	req = umap_req->req;
> +	req->timeout = 0;
> +	req->end_io_data = (void *)umap_req;
> +	rq = scsi_req(req);
> +	ufshpb_set_unmap_cmd(rq->cmd, rgn);
> +	rq->cmd_len = HPB_WRITE_BUFFER_CMD_LENGTH;
> +
> +	blk_execute_rq_nowait(NULL, req, 1, ufshpb_umap_req_compl_fn);
> +
> +	return 0;
> +}

This function always returns 0. Please change the return type from 'int'
into 'void'.

> +/* SYSFS functions */
> +#define ufshpb_sysfs_param_show_func(__name)				\
> +static ssize_t __name##_show(struct device *dev,			\
> +	struct device_attribute *attr, char *buf)			\
> +{									\
> +	struct scsi_device *sdev = to_scsi_device(dev);			\
> +	struct ufshpb_lu *hpb = ufshpb_get_hpb_data(sdev);		\
> +	if (!hpb)							\
> +		return -ENODEV;						\
> +									\
> +	return sysfs_emit(buf, "%d\n", hpb->params.__name);		\
> +}

Please leave a blank line between variable declarations and code.

Thanks,

Bart.