Hi, Po-Wen,
Previous email did not properly append the patch, this one is better
for your review and test.
From: Bean Huo <[email protected]>
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 <[email protected]>
---
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);
--
2.34.1
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 <[email protected]>
> 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 <[email protected]>
> ---
> 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 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 <[email protected]>
> 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 <[email protected]>
> ---
> 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);