Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: cang=codeaurora.org@mg.codeaurora.org
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Wed, 21 Oct 2020 09:18:58 +0800
From:   Can Guo <cang@codeaurora.org>
To:     Bean Huo <beanhuo.cn@gmail.com>
Cc:     asutoshd@codeaurora.org, nguyenb@codeaurora.org,
        hongwus@codeaurora.org, rnayak@codeaurora.org,
        linux-scsi@vger.kernel.org, kernel-team@android.com,
        saravanak@google.com, salyzyn@google.com,
        Alim Akhtar <alim.akhtar@samsung.com>,
        Avri Altman <avri.altman@wdc.com>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        Stanley Chu <stanley.chu@mediatek.com>,
        Bean Huo <beanhuo@micron.com>,
        Bart Van Assche <bvanassche@acm.org>,
        Satya Tangirala <satyat@google.com>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v1 1/2] scsi: ufs: Serialize eh_work with system PM events
 and async scan
In-Reply-To: <e2976fe3f85c2625e42dbddbf61faed651caa5c0.camel@gmail.com>
References: <1600752747-31881-1-git-send-email-cang@codeaurora.org>
 <1600752747-31881-2-git-send-email-cang@codeaurora.org>
 <e2976fe3f85c2625e42dbddbf61faed651caa5c0.camel@gmail.com>
Message-ID: <877678c6b188fc6a85d4d9c6cb7b89c1@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Precedence: bulk

On 2020-10-20 22:19, Bean Huo wrote:
> On Mon, 2020-09-21 at 22:32 -0700, Can Guo wrote:
>> Serialize eh_work with system PM events and async scan to make sure
>> eh_work
>> does not run in parallel with them.
>> 
>> Change-Id: I33012c68e2ea443950313c59a4a46ad88cf3c82d
>> Signed-off-by: Can Guo <cang@codeaurora.org>
>> ---
>>  drivers/scsi/ufs/ufshcd.c | 64 +++++++++++++++++++++++++++++------
>> ------------
>>  drivers/scsi/ufs/ufshcd.h |  1 +
>>  2 files changed, 41 insertions(+), 24 deletions(-)
>> 
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 1d8134e..7e764e8 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -5597,7 +5597,9 @@ static inline void
>> ufshcd_schedule_eh_work(struct ufs_hba *hba)
>>  static void ufshcd_err_handling_prepare(struct ufs_hba *hba)
>>  {
>>  	pm_runtime_get_sync(hba->dev);
>> -	if (pm_runtime_suspended(hba->dev)) {
>> +	if (pm_runtime_status_suspended(hba->dev) || hba-
>> >is_sys_suspended) {
> 
> Hi Can
> 
> I curiously want to know how this can be produced in real system.
> 
> IMO, The system has been in system PM suspeded mode, how can trigger
> error handler? because the tasks have been freezed, the device
> interrupts disabled, before entering system PM suspended mode, the
> tasks in the queue should be completed, otherwises, there is bug in the
> whole flow.
> 
> 
> thanks,
> Bean

Hi Bean,

You might misunderstand here - the hba->is_sys_suspended check is
not for the case that system has entered suspend, but for the case a 
resume
failure happens. If system resume fails, hba->is_sys_suspended is set 
and
powers/clks/IRQs are OFF, so we need to prepare the environments for 
err_handler to run.
To reproduce this scenario, you can fake a hibern8 exit failure during 
system resume.

If the whole system has entered suspend, of course err_handler won't 
run.
I guess your real concern is that if UFS has entered system suspend (not 
the whole system),
but err_handling was somehow scheduled (most likely due to an non-fatal 
error).
This definitely is a problem and it is also why I make this change. With 
this change,
if UFS has entered system suspend, the eh_sem lock is held, err_handler 
won't even get
a chance to run, the err_handler will run only after UFS is resumed.

Thanks,

Can Guo.