lkml.org 
[lkml]   [2021]   [Jun]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v36 4/4] scsi: ufs: Add HPB 2.0 support
    From
    Date
    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).

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

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

    \
     
     \ /
      Last update: 2021-06-10 05:38    [W:2.529 / U:0.144 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site