RE: [PATCH v3 4/6] scsi: ufshpb: Change sysfs node hpb_stats/rb_* prefix to start with rcmd_*

Keoseong Park
2022-04-25T04:10:58Z

>From: Bean Huo <beanhuo@micron.com>
>
>According to the documentation of the sysfs nodes rb_noti_cnt, rb_active_cnt
>and rb_inactive_cnt, they are all related to HPB recommendation in UPIU response
>packet. I don't know what 'rb' refers to, I think 'rcmd'
>(recommendation) should be the correct abbreviation.
>
>Change the sysfs documentation about these sysfs nodes to highlight what they mean
>under different HPB control modes.
I think it's better than what I suggested.

>
>Signed-off-by: Bean Huo <beanhuo@micron.com>
>---
> Documentation/ABI/testing/sysfs-driver-ufs | 18 +++++++++-------
> drivers/scsi/ufs/ufshpb.c | 24 +++++++++++-----------
> drivers/scsi/ufs/ufshpb.h | 6 +++---
> 3 files changed, 26 insertions(+), 22 deletions(-)
>
>diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
>index a44ef8bfbadf..6b248abb1bd7 100644
>--- a/Documentation/ABI/testing/sysfs-driver-ufs
>+++ b/Documentation/ABI/testing/sysfs-driver-ufs
>@@ -1518,7 +1518,7 @@ Description: This entry shows the number of reads that cannot be changed to
>
> The file is read only.
>
>-What: /sys/class/scsi_device/*/device/hpb_stats/rb_noti_cnt
>+What: /sys/class/scsi_device/*/device/hpb_stats/rcmd_noti_cnt
> Date: June 2021
> Contact: Daejun Park <daejun7.park@samsung.com>
> Description: This entry shows the number of response UPIUs that has
>@@ -1526,19 +1526,23 @@ Description: This entry shows the number of response UPIUs that has
>
> The file is read only.
>
>-What: /sys/class/scsi_device/*/device/hpb_stats/rb_active_cnt
>+What: /sys/class/scsi_device/*/device/hpb_stats/rcmd_active_cnt
> Date: June 2021
> Contact: Daejun Park <daejun7.park@samsung.com>
>-Description: This entry shows the number of active sub-regions recommended by
>- response UPIUs.
>+Description: For the HPB device control mode, this entry shows the number of
>+ active sub-regions recommended by response UPIUs. For the HPB host control
>+ mode, this entry shows the number of active sub-regions recommended by the
>+ HPB host control mode heuristic algorithm.
Please check indentation.

>
> The file is read only.
>
>-What: /sys/class/scsi_device/*/device/hpb_stats/rb_inactive_cnt
>+What: /sys/class/scsi_device/*/device/hpb_stats/rcmd_inactive_cnt
> Date: June 2021
> Contact: Daejun Park <daejun7.park@samsung.com>
>-Description: This entry shows the number of inactive regions recommended by
>- response UPIUs.
>+Description: For the HPB device control mode, this entry shows the number of
>+ inactive regions recommended by response UPIUs. For the HPB host control
>+ mode, this entry shows the number of inactive regions recommended by the
>+ HPB host control mode heuristic algorithm.
Please check indentation.

>
> The file is read only.
>
>diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
>index f1f30d4c3d65..e7f311bb4401 100644
>--- a/drivers/scsi/ufs/ufshpb.c
>+++ b/drivers/scsi/ufs/ufshpb.c
>@@ -563,7 +563,7 @@ static void ufshpb_update_active_info(struct ufshpb_lu *hpb, int rgn_idx,
> if (list_empty(&srgn->list_act_srgn))
> list_add_tail(&srgn->list_act_srgn, &hpb->lh_act_srgn);
>
>- hpb->stats.rb_active_cnt++;
>+ hpb->stats.rcmd_active_cnt++;
> }
>
> static void ufshpb_update_inactive_info(struct ufshpb_lu *hpb, int rgn_idx)
>@@ -580,7 +580,7 @@ static void ufshpb_update_inactive_info(struct ufshpb_lu *hpb, int rgn_idx)
> if (list_empty(&rgn->list_inact_rgn))
> list_add_tail(&rgn->list_inact_rgn, &hpb->lh_inact_rgn);
>
>- hpb->stats.rb_inactive_cnt++;
>+ hpb->stats.rcmd_inactive_cnt++;
> }
>
> static void ufshpb_activate_subregion(struct ufshpb_lu *hpb,
>@@ -1321,7 +1321,7 @@ void ufshpb_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
> if (!ufshpb_is_hpb_rsp_valid(hba, lrbp, rsp_field))
> return;
>
>- hpb->stats.rb_noti_cnt++;
>+ hpb->stats.rcmd_noti_cnt++;
>
> switch (rsp_field->hpb_op) {
> case HPB_RSP_REQ_REGION_UPDATE:
>@@ -1724,18 +1724,18 @@ static DEVICE_ATTR_RO(__name)
>
> ufshpb_sysfs_attr_show_func(hit_cnt);
> ufshpb_sysfs_attr_show_func(miss_cnt);
>-ufshpb_sysfs_attr_show_func(rb_noti_cnt);
>-ufshpb_sysfs_attr_show_func(rb_active_cnt);
>-ufshpb_sysfs_attr_show_func(rb_inactive_cnt);
>+ufshpb_sysfs_attr_show_func(rcmd_noti_cnt);
>+ufshpb_sysfs_attr_show_func(rcmd_active_cnt);
>+ufshpb_sysfs_attr_show_func(rcmd_inactive_cnt);
> ufshpb_sysfs_attr_show_func(map_req_cnt);
> ufshpb_sysfs_attr_show_func(umap_req_cnt);
>
> static struct attribute *hpb_dev_stat_attrs[] = {
> &dev_attr_hit_cnt.attr,
> &dev_attr_miss_cnt.attr,
>- &dev_attr_rb_noti_cnt.attr,
>- &dev_attr_rb_active_cnt.attr,
>- &dev_attr_rb_inactive_cnt.attr,
>+ &dev_attr_rcmd_noti_cnt.attr,
>+ &dev_attr_rcmd_active_cnt.attr,
>+ &dev_attr_rcmd_inactive_cnt.attr,
> &dev_attr_map_req_cnt.attr,
> &dev_attr_umap_req_cnt.attr,
> NULL,
>@@ -2098,9 +2098,9 @@ static void ufshpb_stat_init(struct ufshpb_lu *hpb)
> {
> hpb->stats.hit_cnt = 0;
> hpb->stats.miss_cnt = 0;
>- hpb->stats.rb_noti_cnt = 0;
>- hpb->stats.rb_active_cnt = 0;
>- hpb->stats.rb_inactive_cnt = 0;
>+ hpb->stats.rcmd_noti_cnt = 0;
>+ hpb->stats.rcmd_active_cnt = 0;
>+ hpb->stats.rcmd_inactive_cnt = 0;
> hpb->https://protect2.fireeye.com/v1/url?k=9b807207-fa0b6728-9b81f948-74fe485cbfe7-c8e650b167f03dee&q=1&e=ec9d9a5f-605d-45e5-b145-e3a4c9a96f3e&u=http%3A%2F%2Fstats.map%2F_req_cnt = 0;
It's not related to the patch, but the code seems to be wrong.

> hpb->stats.umap_req_cnt = 0;
> }
>diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
>index b83b9ec9044a..0d6e6004d783 100644
>--- a/drivers/scsi/ufs/ufshpb.h
>+++ b/drivers/scsi/ufs/ufshpb.h
>@@ -211,9 +211,9 @@ struct ufshpb_params {
> struct ufshpb_stats {
> u64 hit_cnt;
> u64 miss_cnt;
>- u64 rb_noti_cnt;
>- u64 rb_active_cnt;
>- u64 rb_inactive_cnt;
>+ u64 rcmd_noti_cnt;
>+ u64 rcmd_active_cnt;
>+ u64 rcmd_inactive_cnt;
> u64 map_req_cnt;
> u64 pre_req_cnt;
> u64 umap_req_cnt;
>--
>2.34.1
>
>
Apart from the indentation, the patch looks good to me, so once
you've fixed them, feel free to add my:

Reviewed-by: Keoseong Park <keosung.park@samsung.com>

Best Regards,
Keoseong Park