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

[Daejun Park <daejun7.park@samsung.com>]

Hi Bart,

>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@samsung.com>
>>  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).

OK, I will change it.

> 
>> +What:                /sys/bus/platform/drivers/ufshcd/*/attributes/max_data_size_hpb_single_cmd
>> +Date:                June 2021
>> +Contact:        Daejun Park <daejun7.park@samsung.com>
>> +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.

We show the bMAX_DATA_SIZE_FOR_SINGLE_CMD value just like other existing
ufs sysfs attributes. In the case of sysfs for the bRefClkFreq value, it
shows the raw value read from the device, not the value converted to HZ.

>> 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.

OK, it will be u8 and the comments will be as follows.
@max_hpb_single_cmd: the value of bMAX_DATA_SIZE_FOR_SINGLE_CMD.

> 
>> +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.

OK, I will add:
/* HPB version 1.0 is called as legacy version. */

> 
>> +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'.

Sure.

> 
>> +/* 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.

OK, I will add it.

Thanks,
Daejun

>Thanks,
> 
>Bart.
> 
> 
>