| Message ID | 20220419183044.789065-5-huobean@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Several changes for UFSHPB | expand |
Hi Bean, >From: Bean Huo <beanhuo@micron.com> > >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@micron.com> >--- > 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
On Wed, 2022-04-20 at 14:31 +0900, Keoseong Park wrote: > > +/** > > + *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(). based on Documentation/ABI/testing/sysfs-driver-ufs: "What: /sys/class/scsi_device/*/device/hpb_stats/rb_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." This parameter should be increased only when recieving inactive recommended. I will change it in the next version, thanks. Kind regards, Bean
> On Wed, 2022-04-20 at 14:31 +0900, Keoseong Park wrote: > > > > > > + /* > > > + * 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(). Hi Keoseong Park, I didn't take this hpb->stats.rb_inactive_cnt++ out, since I think if the host receives HPB operation:02h, which means the device recommends the host to de-activate all active regions on the host side. we need to add these de-activations to this parameter because the device has inactivated all active regions. Otherwise, we will find an inconsistent inactivation and activation counter. Please have a look a review the v3. Kind regards, Bean
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); + 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 * 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, + * 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); + } } }