Re: [PATCH v3 8/9] scsi: ufs: Update the fast abort path in ufshcd_abort() for PM requests

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2021-06-16 02:25, Bart Van Assche wrote:
On 6/14/21 7:36 PM, Can Guo wrote:
I've considered the similar way (leverage hba->host->eh_noresume) last
year,
but I didn't take this way due to below reasons:

1. UFS error handler basically does one thing - reset and restore, which stops hba [1], resets device [2] and re-probes the device [3]. Stopping
hba [1]
shall complete any pending requests in the doorbell (with error or no
error).
After [1], suspend/resume contexts, blocked by SSU cmd, shall be unblocked right away to do whatever it needs to handle the SSU cmd failure (completed in [1], so scsi_execute() returns an error), e.g., put link back to the old state. call ufshcd_vops_suspend(), turn off irq/clocks/powers and etc...
However, reset and restore ([2] and [3]) is still running, and it can
(most likely)
be disturbed by suspend/resume. So passing a parameter or using
hba->host->eh_noresume
to skip lock_system_sleep() and unlock_system_sleep() can break the cycle,
but error handling may run concurrently with suspend/resume. Of course
we can
modify suspend/resume to avoid it, but I was pursuing a minimal change
to get this fixed.

2. Whatever way we take to break the cycle, suspend/resume shall fail and
RPM framework shall save the error to dev.power.runtime_error, leaving
the device in runtime suspended or active mode permanently. If it is left runtime suspended, UFS driver won't accept cmd anymore, while if it is left
runtime active, powers of UFS device and host will be left ON, leading
to power
penalty. So my main idea is to let suspend/resume contexts, blocked by
PM cmds,
fail fast first and then error handler recover everything back to work.

Hi Can,

Has it been considered to make the UFS error handler fail pending
commands with an error code that causes the SCSI core to resubmit the
SCSI command, e.g. DID_IMM_RETRY or DID_TRANSPORT_DISRUPTED? I want to
prevent that power management or suspend/resume callbacks fail if the
error handler succeeds with recovering the UFS transport.


Hi Bart,

Thanks for the suggestion, I thought about it but I didn't go that
far in this path because I believe letting a context fast fail is
better than retrying/blocking it (to me suspend/resume can fail
due to many reasons and task abort is just one of them). I appreciate
the idea, but I would like to stick to my way as of now because

1. Merely preventing task abort cannot prevent suspend/resume fail.
Task abort (to PM requests), in real cases, is just one of many kinds
of failure which can fail the suspend/resume callbacks. During
suspend/resume, if AH8 error and/or UIC errors happen, IRQ handler
may complete SSU cmd with errors and schedule the error handler (I've
seen such scenarios in real customer cases). My idea is to treat task
abort (to PM requests) as a failure (let scsi_execute() return with
whatever error) and let error handler recover everything just like
any other UFS errors which invoke error handler. In case this, again,
goes back to the topic that is why don't just do error recovery in
suspend/resume, let me paste my previous reply here -

"
Error handler 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.

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)

- 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.

- 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.

- 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.
"

2. And say we want SCSI layer to resubmit PM requests to prevent
suspend/resume fail, we should keep retrying the PM requests (so
long as error handler can recover everything successfully), meaning
we should give them unlimited retries (which I think is a bad idea),
otherwise (if they have zero retries or limited retries), in extreme
conditions, what may happen is that error handler can recover everything
successfully every time, but all these retries (say 3) still time out,
which block the power management for too long (retries * 60 seconds) and,
most important, when the last retry times out, scsi layer will anyways
complete the PM request (even we return DID_IMM_RETRY), then we end up
same - suspend/resume shall run concurrently with error handler and we
couldn't recover saved PM errors.

Thanks,

Can Guo.

Thanks,

Bart.



[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux