Re: [PATCH v4 06/10] scsi: ufs: Remove host_sem used in suspend/resume

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

 



On 2021-06-24 18:04, Adrian Hunter wrote:
On 24/06/21 9:31 am, Can Guo wrote:
On 2021-06-24 14:23, Adrian Hunter wrote:
On 24/06/21 9:12 am, Can Guo wrote:
On 2021-06-24 13:52, Adrian Hunter wrote:
On 24/06/21 5:16 am, Can Guo wrote:
On 2021-06-23 22:30, Adrian Hunter wrote:
On 23/06/21 10:35 am, Can Guo wrote:
To protect system suspend/resume from being disturbed by error handling, instead of using host_sem, let error handler call lock_system_sleep() and unlock_system_sleep() which achieve the same purpose. Remove the host_sem
used in suspend/resume paths to make the code more readable.

Suggested-by: Bart Van Assche <bvanassche@xxxxxxx>
Signed-off-by: Can Guo <cang@xxxxxxxxxxxxxx>
---
 drivers/scsi/ufs/ufshcd.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 3695dd2..a09e4a2 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5907,6 +5907,11 @@ static void ufshcd_clk_scaling_suspend(struct ufs_hba *hba, bool suspend)

 static void ufshcd_err_handling_prepare(struct ufs_hba *hba)
 {
+    /*
+     * It is not safe to perform error handling while suspend or resume is
+     * in progress. Hence the lock_system_sleep() call.
+     */
+    lock_system_sleep();

It looks to me like the system takes this lock quite early, even before freezing tasks, so if anything needs the error handler to run it will
deadlock.

Hi Adrian,

UFS/hba system suspend/resume does not invoke or call error handling in a synchronous way. So, whatever UFS errors (which schedules the error handler) happens during suspend/resume, error handler will just wait here till system suspend/resume release the lock. Hence no worries of deadlock here.

It looks to me like the state can change to UFSHCD_STATE_EH_SCHEDULED_FATAL and since user processes are not frozen, nor file systems sync'ed, everything
is going to deadlock.
i.e.
I/O is blocked waiting on error handling
error handling is blocked waiting on lock_system_sleep()
suspend is blocked waiting on I/O


Hi Adrian,

First of all, enter_state(suspend_state_t state) uses mutex_trylock(&system_transition_mutex).

Yes, in the case I am outlining it gets the mutex.

Second, even that happens, in ufshcd_queuecommand(), below logic will break the cycle, by fast failing the PM request (below codes are from the code tip with this whole series applied).

It won't get that far because the suspend will be waiting to sync filesystems.
Filesystems will be waiting on I/O.
I/O will be waiting on the error handler.
The error handler will be waiting on system_transition_mutex.
But system_transition_mutex is already held by PM core.

Hi Adrian,

You are right.... I missed the action of syncing filesystems...

Using back host_sem in suspend_prepare()/resume_complete() won't have this
problem of deadlock, right?

I am not sure, but what was problem that the V3 patch was fixing?
Can you give an example?

V3 was moving host_sem from wl_system_suspend/resume() to
ufshcd_suspend_prepare()/ufshcd_resume_complete(). It is to
make sure error handling does not run concurrenly with system
PM, since error handling is recovering/clearing runtime PM
errors of all the scsi devices under hba (in patch #8). Having the
error handling doing so (in patch 8) is because runtime PM framework
may save the runtime errors of the supplier to one or more consumers (
unlike the children - parent relationship), for example if wlu resume
fails, sda and/or other scsi devices may save the resume error, then
they will be left runtime suspended permanently.

Thanks,

Can Guo.



Thanks,

Can Guo.



        case UFSHCD_STATE_EH_SCHEDULED_FATAL:
                /*
                 * 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                  * state, the scsi cmd may eventually time out, which will get                  * err handler blocked for too long. So, just fail the scsi cmd                  * sent from PM ops, err handler can recover PM error anyways.
                 */
                if (cmd->request->rq_flags & RQF_PM) {
                        hba->force_reset = true;
                        set_host_byte(cmd, DID_BAD_TARGET);
                        cmd->scsi_done(cmd);
                        goto out;
                }
                fallthrough;
        case UFSHCD_STATE_RESET:

Thanks,

Can Guo.


Thanks,

Can Guo.


     ufshcd_rpm_get_sync(hba);
     if (pm_runtime_status_suspended(&hba->sdev_ufs_device->sdev_gendev) ||
         hba->is_wlu_sys_suspended) {
@@ -5951,6 +5956,7 @@ static void ufshcd_err_handling_unprepare(struct ufs_hba *hba)
         ufshcd_clk_scaling_suspend(hba, false);
     ufshcd_clear_ua_wluns(hba);
     ufshcd_rpm_put(hba);
+    unlock_system_sleep();
 }

 static inline bool ufshcd_err_handling_should_stop(struct ufs_hba *hba) @@ -9053,16 +9059,13 @@ static int ufshcd_wl_suspend(struct device *dev)
     ktime_t start = ktime_get();

     hba = shost_priv(sdev->host);
-    down(&hba->host_sem);

     if (pm_runtime_suspended(dev))
         goto out;

     ret = __ufshcd_wl_suspend(hba, UFS_SYSTEM_PM);
-    if (ret) {
+    if (ret)
         dev_err(&sdev->sdev_gendev, "%s failed: %d\n", __func__,  ret);
-        up(&hba->host_sem);
-    }

 out:
     if (!ret)
@@ -9095,7 +9098,6 @@ static int ufshcd_wl_resume(struct device *dev)
         hba->curr_dev_pwr_mode, hba->uic_link_state);
     if (!ret)
         hba->is_wlu_sys_suspended = false;
-    up(&hba->host_sem);
     return ret;
 }
 #endif




[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