Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Mime-Version: 1.0
Subject: RE: [PATCH v3 4/6] scsi: ufshpb: Change sysfs node hpb_stats/rb_*
 prefix to start with rcmd_*
Reply-To: keosung.park@samsung.com
Sender: Keoseong Park <keosung.park@samsung.com>
From:   Keoseong Park <keosung.park@samsung.com>
To:     Bean Huo <huobean@gmail.com>,
        ALIM AKHTAR <alim.akhtar@samsung.com>,
        "avri.altman@wdc.com" <avri.altman@wdc.com>,
        "jejb@linux.ibm.com" <jejb@linux.ibm.com>,
        "martin.petersen@oracle.com" <martin.petersen@oracle.com>,
        "stanley.chu@mediatek.com" <stanley.chu@mediatek.com>,
        "beanhuo@micron.com" <beanhuo@micron.com>,
        "bvanassche@acm.org" <bvanassche@acm.org>,
        "tomas.winkler@intel.com" <tomas.winkler@intel.com>,
        Daejun Park <daejun7.park@samsung.com>,
        Keoseong Park <keosung.park@samsung.com>,
        "peter.wang@mediatek.com" <peter.wang@mediatek.com>,
        "powen.kao@mediatek.com" <powen.kao@mediatek.com>,
        cpgsproxy3 <cpgsproxy3@samsung.com>
CC:     "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
In-Reply-To: <20220424220713.1253049-5-huobean@gmail.com>
Message-ID: <1891546521.01650859681867.JavaMail.epsvc@epcpadp4>
Date:   Mon, 25 Apr 2022 12:43:53 +0900
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="utf-8"
References: <20220424220713.1253049-5-huobean@gmail.com>
        <20220424220713.1253049-1-huobean@gmail.com>
        <CGME20220424220757epcas2p37d7fcf3c58b1c29291f6e6765e6690ed@epcms2p3>
Precedence: bulk

>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