| 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: <f0ae504bccc428fa674a183608174bdd@codeaurora.org>
Date: Fri, 11 Jun 2021 11:01:12 +0800
From: Can Guo <cang@...eaurora.org>
To: Adrian Hunter <adrian.hunter@...el.com>
Cc: asutoshd@...eaurora.org, nguyenb@...eaurora.org,
hongwus@...eaurora.org, ziqichen@...eaurora.org,
linux-scsi@...r.kernel.org, kernel-team@...roid.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>,
Stanley Chu <stanley.chu@...iatek.com>,
Bean Huo <beanhuo@...ron.com>,
Jaegeuk Kim <jaegeuk@...nel.org>,
open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 5/9] scsi: ufs: Simplify error handling preparation
Hi Adrian,
On 2021-06-10 20:30, Adrian Hunter wrote:
> On 10/06/21 7:43 am, Can Guo wrote:
>> Commit cb7e6f05fce67c965194ac04467e1ba7bc70b069 ("scsi: ufs: core:
>> Enable
>> power management for wlun") moves UFS operations out of
>> ufshcd_resume(), so
>> in error handling preparation, if ufshcd hba has failed to resume,
>> there is
>> no point to re-enable IRQ/clk/pwr.
>
> I am not sure how cb7e6f05fce67c965194ac04467e1ba7bc70b069 made things
> any
> different,
Previously, without commit cb7e6f05fce67c965194ac04467e1ba7bc70b069,
ufshcd_resume()
may turn off pwr and clk due to UFS error, e.g., link transition failure
and SSU
error/abort (and these UFS error would invoke error handling). When
error handling
kicks start, it should re-enable the pwr and clk before proceeding. Now,
commit
cb7e6f05fce67c965194ac04467e1ba7bc70b069 makes ufshcd_resume() purely
control pwr and
clk, meaning if ufshcd_resume() fails, there is nothing we can do about
it - pwr or
clk enabling must have failed, and it is not because of UFS error. This
is why I am
removing the re-enabling pwr/clk in error handling prepare.
> but what I really wonder is why we don't just do recovery
> directly in __ufshcd_wl_suspend() and __ufshcd_wl_resume() and strip
> all
> the PM complexity out of ufshcd_err_handling()?
>
This is a good question and I've been strugled with this idea ever since
I
started to fix error handling.
Just so you know, there are runtime and system suspend/resume. And error
handling has the same nature of user access - it is unpredictable,
meaning it
can be invoked at any time (from IRQ handler), even when there is no
ongoing
cmd/data transactions (like auto hibern8 failure and UIC errors, such as
DME
error and some errors in data link layer) [1], unless you disable UFS
IRQ.
For runtime suspend/resume, it is fine, since we call
pm_runtime_get/put_sync() in
error handling - error handling won't run into parallel with runtime
suspend/resume.
For system suspend/resume, since error handling has the same nature like
user
access, so we are using host_sem to avoid concurrency of error handling
and
system suspend/resume.
Back to your question - can we just do recovery directly in
__ufshcd_wl_suspend()
and __ufshcd_wl_resume()? Yes, we can.
However, the reasons why I choose not to do it that way are (althrough
error
handler prepare has became much more simple after apply this change)
1. I want to keep all the complexity within error handler, and re-direct
all error
recovery needs to error handler. It can avoid calling
ufshcd_reset_and_restore()
and/or flush_work(&hba->eh_work) here and there. The entire UFS
suspend/resume is
already complex enough, I don't want to mess up with it.
2. We do explicit recovery only when we see certain errors, e.g., H8
enter func
returns an error during suspend, but as mentioned above [1], error
handling can
be invoked already from IRQ handler (due to all kinds of UIC errors
before H8 enter
func returns). So, we still need host_sem (in case of system
suspend/resume) to
avoid concurrency.
3. During system suspend/resume, error handling can be invoked (due to
non-fatal
errors) but still UFS cmds return no error at all. Similar like above,
we need
host_sem to avoid concurrency.
There are more reasons why I chose this way, but it is really this way
or others.
I am glad to see someone cares about error handling and can make it
better and
more robust, no matter what that way is. :)
Thanks,
Can Guo.
>>
>> Signed-off-by: Can Guo <cang@...eaurora.org>
>> ---
>> drivers/scsi/ufs/ufshcd.c | 58
>> +++++++++++++++++++++++++----------------------
>> 1 file changed, 31 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 7dc0fda..0afad6b 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -2727,8 +2727,8 @@ static int ufshcd_queuecommand(struct Scsi_Host
>> *host, struct scsi_cmnd *cmd)
>> break;
>> case UFSHCD_STATE_EH_SCHEDULED_FATAL:
>> /*
>> - * pm_runtime_get_sync() is used at error handling preparation
>> - * stage. If a scsi cmd, e.g. the SSU cmd, is sent from hba's
>> + * ufshcd_rpm_get_sync() is used at error handling preparation
>> + * stage. If a scsi cmd, e.g., the SSU cmd, is sent from the
>> * PM ops, it can never be finished if we let SCSI layer keep
>> * retrying it, which gets err handler stuck forever. Neither
>> * can we let the scsi cmd pass through, because UFS is in bad
>> @@ -5915,29 +5915,26 @@ static void ufshcd_clk_scaling_suspend(struct
>> ufs_hba *hba, bool suspend)
>> }
>> }
>>
>> -static void ufshcd_err_handling_prepare(struct ufs_hba *hba)
>> +static int ufshcd_err_handling_prepare(struct ufs_hba *hba)
>> {
>> + /*
>> + * Exclusively call pm_runtime_get_sync(hba->dev) once, in case
>> + * following ufshcd_rpm_get_sync() fails.
>> + */
>> + pm_runtime_get_sync(hba->dev);
>> + /* End of the world. */
>> + if (pm_runtime_suspended(hba->dev)) {
>> + pm_runtime_put(hba->dev);
>> + return -EINVAL;
>> + }
>> +
>> + ufshcd_set_eh_in_progress(hba);
>> ufshcd_rpm_get_sync(hba);
>> - if (pm_runtime_status_suspended(&hba->sdev_ufs_device->sdev_gendev)
>> ||
>> + if (pm_runtime_suspended(&hba->sdev_ufs_device->sdev_gendev) ||
>> hba->is_wl_sys_suspended) {
>> - enum ufs_pm_op pm_op;
>> + enum ufs_pm_op pm_op = hba->is_wl_sys_suspended ?
>> + UFS_SYSTEM_PM : UFS_RUNTIME_PM;
>>
>> - /*
>> - * Don't assume anything of resume, if
>> - * resume fails, irq and clocks can be OFF, and powers
>> - * can be OFF or in LPM.
>> - */
>> - ufshcd_setup_hba_vreg(hba, true);
>> - ufshcd_setup_vreg(hba, true);
>> - ufshcd_config_vreg_hpm(hba, hba->vreg_info.vccq);
>> - ufshcd_config_vreg_hpm(hba, hba->vreg_info.vccq2);
>> - ufshcd_hold(hba, false);
>> - if (!ufshcd_is_clkgating_allowed(hba)) {
>> - ufshcd_setup_clocks(hba, true);
>> - ufshcd_enable_irq(hba);
>> - }
>> - ufshcd_release(hba);
>> - pm_op = hba->is_wl_sys_suspended ? UFS_SYSTEM_PM : UFS_RUNTIME_PM;
>> ufshcd_vops_resume(hba, pm_op);
>> } else {
>> ufshcd_hold(hba, false);
>> @@ -5951,22 +5948,25 @@ static void ufshcd_err_handling_prepare(struct
>> ufs_hba *hba)
>> down_write(&hba->clk_scaling_lock);
>> up_write(&hba->clk_scaling_lock);
>> cancel_work_sync(&hba->eeh_work);
>> + return 0;
>> }
>>
>> static void ufshcd_err_handling_unprepare(struct ufs_hba *hba)
>> {
>> + ufshcd_clear_eh_in_progress(hba);
>> ufshcd_scsi_unblock_requests(hba);
>> ufshcd_release(hba);
>> if (ufshcd_is_clkscaling_supported(hba))
>> ufshcd_clk_scaling_suspend(hba, false);
>> ufshcd_clear_ua_wluns(hba);
>> ufshcd_rpm_put(hba);
>> + pm_runtime_put(hba->dev);
>> }
>>
>> static inline bool ufshcd_err_handling_should_stop(struct ufs_hba
>> *hba)
>> {
>> return (!hba->is_powered || hba->shutting_down ||
>> - !hba->sdev_ufs_device ||
>> + !hba->sdev_ufs_device || hba->is_sys_suspended ||
>> hba->ufshcd_state == UFSHCD_STATE_ERROR ||
>> (!(hba->saved_err || hba->saved_uic_err || hba->force_reset ||
>> ufshcd_is_link_broken(hba))));
>> @@ -6052,9 +6052,13 @@ static void ufshcd_err_handler(struct
>> work_struct *work)
>> up(&hba->host_sem);
>> return;
>> }
>> - ufshcd_set_eh_in_progress(hba);
>> spin_unlock_irqrestore(hba->host->host_lock, flags);
>> - ufshcd_err_handling_prepare(hba);
>> + if (ufshcd_err_handling_prepare(hba)) {
>> + dev_err(hba->dev, "%s: error handling preparation failed\n",
>> + __func__);
>> + up(&hba->host_sem);
>> + return;
>> + }
>> /* Complete requests that have door-bell cleared by h/w */
>> ufshcd_complete_requests(hba);
>> spin_lock_irqsave(hba->host->host_lock, flags);
>> @@ -6198,7 +6202,6 @@ static void ufshcd_err_handler(struct
>> work_struct *work)
>> dev_err_ratelimited(hba->dev, "%s: exit: saved_err 0x%x
>> saved_uic_err 0x%x",
>> __func__, hba->saved_err, hba->saved_uic_err);
>> }
>> - ufshcd_clear_eh_in_progress(hba);
>> spin_unlock_irqrestore(hba->host->host_lock, flags);
>> ufshcd_err_handling_unprepare(hba);
>> up(&hba->host_sem);
>> @@ -8999,6 +9002,9 @@ static int __ufshcd_wl_resume(struct ufs_hba
>> *hba, enum ufs_pm_op pm_op)
>>
>> /* Enable Auto-Hibernate if configured */
>> ufshcd_auto_hibern8_enable(hba);
>> +
>> + hba->clk_gating.is_suspended = false;
>> + ufshcd_release(hba);
>> goto out;
>>
>> set_old_link_state:
>> @@ -9008,8 +9014,6 @@ static int __ufshcd_wl_resume(struct ufs_hba
>> *hba, enum ufs_pm_op pm_op)
>> out:
>> if (ret)
>> ufshcd_update_evt_hist(hba, UFS_EVT_WL_RES_ERR, (u32)ret);
>> - hba->clk_gating.is_suspended = false;
>> - ufshcd_release(hba);
>> hba->wl_pm_op_in_progress = false;
>> return ret <= 0 ? ret : -EINVAL;
>> }
>>
Powered by blists - more mailing lists