* [PATCH 1/1] scsi: ufs: remove redundant HPB unmap
@ 2022-04-12 7:31 Po-Wen Kao
2022-04-12 15:06 ` Bean Huo
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Po-Wen Kao @ 2022-04-12 7:31 UTC (permalink / raw)
To: Alim Akhtar, Avri Altman, James E.J. Bottomley,
Martin K. Petersen, Matthias Brugger
Cc: wsd_upstream, peter.wang, stanley.chu, powen.kao, alice.chao,
chun-hung.wu, cc.chou, chaotian.jing, jiajie.hao, huobean,
yohan.joung, jason.li, linux-scsi, linux-kernel,
linux-arm-kernel, linux-mediatek
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);
--
2.18.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] scsi: ufs: remove redundant HPB unmap
2022-04-12 7:31 [PATCH 1/1] scsi: ufs: remove redundant HPB unmap Po-Wen Kao
@ 2022-04-12 15:06 ` Bean Huo
2022-04-12 15:20 ` Bean Huo
2022-04-13 21:29 ` Bean Huo
2022-04-26 4:00 ` Martin K. Petersen
2 siblings, 1 reply; 9+ messages in thread
From: Bean Huo @ 2022-04-12 15:06 UTC (permalink / raw)
To: Po-Wen Kao, Alim Akhtar, Avri Altman, James E.J. Bottomley,
Martin K. Petersen, Matthias Brugger
Cc: wsd_upstream, peter.wang, stanley.chu, alice.chao, chun-hung.wu,
cc.chou, chaotian.jing, jiajie.hao, yohan.joung, jason.li,
linux-scsi, linux-kernel, linux-arm-kernel, linux-mediatek
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);
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] scsi: ufs: remove redundant HPB unmap
2022-04-12 15:06 ` Bean Huo
@ 2022-04-12 15:20 ` Bean Huo
2022-04-13 3:53 ` Peter Wang
2022-04-14 4:49 ` Po-Wen Kao
0 siblings, 2 replies; 9+ messages in thread
From: Bean Huo @ 2022-04-12 15:20 UTC (permalink / raw)
To: Po-Wen Kao, Alim Akhtar, Avri Altman, James E.J. Bottomley,
Martin K. Petersen, Matthias Brugger
Cc: wsd_upstream, peter.wang, stanley.chu, alice.chao, chun-hung.wu,
cc.chou, chaotian.jing, jiajie.hao, yohan.joung, jason.li,
linux-scsi, linux-kernel, linux-arm-kernel, linux-mediatek
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);
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] scsi: ufs: remove redundant HPB unmap
2022-04-12 15:20 ` Bean Huo
@ 2022-04-13 3:53 ` Peter Wang
2022-04-13 12:43 ` Bean Huo
2022-04-14 4:49 ` Po-Wen Kao
1 sibling, 1 reply; 9+ messages in thread
From: Peter Wang @ 2022-04-13 3:53 UTC (permalink / raw)
To: Bean Huo, Po-Wen Kao, Alim Akhtar, Avri Altman,
James E.J. Bottomley, Martin K. Petersen, Matthias Brugger
Cc: wsd_upstream, stanley.chu, alice.chao, chun-hung.wu, cc.chou,
chaotian.jing, jiajie.hao, yohan.joung, jason.li, linux-scsi,
linux-kernel, linux-arm-kernel, linux-mediatek
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);
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] scsi: ufs: remove redundant HPB unmap
2022-04-13 3:53 ` Peter Wang
@ 2022-04-13 12:43 ` Bean Huo
0 siblings, 0 replies; 9+ messages in thread
From: Bean Huo @ 2022-04-13 12:43 UTC (permalink / raw)
To: Peter Wang, Po-Wen Kao, Alim Akhtar, Avri Altman,
James E.J. Bottomley, Martin K. Petersen, Matthias Brugger
Cc: wsd_upstream, stanley.chu, alice.chao, chun-hung.wu, cc.chou,
chaotian.jing, jiajie.hao, yohan.joung, jason.li, linux-scsi,
linux-kernel, linux-arm-kernel, linux-mediatek
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] scsi: ufs: remove redundant HPB unmap
2022-04-12 7:31 [PATCH 1/1] scsi: ufs: remove redundant HPB unmap Po-Wen Kao
2022-04-12 15:06 ` Bean Huo
@ 2022-04-13 21:29 ` Bean Huo
2022-04-26 4:00 ` Martin K. Petersen
2 siblings, 0 replies; 9+ messages in thread
From: Bean Huo @ 2022-04-13 21:29 UTC (permalink / raw)
To: Po-Wen Kao, Alim Akhtar, Avri Altman, James E.J. Bottomley,
Martin K. Petersen, Matthias Brugger
Cc: wsd_upstream, peter.wang, stanley.chu, alice.chao, chun-hung.wu,
cc.chou, chaotian.jing, jiajie.hao, yohan.joung, jason.li,
linux-scsi, linux-kernel, linux-arm-kernel, linux-mediatek
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] scsi: ufs: remove redundant HPB unmap
2022-04-12 15:20 ` Bean Huo
2022-04-13 3:53 ` Peter Wang
@ 2022-04-14 4:49 ` Po-Wen Kao
2022-04-14 22:07 ` Bean Huo
1 sibling, 1 reply; 9+ messages in thread
From: Po-Wen Kao @ 2022-04-14 4:49 UTC (permalink / raw)
To: Bean Huo, Alim Akhtar, Avri Altman, James E.J. Bottomley,
Martin K. Petersen, Matthias Brugger
Cc: wsd_upstream, peter.wang, stanley.chu, alice.chao, chun-hung.wu,
cc.chou, chaotian.jing, jiajie.hao, yohan.joung, jason.li,
linux-scsi, linux-kernel, linux-arm-kernel, linux-mediatek
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);
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] scsi: ufs: remove redundant HPB unmap
2022-04-14 4:49 ` Po-Wen Kao
@ 2022-04-14 22:07 ` Bean Huo
0 siblings, 0 replies; 9+ messages in thread
From: Bean Huo @ 2022-04-14 22:07 UTC (permalink / raw)
To: Po-Wen Kao, Alim Akhtar, Avri Altman, James E.J. Bottomley,
Martin K. Petersen, Matthias Brugger
Cc: wsd_upstream, peter.wang, stanley.chu, alice.chao, chun-hung.wu,
cc.chou, chaotian.jing, jiajie.hao, yohan.joung, jason.li,
linux-scsi, linux-kernel, linux-arm-kernel, linux-mediatek
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.
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] scsi: ufs: remove redundant HPB unmap
2022-04-12 7:31 [PATCH 1/1] scsi: ufs: remove redundant HPB unmap Po-Wen Kao
2022-04-12 15:06 ` Bean Huo
2022-04-13 21:29 ` Bean Huo
@ 2022-04-26 4:00 ` Martin K. Petersen
2 siblings, 0 replies; 9+ messages in thread
From: Martin K. Petersen @ 2022-04-26 4:00 UTC (permalink / raw)
To: Po-Wen Kao, James E.J. Bottomley, Avri Altman, Matthias Brugger,
Alim Akhtar
Cc: Martin K . Petersen, chun-hung.wu, chaotian.jing, linux-scsi,
stanley.chu, linux-arm-kernel, cc.chou, wsd_upstream, peter.wang,
alice.chao, yohan.joung, jiajie.hao, linux-mediatek, huobean,
jason.li, linux-kernel
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
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-04-26 4:01 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-12 7:31 [PATCH 1/1] scsi: ufs: remove redundant HPB unmap Po-Wen Kao
2022-04-12 15:06 ` Bean Huo
2022-04-12 15:20 ` Bean Huo
2022-04-13 3:53 ` Peter Wang
2022-04-13 12:43 ` Bean Huo
2022-04-14 4:49 ` Po-Wen Kao
2022-04-14 22:07 ` Bean Huo
2022-04-13 21:29 ` Bean Huo
2022-04-26 4:00 ` Martin K. Petersen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).