| Message ID | 20220412073131.10644-1-powen.kao@mediatek.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/1] scsi: ufs: remove redundant HPB unmap | expand |
On Tue, 2022-04-12 at 15:31 +0800, Po-Wen Kao wrote: > Since the HPB mapping is already reset in ufshpb_init by setting > flag QUERY_FLAG_IDN_HPB_RESET, there is no need doing so again in > ufshpb_hpb_lu_prepared. > > This would also resolve the issue where HPB WRTIE BUFFER is issued > before UAC being cleared. > > Signed-off-by: Po-Wen Kao <powen.kao@mediatek.com> > --- > drivers/scsi/ufs/ufshpb.c | 7 ------- > 1 file changed, 7 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c > index a86d0cc50de2..5c09d44c4bd5 100644 > --- a/drivers/scsi/ufs/ufshpb.c > +++ b/drivers/scsi/ufs/ufshpb.c > @@ -934,11 +934,6 @@ static int ufshpb_issue_umap_single_req(struct > ufshpb_lu *hpb, > return ufshpb_issue_umap_req(hpb, rgn, true); > } > > -static int ufshpb_issue_umap_all_req(struct ufshpb_lu *hpb) > -{ > - return ufshpb_issue_umap_req(hpb, NULL, false); > -} > - > static void __ufshpb_evict_region(struct ufshpb_lu *hpb, > struct ufshpb_region *rgn) > { > @@ -2459,8 +2454,6 @@ static void ufshpb_hpb_lu_prepared(struct > ufs_hba *hba) > ufshpb_set_state(hpb, HPB_PRESENT); > if ((hpb->lu_pinned_end - hpb- > >lu_pinned_start) > 0) > queue_work(ufshpb_wq, &hpb- > >map_work); > - if (!hpb->is_hcm) > - ufshpb_issue_umap_all_req(hpb); > } else { > dev_err(hba->dev, "destroy HPB lu %d\n", hpb- > >lun); > ufshpb_destroy_lu(hba, sdev); Hi Po-Wen, Yes, it is redundant in this flow, but it is needed in the reset flow, I built this cleanup patch, but don't know if it can fix the issue where HPB WRTIE BUFFER is issued before UAC being cleared. would you test on your platform? I will verify it on our UFS later. Subject: [PATCH] scsi: ufshpb: UFSHPB cleanup Remove redundant ufshpb_reset*, and merge into a single helper ufshbp_state_toggle(). Delete the redundant Inactivation code of all HPB Regions in the cold boot stage, and add inactivating all HPB Regions when the HPP status changes from HPB_RESET to HBP_PRESENT. Signed-off-by: Bean Huo <beanhuo@micron.com> --- drivers/scsi/ufs/ufshcd.c | 4 ++-- drivers/scsi/ufs/ufshpb.c | 38 +++++++++++++------------------------- drivers/scsi/ufs/ufshpb.h | 6 ++---- 3 files changed, 17 insertions(+), 31 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 0899d5b8cdad..d8b59d017ce4 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -7223,7 +7223,7 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba) * Stop the host controller and complete the requests * cleared by h/w */ - ufshpb_reset_host(hba); + ufshpb_state_toggle(hba, HPB_RESET); ufshcd_hba_stop(hba); hba->silence_err_logs = true; ufshcd_complete_requests(hba); @@ -8184,7 +8184,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params) /* Enable Auto-Hibernate if configured */ ufshcd_auto_hibern8_enable(hba); - ufshpb_reset(hba); + ufshpb_state_toggle(hba, HPB_PRESENT); out: spin_lock_irqsave(hba->host->host_lock, flags); if (ret) diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c index 3ca745ad616c..4ed156031413 100644 --- a/drivers/scsi/ufs/ufshpb.c +++ b/drivers/scsi/ufs/ufshpb.c @@ -2278,39 +2278,29 @@ static bool ufshpb_check_hpb_reset_query(struct ufs_hba *hba) return flag_res; } -void ufshpb_reset(struct ufs_hba *hba) +void ufshpb_state_toggle(struct ufs_hba *hba, enum UFSHPB_STATE state) { struct ufshpb_lu *hpb; struct scsi_device *sdev; shost_for_each_device(sdev, hba->host) { hpb = ufshpb_get_hpb_data(sdev); - if (!hpb) - continue; - if (ufshpb_get_state(hpb) != HPB_RESET) + if (!hpb || ufshpb_get_state(hpb) != state) continue; - ufshpb_set_state(hpb, HPB_PRESENT); - } -} - -void ufshpb_reset_host(struct ufs_hba *hba) -{ - struct ufshpb_lu *hpb; - struct scsi_device *sdev; - - shost_for_each_device(sdev, hba->host) { - hpb = ufshpb_get_hpb_data(sdev); - if (!hpb) - continue; - - if (ufshpb_get_state(hpb) != HPB_PRESENT) - continue; - ufshpb_set_state(hpb, HPB_RESET); - ufshpb_cancel_jobs(hpb); - ufshpb_discard_rsp_lists(hpb); + ufshpb_set_state(hpb, state); + if (state == HPB_RESET) { + ufshpb_cancel_jobs(hpb); + ufshpb_discard_rsp_lists(hpb); + } } + /* + * Inactivating all HPB Region in device side in case HPB state changed + * from HPB_RESET to HPB_PRESENT + */ + if (!hpb->is_hcm && state == HPB_PRESENT) + ufshpb_issue_umap_all_req(hpb); } void ufshpb_suspend(struct ufs_hba *hba) @@ -2456,8 +2446,6 @@ static void ufshpb_hpb_lu_prepared(struct ufs_hba *hba) ufshpb_set_state(hpb, HPB_PRESENT); if ((hpb->lu_pinned_end - hpb- >lu_pinned_start) > 0) queue_work(ufshpb_wq, &hpb->map_work); - if (!hpb->is_hcm) - ufshpb_issue_umap_all_req(hpb); } else { dev_err(hba->dev, "destroy HPB lu %d\n", hpb- >lun); ufshpb_destroy_lu(hba, sdev); diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h index b475dbd78988..a130f0b16c3e 100644 --- a/drivers/scsi/ufs/ufshpb.h +++ b/drivers/scsi/ufs/ufshpb.h @@ -288,8 +288,7 @@ static int ufshpb_prep(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) { return 0; static void ufshpb_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) {} static void ufshpb_resume(struct ufs_hba *hba) {} static void ufshpb_suspend(struct ufs_hba *hba) {} -static void ufshpb_reset(struct ufs_hba *hba) {} -static void ufshpb_reset_host(struct ufs_hba *hba) {} +static void ufshpb_state_toggle(struct ufs_hba *hba, enum UFSHPB_STATE state) {} static void ufshpb_init(struct ufs_hba *hba) {} static void ufshpb_init_hpb_lu(struct ufs_hba *hba, struct scsi_device *sdev) {} static void ufshpb_destroy_lu(struct ufs_hba *hba, struct scsi_device *sdev) {} @@ -303,8 +302,7 @@ int ufshpb_prep(struct ufs_hba *hba, struct ufshcd_lrb *lrbp); void ufshpb_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp); void ufshpb_resume(struct ufs_hba *hba); void ufshpb_suspend(struct ufs_hba *hba); -void ufshpb_reset(struct ufs_hba *hba); -void ufshpb_reset_host(struct ufs_hba *hba); +void ufshpb_state_toggle(struct ufs_hba *hba, enum UFSHPB_STATE state); void ufshpb_init(struct ufs_hba *hba); void ufshpb_init_hpb_lu(struct ufs_hba *hba, struct scsi_device *sdev); void ufshpb_destroy_lu(struct ufs_hba *hba, struct scsi_device *sdev);
Hi, Po-Wen, Previous email did not properly append the patch, this one is better for your review and test. From: Bean Huo <beanhuo@micron.com> Date: Tue, 12 Apr 2022 16:56:34 +0200 Subject: [PATCH] scsi: ufshpb: UFSHPB cleanup Remove redundant ufshpb_reset*, and merge into a single helper ufshbp_state_toggle(). Delete the redundant Inactivation code of all HPB Regions in the cold boot stage, and add inactivating all HPB Regions when the HPP status changes from HPB_RESET to HBP_PRESENT. Signed-off-by: Bean Huo <beanhuo@micron.com> --- drivers/scsi/ufs/ufshcd.c | 4 ++-- drivers/scsi/ufs/ufshpb.c | 38 +++++++++++++------------------------- drivers/scsi/ufs/ufshpb.h | 6 ++---- 3 files changed, 17 insertions(+), 31 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 0899d5b8cdad..d8b59d017ce4 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -7223,7 +7223,7 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba) * Stop the host controller and complete the requests * cleared by h/w */ - ufshpb_reset_host(hba); + ufshpb_state_toggle(hba, HPB_RESET); ufshcd_hba_stop(hba); hba->silence_err_logs = true; ufshcd_complete_requests(hba); @@ -8184,7 +8184,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params) /* Enable Auto-Hibernate if configured */ ufshcd_auto_hibern8_enable(hba); - ufshpb_reset(hba); + ufshpb_state_toggle(hba, HPB_PRESENT); out: spin_lock_irqsave(hba->host->host_lock, flags); if (ret) diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c index 3ca745ad616c..4ed156031413 100644 --- a/drivers/scsi/ufs/ufshpb.c +++ b/drivers/scsi/ufs/ufshpb.c @@ -2278,39 +2278,29 @@ static bool ufshpb_check_hpb_reset_query(struct ufs_hba *hba) return flag_res; } -void ufshpb_reset(struct ufs_hba *hba) +void ufshpb_state_toggle(struct ufs_hba *hba, enum UFSHPB_STATE state) { struct ufshpb_lu *hpb; struct scsi_device *sdev; shost_for_each_device(sdev, hba->host) { hpb = ufshpb_get_hpb_data(sdev); - if (!hpb) - continue; - if (ufshpb_get_state(hpb) != HPB_RESET) + if (!hpb || ufshpb_get_state(hpb) != state) continue; - ufshpb_set_state(hpb, HPB_PRESENT); - } -} - -void ufshpb_reset_host(struct ufs_hba *hba) -{ - struct ufshpb_lu *hpb; - struct scsi_device *sdev; - - shost_for_each_device(sdev, hba->host) { - hpb = ufshpb_get_hpb_data(sdev); - if (!hpb) - continue; - - if (ufshpb_get_state(hpb) != HPB_PRESENT) - continue; - ufshpb_set_state(hpb, HPB_RESET); - ufshpb_cancel_jobs(hpb); - ufshpb_discard_rsp_lists(hpb); + ufshpb_set_state(hpb, state); + if (state == HPB_RESET) { + ufshpb_cancel_jobs(hpb); + ufshpb_discard_rsp_lists(hpb); + } } + /* + * Inactivating all HPB Region in device side in case HPB state changed + * from HPB_RESET to HPB_PRESENT + */ + if (!hpb->is_hcm && state == HPB_PRESENT) + ufshpb_issue_umap_all_req(hpb); } void ufshpb_suspend(struct ufs_hba *hba) @@ -2456,8 +2446,6 @@ static void ufshpb_hpb_lu_prepared(struct ufs_hba *hba) ufshpb_set_state(hpb, HPB_PRESENT); if ((hpb->lu_pinned_end - hpb- >lu_pinned_start) > 0) queue_work(ufshpb_wq, &hpb->map_work); - if (!hpb->is_hcm) - ufshpb_issue_umap_all_req(hpb); } else { dev_err(hba->dev, "destroy HPB lu %d\n", hpb- >lun); ufshpb_destroy_lu(hba, sdev); diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h index b475dbd78988..a130f0b16c3e 100644 --- a/drivers/scsi/ufs/ufshpb.h +++ b/drivers/scsi/ufs/ufshpb.h @@ -288,8 +288,7 @@ static int ufshpb_prep(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) { return 0; static void ufshpb_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) {} static void ufshpb_resume(struct ufs_hba *hba) {} static void ufshpb_suspend(struct ufs_hba *hba) {} -static void ufshpb_reset(struct ufs_hba *hba) {} -static void ufshpb_reset_host(struct ufs_hba *hba) {} +static void ufshpb_state_toggle(struct ufs_hba *hba, enum UFSHPB_STATE state) {} static void ufshpb_init(struct ufs_hba *hba) {} static void ufshpb_init_hpb_lu(struct ufs_hba *hba, struct scsi_device *sdev) {} static void ufshpb_destroy_lu(struct ufs_hba *hba, struct scsi_device *sdev) {} @@ -303,8 +302,7 @@ int ufshpb_prep(struct ufs_hba *hba, struct ufshcd_lrb *lrbp); void ufshpb_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp); void ufshpb_resume(struct ufs_hba *hba); void ufshpb_suspend(struct ufs_hba *hba); -void ufshpb_reset(struct ufs_hba *hba); -void ufshpb_reset_host(struct ufs_hba *hba); +void ufshpb_state_toggle(struct ufs_hba *hba, enum UFSHPB_STATE state); void ufshpb_init(struct ufs_hba *hba); void ufshpb_init_hpb_lu(struct ufs_hba *hba, struct scsi_device *sdev); void ufshpb_destroy_lu(struct ufs_hba *hba, struct scsi_device *sdev);
Hi Bean, How about if reset flow not clear hpb mapping? After device is reset by RST_N pin, if the hpb mapping table still exist? BTW, Host seem not clear region mapping table in reset flow. Thanks. On 4/12/22 11:20 PM, Bean Huo wrote: > Hi, Po-Wen, > Previous email did not properly append the patch, this one is better > for your review and test. > > > > > From: Bean Huo <beanhuo@micron.com> > Date: Tue, 12 Apr 2022 16:56:34 +0200 > Subject: [PATCH] scsi: ufshpb: UFSHPB cleanup > > Remove redundant ufshpb_reset*, and merge into a single helper > ufshbp_state_toggle(). > > Delete the redundant Inactivation code of all HPB Regions in the cold > boot stage, > and add inactivating all HPB Regions when the HPP status changes from > HPB_RESET > to HBP_PRESENT. > > Signed-off-by: Bean Huo <beanhuo@micron.com> > --- > drivers/scsi/ufs/ufshcd.c | 4 ++-- > drivers/scsi/ufs/ufshpb.c | 38 +++++++++++++------------------------- > drivers/scsi/ufs/ufshpb.h | 6 ++---- > 3 files changed, 17 insertions(+), 31 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index 0899d5b8cdad..d8b59d017ce4 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -7223,7 +7223,7 @@ static int ufshcd_host_reset_and_restore(struct > ufs_hba *hba) > * Stop the host controller and complete the requests > * cleared by h/w > */ > - ufshpb_reset_host(hba); > + ufshpb_state_toggle(hba, HPB_RESET); > ufshcd_hba_stop(hba); > hba->silence_err_logs = true; > ufshcd_complete_requests(hba); > @@ -8184,7 +8184,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, > bool init_dev_params) > /* Enable Auto-Hibernate if configured */ > ufshcd_auto_hibern8_enable(hba); > > - ufshpb_reset(hba); > + ufshpb_state_toggle(hba, HPB_PRESENT); > out: > spin_lock_irqsave(hba->host->host_lock, flags); > if (ret) > diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c > index 3ca745ad616c..4ed156031413 100644 > --- a/drivers/scsi/ufs/ufshpb.c > +++ b/drivers/scsi/ufs/ufshpb.c > @@ -2278,39 +2278,29 @@ static bool ufshpb_check_hpb_reset_query(struct > ufs_hba *hba) > return flag_res; > } > > -void ufshpb_reset(struct ufs_hba *hba) > +void ufshpb_state_toggle(struct ufs_hba *hba, enum UFSHPB_STATE state) > { > struct ufshpb_lu *hpb; > struct scsi_device *sdev; > > shost_for_each_device(sdev, hba->host) { > hpb = ufshpb_get_hpb_data(sdev); > - if (!hpb) > - continue; > > - if (ufshpb_get_state(hpb) != HPB_RESET) > + if (!hpb || ufshpb_get_state(hpb) != state) > continue; > > - ufshpb_set_state(hpb, HPB_PRESENT); > - } > -} > - > -void ufshpb_reset_host(struct ufs_hba *hba) > -{ > - struct ufshpb_lu *hpb; > - struct scsi_device *sdev; > - > - shost_for_each_device(sdev, hba->host) { > - hpb = ufshpb_get_hpb_data(sdev); > - if (!hpb) > - continue; > - > - if (ufshpb_get_state(hpb) != HPB_PRESENT) > - continue; > - ufshpb_set_state(hpb, HPB_RESET); > - ufshpb_cancel_jobs(hpb); > - ufshpb_discard_rsp_lists(hpb); > + ufshpb_set_state(hpb, state); > + if (state == HPB_RESET) { > + ufshpb_cancel_jobs(hpb); > + ufshpb_discard_rsp_lists(hpb); > + } > } > + /* > + * Inactivating all HPB Region in device side in case HPB > state changed > + * from HPB_RESET to HPB_PRESENT > + */ > + if (!hpb->is_hcm && state == HPB_PRESENT) may need check hpb is not null? > + ufshpb_issue_umap_all_req(hpb); > } > > void ufshpb_suspend(struct ufs_hba *hba) > @@ -2456,8 +2446,6 @@ static void ufshpb_hpb_lu_prepared(struct ufs_hba > *hba) > ufshpb_set_state(hpb, HPB_PRESENT); > if ((hpb->lu_pinned_end - hpb- >> lu_pinned_start) > 0) > queue_work(ufshpb_wq, &hpb->map_work); > - if (!hpb->is_hcm) > - ufshpb_issue_umap_all_req(hpb); > } else { > dev_err(hba->dev, "destroy HPB lu %d\n", hpb- >> lun); > ufshpb_destroy_lu(hba, sdev); > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h > index b475dbd78988..a130f0b16c3e 100644 > --- a/drivers/scsi/ufs/ufshpb.h > +++ b/drivers/scsi/ufs/ufshpb.h > @@ -288,8 +288,7 @@ static int ufshpb_prep(struct ufs_hba *hba, struct > ufshcd_lrb *lrbp) { return 0; > static void ufshpb_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb > *lrbp) {} > static void ufshpb_resume(struct ufs_hba *hba) {} > static void ufshpb_suspend(struct ufs_hba *hba) {} > -static void ufshpb_reset(struct ufs_hba *hba) {} > -static void ufshpb_reset_host(struct ufs_hba *hba) {} > +static void ufshpb_state_toggle(struct ufs_hba *hba, enum UFSHPB_STATE > state) {} > static void ufshpb_init(struct ufs_hba *hba) {} > static void ufshpb_init_hpb_lu(struct ufs_hba *hba, struct scsi_device > *sdev) {} > static void ufshpb_destroy_lu(struct ufs_hba *hba, struct scsi_device > *sdev) {} > @@ -303,8 +302,7 @@ int ufshpb_prep(struct ufs_hba *hba, struct > ufshcd_lrb *lrbp); > void ufshpb_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp); > void ufshpb_resume(struct ufs_hba *hba); > void ufshpb_suspend(struct ufs_hba *hba); > -void ufshpb_reset(struct ufs_hba *hba); > -void ufshpb_reset_host(struct ufs_hba *hba); > +void ufshpb_state_toggle(struct ufs_hba *hba, enum UFSHPB_STATE > state); > void ufshpb_init(struct ufs_hba *hba); > void ufshpb_init_hpb_lu(struct ufs_hba *hba, struct scsi_device > *sdev); > void ufshpb_destroy_lu(struct ufs_hba *hba, struct scsi_device *sdev);
Hi Peter, On Wed, 2022-04-13 at 11:53 +0800, Peter Wang wrote: > Hi Bean, > > How about if reset flow not clear hpb mapping? UFS will go through normal read, because regions are not active in the device. The responses mabye different among different vendors. for example, UFS device will raise a response to host to update the region (in case of host mode) or inactivate it in case of device mode. The best way is that inactivate the active regions in the host side in the case the regions are not active in the device. > > After device is reset by RST_N pin, if the hpb mapping table still > exist? no, the regions in the device will be inactivated. > > BTW, Host seem not clear region mapping table in reset flow. > Yes, you are right. There will be a performance impact. I will look at it, then back to you. By the way, there is one issue in my patch, ignore that patch. Kind regards, Bean
Hi, Po wen and Peter
I must give this patch Ack, because:
1. “fHPBReset Flag” can be sent in both host control mode and device
control mode. “buffer id=3 HpbWriteBuffer” can only be sent in device
control mode.
2. “fHPBReset Flag” inactivate all the regions for the whole device.
“buffer id=3 HpbWriteBuffer” can only inactivate all the regions for
this LUN. The LUN is written in the UPIU header.
In the ufshpb_init, we have set fHPBReset Flag. here doesn't need to
issue “buffer id=3 HpbWriteBuffer” in the case of device mode.
Regarding the reset flow issue we are talking about, I will submit a
fix.
Acked-by: Bean Huo <beanhuo@micron.com>
Bean
Hi Bean, The general idea looks good. As ufshpb_issue_umap_all_req in ufshpb_state_toggle won't actually runs at cold boot probe due to the fact that sdev are not present yet, this should avoid sending HPB command before UAC being cleared while keeps HPB mapping clean when rest flow occurs. Just a concern, shouldn't ufshpb_issue_umap_all_req be send for each sdev's corresponding hpb? Looking forward to your further fixes. Po-Wen On 4/12/22 11:20 PM, Bean Huo wrote: > Hi, Po-Wen, > Previous email did not properly append the patch, this one is better > for your review and test. > > > > > From: Bean Huo <beanhuo@micron.com> > Date: Tue, 12 Apr 2022 16:56:34 +0200 > Subject: [PATCH] scsi: ufshpb: UFSHPB cleanup > > Remove redundant ufshpb_reset*, and merge into a single helper > ufshbp_state_toggle(). > > Delete the redundant Inactivation code of all HPB Regions in the cold > boot stage, > and add inactivating all HPB Regions when the HPP status changes from > HPB_RESET > to HBP_PRESENT. > > Signed-off-by: Bean Huo <beanhuo@micron.com> > --- > drivers/scsi/ufs/ufshcd.c | 4 ++-- > drivers/scsi/ufs/ufshpb.c | 38 +++++++++++++------------------------- > drivers/scsi/ufs/ufshpb.h | 6 ++---- > 3 files changed, 17 insertions(+), 31 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index 0899d5b8cdad..d8b59d017ce4 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -7223,7 +7223,7 @@ static int ufshcd_host_reset_and_restore(struct > ufs_hba *hba) > * Stop the host controller and complete the requests > * cleared by h/w > */ > - ufshpb_reset_host(hba); > + ufshpb_state_toggle(hba, HPB_RESET); > ufshcd_hba_stop(hba); > hba->silence_err_logs = true; > ufshcd_complete_requests(hba); > @@ -8184,7 +8184,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, > bool init_dev_params) > /* Enable Auto-Hibernate if configured */ > ufshcd_auto_hibern8_enable(hba); > > - ufshpb_reset(hba); > + ufshpb_state_toggle(hba, HPB_PRESENT); > out: > spin_lock_irqsave(hba->host->host_lock, flags); > if (ret) > diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c > index 3ca745ad616c..4ed156031413 100644 > --- a/drivers/scsi/ufs/ufshpb.c > +++ b/drivers/scsi/ufs/ufshpb.c > @@ -2278,39 +2278,29 @@ static bool ufshpb_check_hpb_reset_query(struct > ufs_hba *hba) > return flag_res; > } > > -void ufshpb_reset(struct ufs_hba *hba) > +void ufshpb_state_toggle(struct ufs_hba *hba, enum UFSHPB_STATE state) > { > struct ufshpb_lu *hpb; > struct scsi_device *sdev; > > shost_for_each_device(sdev, hba->host) { > hpb = ufshpb_get_hpb_data(sdev); > - if (!hpb) > - continue; > > - if (ufshpb_get_state(hpb) != HPB_RESET) > + if (!hpb || ufshpb_get_state(hpb) != state) > continue; > > - ufshpb_set_state(hpb, HPB_PRESENT); > - } > -} > - > -void ufshpb_reset_host(struct ufs_hba *hba) > -{ > - struct ufshpb_lu *hpb; > - struct scsi_device *sdev; > - > - shost_for_each_device(sdev, hba->host) { > - hpb = ufshpb_get_hpb_data(sdev); > - if (!hpb) > - continue; > - > - if (ufshpb_get_state(hpb) != HPB_PRESENT) > - continue; > - ufshpb_set_state(hpb, HPB_RESET); > - ufshpb_cancel_jobs(hpb); > - ufshpb_discard_rsp_lists(hpb); > + ufshpb_set_state(hpb, state); > + if (state == HPB_RESET) { > + ufshpb_cancel_jobs(hpb); > + ufshpb_discard_rsp_lists(hpb); > + } > } > + /* > + * Inactivating all HPB Region in device side in case HPB > state changed > + * from HPB_RESET to HPB_PRESENT > + */ > + if (!hpb->is_hcm && state == HPB_PRESENT) > + ufshpb_issue_umap_all_req(hpb); > } > > void ufshpb_suspend(struct ufs_hba *hba) > @@ -2456,8 +2446,6 @@ static void ufshpb_hpb_lu_prepared(struct ufs_hba > *hba) > ufshpb_set_state(hpb, HPB_PRESENT); > if ((hpb->lu_pinned_end - hpb- >> lu_pinned_start) > 0) > queue_work(ufshpb_wq, &hpb->map_work); > - if (!hpb->is_hcm) > - ufshpb_issue_umap_all_req(hpb); > } else { > dev_err(hba->dev, "destroy HPB lu %d\n", hpb- >> lun); > ufshpb_destroy_lu(hba, sdev); > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h > index b475dbd78988..a130f0b16c3e 100644 > --- a/drivers/scsi/ufs/ufshpb.h > +++ b/drivers/scsi/ufs/ufshpb.h > @@ -288,8 +288,7 @@ static int ufshpb_prep(struct ufs_hba *hba, struct > ufshcd_lrb *lrbp) { return 0; > static void ufshpb_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb > *lrbp) {} > static void ufshpb_resume(struct ufs_hba *hba) {} > static void ufshpb_suspend(struct ufs_hba *hba) {} > -static void ufshpb_reset(struct ufs_hba *hba) {} > -static void ufshpb_reset_host(struct ufs_hba *hba) {} > +static void ufshpb_state_toggle(struct ufs_hba *hba, enum UFSHPB_STATE > state) {} > static void ufshpb_init(struct ufs_hba *hba) {} > static void ufshpb_init_hpb_lu(struct ufs_hba *hba, struct scsi_device > *sdev) {} > static void ufshpb_destroy_lu(struct ufs_hba *hba, struct scsi_device > *sdev) {} > @@ -303,8 +302,7 @@ int ufshpb_prep(struct ufs_hba *hba, struct > ufshcd_lrb *lrbp); > void ufshpb_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp); > void ufshpb_resume(struct ufs_hba *hba); > void ufshpb_suspend(struct ufs_hba *hba); > -void ufshpb_reset(struct ufs_hba *hba); > -void ufshpb_reset_host(struct ufs_hba *hba); > +void ufshpb_state_toggle(struct ufs_hba *hba, enum UFSHPB_STATE > state); > void ufshpb_init(struct ufs_hba *hba); > void ufshpb_init_hpb_lu(struct ufs_hba *hba, struct scsi_device > *sdev); > void ufshpb_destroy_lu(struct ufs_hba *hba, struct scsi_device *sdev);
On Thu, 2022-04-14 at 12:49 +0800, Po-Wen Kao wrote: > Hi Bean, > > The general idea looks good. > > As ufshpb_issue_umap_all_req in ufshpb_state_toggle won't actually > runs > at cold boot probe due to the fact that sdev are not present yet, > this > should avoid sending HPB command before UAC being cleared while keeps > HPB mapping clean when rest flow occurs. > > Just a concern, shouldn't ufshpb_issue_umap_all_req be send for each > sdev's corresponding hpb? > Hi Po Wen, I replied in another email that setting the flag resets all active regions in the device, which is a more efficient way, I have submitted a fix for HPB device mode, your review and testing are welcome. Kind regards, Bean > Looking forward to your further fixes. > >
On Tue, 12 Apr 2022 15:31:28 +0800, Po-Wen Kao wrote: > Since the HPB mapping is already reset in ufshpb_init by setting > flag QUERY_FLAG_IDN_HPB_RESET, there is no need doing so again in > ufshpb_hpb_lu_prepared. > > This would also resolve the issue where HPB WRTIE BUFFER is issued > before UAC being cleared. > > [...] Applied to 5.19/scsi-queue, thanks! [1/1] scsi: ufs: remove redundant HPB unmap https://git.kernel.org/mkp/scsi/c/25a0bf213b8a
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c index a86d0cc50de2..5c09d44c4bd5 100644 --- a/drivers/scsi/ufs/ufshpb.c +++ b/drivers/scsi/ufs/ufshpb.c @@ -934,11 +934,6 @@ static int ufshpb_issue_umap_single_req(struct ufshpb_lu *hpb, return ufshpb_issue_umap_req(hpb, rgn, true); } -static int ufshpb_issue_umap_all_req(struct ufshpb_lu *hpb) -{ - return ufshpb_issue_umap_req(hpb, NULL, false); -} - static void __ufshpb_evict_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn) { @@ -2459,8 +2454,6 @@ static void ufshpb_hpb_lu_prepared(struct ufs_hba *hba) ufshpb_set_state(hpb, HPB_PRESENT); if ((hpb->lu_pinned_end - hpb->lu_pinned_start) > 0) queue_work(ufshpb_wq, &hpb->map_work); - if (!hpb->is_hcm) - ufshpb_issue_umap_all_req(hpb); } else { dev_err(hba->dev, "destroy HPB lu %d\n", hpb->lun); ufshpb_destroy_lu(hba, sdev);
Since the HPB mapping is already reset in ufshpb_init by setting flag QUERY_FLAG_IDN_HPB_RESET, there is no need doing so again in ufshpb_hpb_lu_prepared. This would also resolve the issue where HPB WRTIE BUFFER is issued before UAC being cleared. Signed-off-by: Po-Wen Kao <powen.kao@mediatek.com> --- drivers/scsi/ufs/ufshpb.c | 7 ------- 1 file changed, 7 deletions(-)