RE: [PATCH v2 4/5] scsi: ufshpb: Add handing of device reset HPB regions Infos in HPB device mode

[Keoseong Park]

Hi Bean,

>From: Bean Huo <beanhuo@xxxxxxxxxx>
> 
>In UFS HPB Spec JESD220-3A,
> 
>"5.8. Active and inactive information upon power cycle
>...
>When the device is powered off by the host, the device may restore L2P map data
>upon power up or build from the host’s HPB READ command. In case device powered
>up and lost HPB information, device can signal to the host through HPB Sense data,
>by setting HPB Operation as ‘2’ which will inform the host that device reset HPB
>information."
> 
>Therefore, for HPB device control mode, if the UFS device is reset via the RST_N
>pin, the active region information in the device will be reset. If the host side
>receives this notification from the device side, it is recommended to inactivate
>all active regions in the host's HPB cache.
> 
>Signed-off-by: Bean Huo <beanhuo@xxxxxxxxxx>
>---
> drivers/scsi/ufs/ufshpb.c | 73 ++++++++++++++++++++++++++++-----------
> 1 file changed, 53 insertions(+), 20 deletions(-)
> 
>diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
>index 4538164fc493..4b15fa862beb 100644
>--- a/drivers/scsi/ufs/ufshpb.c
>+++ b/drivers/scsi/ufs/ufshpb.c
>@@ -1143,6 +1143,39 @@ static int ufshpb_add_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn)
>         spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
>         return ret;
> }
>+/**
>+ *ufshpb_submit_region_inactive() - submit a region to be inactivated later
>+ *@hpb: per-LU HPB instance
>+ *@region_index: the index associated with the region that will be inactivated later
>+ */
>+static void ufshpb_submit_region_inactive(struct ufshpb_lu *hpb, int region_index)
>+{
>+        int subregion_index;
>+        struct ufshpb_region *rgn;
>+        struct ufshpb_subregion *srgn;
>+
>+        /*
>+         * Remove this region from active region list and add it to inactive list
>+         */
>+        spin_lock(&hpb->rsp_list_lock);
>+        ufshpb_update_inactive_info(hpb, region_index);
How about separating the "hpb->stats.rb_inactive_cnt++" code from ufshpb_update_inactive_info()?
Because I think this code should only be used in ufshpb_rsp_req_region_update().

>+        spin_unlock(&hpb->rsp_list_lock);
>+
>+        rgn = hpb->rgn_tbl + region_index;
>+
>+        /*
>+         * Set subregion state to be HPB_SRGN_INVALID, there will no HPB read on this subregion
>+         */
>+        spin_lock(&hpb->rgn_state_lock);
>+        if (rgn->rgn_state != HPB_RGN_INACTIVE) {
>+                for (subregion_index = 0; subregion_index < rgn->srgn_cnt; subregion_index++) {
>+                        srgn = rgn->srgn_tbl + subregion_index;
>+                        if (srgn->srgn_state == HPB_SRGN_VALID)
>+                                srgn->srgn_state = HPB_SRGN_INVALID;
>+                }
>+        }
>+        spin_unlock(&hpb->rgn_state_lock);
>+}
> 
> static void ufshpb_rsp_req_region_update(struct ufshpb_lu *hpb,
>                                          struct utp_hpb_rsp *rsp_field)
>@@ -1202,25 +1235,8 @@ static void ufshpb_rsp_req_region_update(struct ufshpb_lu *hpb,
> 
>         for (i = 0; i < rsp_field->inactive_rgn_cnt; i++) {
>                 rgn_i = be16_to_cpu(rsp_field->hpb_inactive_field[i]);
>-                dev_dbg(&hpb->sdev_ufs_lu->sdev_dev,
>-                        "inactivate(%d) region %d\n", i, rgn_i);
>-
>-                spin_lock(&hpb->rsp_list_lock);
>-                ufshpb_update_inactive_info(hpb, rgn_i);
>-                spin_unlock(&hpb->rsp_list_lock);
>-
>-                rgn = hpb->rgn_tbl + rgn_i;
>-
>-                spin_lock(&hpb->rgn_state_lock);
>-                if (rgn->rgn_state != HPB_RGN_INACTIVE) {
>-                        for (srgn_i = 0; srgn_i < rgn->srgn_cnt; srgn_i++) {
>-                                srgn = rgn->srgn_tbl + srgn_i;
>-                                if (srgn->srgn_state == HPB_SRGN_VALID)
>-                                        srgn->srgn_state = HPB_SRGN_INVALID;
>-                        }
>-                }
>-                spin_unlock(&hpb->rgn_state_lock);
>-
>+                dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "inactivate(%d) region %d\n", i, rgn_i);
>+                ufshpb_submit_region_inactive(hpb, rgn_i);
>         }
> 
> out:
>@@ -1255,7 +1271,10 @@ static void ufshpb_dev_reset_handler(struct ufs_hba *hba)
> 
>         __shost_for_each_device(sdev, hba->host) {
>                 hpb = ufshpb_get_hpb_data(sdev);
>-                if (hpb && hpb->is_hcm)
>+                if (!hpb)
>+                        continue;
>+
>+                if (hpb->is_hcm) {
>                         /*
>                          * For the HPB host mode, in case device powered up and lost HPB
For the HPB host control mode, ...

>                          * information, we will set the region flag to be RGN_FLAG_UPDATE,
>@@ -1263,6 +1282,20 @@ static void ufshpb_dev_reset_handler(struct ufs_hba *hba)
>                          * in the UFS device).
>                          */
>                         ufshpb_set_regions_update(hpb);
>+                } else {
>+                        /*
>+                         * For the HPB device mode, we add all active regions to inactive list,
For the HPB device control mode, ...

>+                         * they will be inactivated later in ufshpb_map_work_handler()
>+                         */
>+                        struct victim_select_info *lru_info = &hpb->lru_info;
>+                        struct ufshpb_region *rgn;
>+
>+                        list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn)
>+                                ufshpb_submit_region_inactive(hpb, rgn->rgn_idx);
>+
>+                        if (ufshpb_get_state(hpb) == HPB_PRESENT)
>+                                queue_work(ufshpb_wq, &hpb->map_work);
>+                }
>         }
> }
> 
>-- 
>2.34.1
> 
> 

Best Regards,
Keoseong Park