| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <004b55d8-fcd3-136c-526c-6ada025d6a1c@mediatek.com>
Date: Thu, 14 Apr 2022 12:49:42 +0800
From: Po-Wen Kao <powen.kao@...iatek.com>
To: Bean Huo <huobean@...il.com>,
Alim Akhtar <alim.akhtar@...sung.com>,
"Avri Altman" <avri.altman@....com>,
"James E.J. Bottomley" <jejb@...ux.ibm.com>,
"Martin K. Petersen" <martin.petersen@...cle.com>,
Matthias Brugger <matthias.bgg@...il.com>
CC: <wsd_upstream@...iatek.com>, <peter.wang@...iatek.com>,
<stanley.chu@...iatek.com>, <alice.chao@...iatek.com>,
<chun-hung.wu@...iatek.com>, <cc.chou@...iatek.com>,
<chaotian.jing@...iatek.com>, <jiajie.hao@...iatek.com>,
<yohan.joung@...com>, <jason.li@...com>,
<linux-scsi@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-mediatek@...ts.infradead.org>
Subject: Re: [PATCH 1/1] scsi: ufs: remove redundant HPB unmap
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@...ron.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@...ron.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);
Powered by blists - more mailing lists