From: Bean Huo <[email protected]>
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 <[email protected]>
---
drivers/scsi/ufs/ufshpb.c | 82 +++++++++++++++++++++++++++------------
1 file changed, 58 insertions(+), 24 deletions(-)
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index e7f311bb4401..7868412054bf 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -1137,6 +1137,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);
+ 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)
@@ -1196,25 +1229,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:
@@ -1249,14 +1265,32 @@ 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
- * information, we will set the region flag to be RGN_FLAG_UPDATE,
- * it will let host reload its L2P entries(re-activate the region
- * in the UFS device).
+ * For the HPB host control mode, in case device powered up and lost HPB
+ * information, we will set the region flag to be RGN_FLAG_UPDATE, it will
+ * let host reload its L2P entries(reactivate region in the UFS device).
*/
ufshpb_set_regions_update(hpb);
+ } else {
+ /*
+ * For the HPB device control mode, if host side receives 02h:HPB Operation
+ * in UPIU response, which means device recommends the host side should
+ * inactivate all active regions. Here we add all active regions to inactive
+ * list, 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
>From: Bean Huo <[email protected]>
>
>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 <[email protected]>
>---
> drivers/scsi/ufs/ufshpb.c | 82 +++++++++++++++++++++++++++------------
> 1 file changed, 58 insertions(+), 24 deletions(-)
>
>diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
>index e7f311bb4401..7868412054bf 100644
>--- a/drivers/scsi/ufs/ufshpb.c
>+++ b/drivers/scsi/ufs/ufshpb.c
>@@ -1137,6 +1137,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);
>+ 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)
>@@ -1196,25 +1229,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:
>@@ -1249,14 +1265,32 @@ 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
>- * information, we will set the region flag to be RGN_FLAG_UPDATE,
>- * it will let host reload its L2P entries(re-activate the region
>- * in the UFS device).
>+ * For the HPB host control mode, in case device powered up and lost HPB
>+ * information, we will set the region flag to be RGN_FLAG_UPDATE, it will
>+ * let host reload its L2P entries(reactivate region in the UFS device).
> */
> ufshpb_set_regions_update(hpb);
>+ } else {
>+ /*
>+ * For the HPB device control mode, if host side receives 02h:HPB Operation
>+ * in UPIU response, which means device recommends the host side should
>+ * inactivate all active regions. Here we add all active regions to inactive
>+ * list, 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
>
>
Looks good to me.
Reviewed-by: Keoseong Park <[email protected]>
Best Regards,
Keoseong Park
On Mon, 2022-04-25 at 06:43 +0000, Avri Altman wrote:
> >
> > 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.
> While it makes sense to me, this interpretation of the spec takes
> extreme action,
> Compared to what it is today, and you probably need to get an ack
> from Daejun.
>
Yes, but it seems he is quiet. Keoseong Park is on the email list and
he has looked at the patch. If you have concerns, I can ping Keoseong
Park to sync with Daejun.
> If we are entering a zone in which each vendor is applying a
> different logic -
> I think you need to facilitate that - maybe in a different patch set.
>
Yes, it should be a different patch. I must say that this patch is
based on the HPB Spec, say, if the device loses HPB info. The host side
should also reset its HPB information, Otherwise, performance will
degrade. Of course, it depends on the specific implementation of the
vendor. If they don't report 02:HPB operations, this patch still
doesn't change the original behavior.
Kind regards,
Bean