During system resume/suspend, hba could be NULL. In this case, do not touch
eh_sem.
Fixes: 88a92d6ae4fe ("scsi: ufs: Serialize eh_work with system PM events and async scan")
Signed-off-by: Can Guo <[email protected]>
---
drivers/scsi/ufs/ufshcd.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index e221add..34e2541 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -8896,8 +8896,11 @@ int ufshcd_system_suspend(struct ufs_hba *hba)
int ret = 0;
ktime_t start = ktime_get();
+ if (!hba)
+ return 0;
+
down(&hba->eh_sem);
- if (!hba || !hba->is_powered)
+ if (!hba->is_powered)
return 0;
if ((ufs_get_pm_lvl_to_dev_pwr_mode(hba->spm_lvl) ==
@@ -8945,10 +8948,8 @@ int ufshcd_system_resume(struct ufs_hba *hba)
int ret = 0;
ktime_t start = ktime_get();
- if (!hba) {
- up(&hba->eh_sem);
+ if (!hba)
return -EINVAL;
- }
if (!hba->is_powered || pm_runtime_suspended(hba->dev))
/*
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
On 12/31/20 9:44 PM, Can Guo wrote:
> During system resume/suspend, hba could be NULL. In this case, do not touch
> eh_sem.
>
> Fixes: 88a92d6ae4fe ("scsi: ufs: Serialize eh_work with system PM events and async scan")
>
> Signed-off-by: Can Guo <[email protected]>
> ---
> drivers/scsi/ufs/ufshcd.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index e221add..34e2541 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -8896,8 +8896,11 @@ int ufshcd_system_suspend(struct ufs_hba *hba)
> int ret = 0;
> ktime_t start = ktime_get();
>
> + if (!hba)
> + return 0;
> +
> down(&hba->eh_sem);
> - if (!hba || !hba->is_powered)
> + if (!hba->is_powered)
> return 0;
>
> if ((ufs_get_pm_lvl_to_dev_pwr_mode(hba->spm_lvl) ==
> @@ -8945,10 +8948,8 @@ int ufshcd_system_resume(struct ufs_hba *hba)
> int ret = 0;
> ktime_t start = ktime_get();
>
> - if (!hba) {
> - up(&hba->eh_sem);
> + if (!hba)
> return -EINVAL;
> - }
>
> if (!hba->is_powered || pm_runtime_suspended(hba->dev))
> /*
Hi Can,
How can ufshcd_system_suspend() or ufshcd_system_resume() be called with a
NULL argument? In ufshcd_pci_probe() I see that pci_set_drvdata() is called
before pm_runtime_allow(). ufshcd_pci_remove() calls pm_runtime_forbid().
Thanks,
Bart.
On 2021-01-02 00:05, Bart Van Assche wrote:
> On 12/31/20 9:44 PM, Can Guo wrote:
>> During system resume/suspend, hba could be NULL. In this case, do not
>> touch
>> eh_sem.
>>
>> Fixes: 88a92d6ae4fe ("scsi: ufs: Serialize eh_work with system PM
>> events and async scan")
>>
>> Signed-off-by: Can Guo <[email protected]>
>> ---
>> drivers/scsi/ufs/ufshcd.c | 9 +++++----
>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index e221add..34e2541 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -8896,8 +8896,11 @@ int ufshcd_system_suspend(struct ufs_hba *hba)
>> int ret = 0;
>> ktime_t start = ktime_get();
>>
>> + if (!hba)
>> + return 0;
>> +
>> down(&hba->eh_sem);
>> - if (!hba || !hba->is_powered)
>> + if (!hba->is_powered)
>> return 0;
>>
>> if ((ufs_get_pm_lvl_to_dev_pwr_mode(hba->spm_lvl) ==
>> @@ -8945,10 +8948,8 @@ int ufshcd_system_resume(struct ufs_hba *hba)
>> int ret = 0;
>> ktime_t start = ktime_get();
>>
>> - if (!hba) {
>> - up(&hba->eh_sem);
>> + if (!hba)
>> return -EINVAL;
>> - }
>>
>> if (!hba->is_powered || pm_runtime_suspended(hba->dev))
>> /*
>
> Hi Can,
>
> How can ufshcd_system_suspend() or ufshcd_system_resume() be called
> with a
> NULL argument? In ufshcd_pci_probe() I see that pci_set_drvdata() is
> called
> before pm_runtime_allow(). ufshcd_pci_remove() calls
> pm_runtime_forbid().
>
> Thanks,
>
> Bart.
Hi Bart,
You are right about ufshcd_RUNTIME_suspend/resume() -
platform_set_drvdata()
is called before pm_runtime_enable(), so runtime suspend/resume cannot
happen
before pm_runtime_enable() is called. We can remove the sanity checks of
!hba there, they are outdated.
But for ufshcd_SYSTEM_suspend/resume() callbacks (not runtime ones), my
understanding is that system suspend/resume may happen after probe
(vendor
driver probe calls ufshcd_pltfrm_init()) starts but before
platform_set_drvdata()
is called, in this case hba is NULL.
int ufshcd_pltfrm_init(struct platform_device *pdev,
const struct ufs_hba_variant_ops *vops)
{
...
platform_set_drvdata(pdev, hba);
pm_runtime_set_active(&pdev->dev);
pm_runtime_enable(&pdev->dev);
}
Thanks,
Can Guo.
On 2021-01-02 20:29, Can Guo wrote:
> On 2021-01-02 00:05, Bart Van Assche wrote:
>> On 12/31/20 9:44 PM, Can Guo wrote:
>>> During system resume/suspend, hba could be NULL. In this case, do not
>>> touch
>>> eh_sem.
>>>
>>> Fixes: 88a92d6ae4fe ("scsi: ufs: Serialize eh_work with system PM
>>> events and async scan")
>>>
>>> Signed-off-by: Can Guo <[email protected]>
>>> ---
>>> drivers/scsi/ufs/ufshcd.c | 9 +++++----
>>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>> index e221add..34e2541 100644
>>> --- a/drivers/scsi/ufs/ufshcd.c
>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>> @@ -8896,8 +8896,11 @@ int ufshcd_system_suspend(struct ufs_hba *hba)
>>> int ret = 0;
>>> ktime_t start = ktime_get();
>>>
>>> + if (!hba)
>>> + return 0;
>>> +
>>> down(&hba->eh_sem);
>>> - if (!hba || !hba->is_powered)
>>> + if (!hba->is_powered)
>>> return 0;
>>>
>>> if ((ufs_get_pm_lvl_to_dev_pwr_mode(hba->spm_lvl) ==
>>> @@ -8945,10 +8948,8 @@ int ufshcd_system_resume(struct ufs_hba *hba)
>>> int ret = 0;
>>> ktime_t start = ktime_get();
>>>
>>> - if (!hba) {
>>> - up(&hba->eh_sem);
>>> + if (!hba)
>>> return -EINVAL;
>>> - }
>>>
>>> if (!hba->is_powered || pm_runtime_suspended(hba->dev))
>>> /*
>>
>> Hi Can,
>>
>> How can ufshcd_system_suspend() or ufshcd_system_resume() be called
>> with a
>> NULL argument? In ufshcd_pci_probe() I see that pci_set_drvdata() is
>> called
>> before pm_runtime_allow(). ufshcd_pci_remove() calls
>> pm_runtime_forbid().
>>
>> Thanks,
>>
>> Bart.
>
> Hi Bart,
>
> You are right about ufshcd_RUNTIME_suspend/resume() -
> platform_set_drvdata()
> is called before pm_runtime_enable(), so runtime suspend/resume cannot
> happen
> before pm_runtime_enable() is called. We can remove the sanity checks
> of
> !hba there, they are outdated.
Add more history here - before Stanley's change (see below),
platform_set_drvdata()
is called AFTER pm_runtime_enable(), which was why we needed sanity
checks of !hba.
But now the sanity checks are unnecessary in
ufshcd_RUNTIME_suspend/resume(), so
feel free to remove them.
But still, things are a bit different for
ufshcd_SYSTEM_suspend/resume(), we need
the sanity checks of !hba there if my understanding is correct.
commit 24e2e7a19f7e4b83d0d5189040d997bce3596473
Author: Stanley Chu <[email protected]>
Date: Wed Jun 12 23:19:05 2019 +0800
scsi: ufs: Avoid runtime suspend possibly being blocked forever
Thanks,
Can Guo.
>
> But for ufshcd_SYSTEM_suspend/resume() callbacks (not runtime ones), my
> understanding is that system suspend/resume may happen after probe
> (vendor
> driver probe calls ufshcd_pltfrm_init()) starts but before
> platform_set_drvdata()
> is called, in this case hba is NULL.
>
> int ufshcd_pltfrm_init(struct platform_device *pdev,
> const struct ufs_hba_variant_ops *vops)
> {
> ...
> platform_set_drvdata(pdev, hba);
>
> pm_runtime_set_active(&pdev->dev);
> pm_runtime_enable(&pdev->dev);
> }
>
> Thanks,
>
> Can Guo.
On 2/01/21 3:10 pm, Can Guo wrote:
> On 2021-01-02 20:29, Can Guo wrote:
>> On 2021-01-02 00:05, Bart Van Assche wrote:
>>> On 12/31/20 9:44 PM, Can Guo wrote:
>>>> During system resume/suspend, hba could be NULL. In this case, do not touch
>>>> eh_sem.
>>>>
>>>> Fixes: 88a92d6ae4fe ("scsi: ufs: Serialize eh_work with system PM events
>>>> and async scan")
>>>>
>>>> Signed-off-by: Can Guo <[email protected]>
>>>> ---
>>>> drivers/scsi/ufs/ufshcd.c | 9 +++++----
>>>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>>> index e221add..34e2541 100644
>>>> --- a/drivers/scsi/ufs/ufshcd.c
>>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>>> @@ -8896,8 +8896,11 @@ int ufshcd_system_suspend(struct ufs_hba *hba)
>>>> int ret = 0;
>>>> ktime_t start = ktime_get();
>>>>
>>>> + if (!hba)
>>>> + return 0;
>>>> +
>>>> down(&hba->eh_sem);
>>>> - if (!hba || !hba->is_powered)
>>>> + if (!hba->is_powered)
>>>> return 0;
>>>>
>>>> if ((ufs_get_pm_lvl_to_dev_pwr_mode(hba->spm_lvl) ==
>>>> @@ -8945,10 +8948,8 @@ int ufshcd_system_resume(struct ufs_hba *hba)
>>>> int ret = 0;
>>>> ktime_t start = ktime_get();
>>>>
>>>> - if (!hba) {
>>>> - up(&hba->eh_sem);
>>>> + if (!hba)
>>>> return -EINVAL;
>>>> - }
>>>>
>>>> if (!hba->is_powered || pm_runtime_suspended(hba->dev))
>>>> /*
>>>
>>> Hi Can,
>>>
>>> How can ufshcd_system_suspend() or ufshcd_system_resume() be called with a
>>> NULL argument? In ufshcd_pci_probe() I see that pci_set_drvdata() is called
>>> before pm_runtime_allow(). ufshcd_pci_remove() calls pm_runtime_forbid().
>>>
>>> Thanks,
>>>
>>> Bart.
>>
>> Hi Bart,
>>
>> You are right about ufshcd_RUNTIME_suspend/resume() - platform_set_drvdata()
>> is called before pm_runtime_enable(), so runtime suspend/resume cannot happen
>> before pm_runtime_enable() is called. We can remove the sanity checks of
>> !hba there, they are outdated.
>
> Add more history here - before Stanley's change (see below),
> platform_set_drvdata()
> is called AFTER pm_runtime_enable(), which was why we needed sanity checks
> of !hba.
> But now the sanity checks are unnecessary in
> ufshcd_RUNTIME_suspend/resume(), so
> feel free to remove them.
>
> But still, things are a bit different for ufshcd_SYSTEM_suspend/resume(), we
> need
> the sanity checks of !hba there if my understanding is correct.
>
> commit 24e2e7a19f7e4b83d0d5189040d997bce3596473
> Author: Stanley Chu <[email protected]>
> Date: Wed Jun 12 23:19:05 2019 +0800
>
> scsi: ufs: Avoid runtime suspend possibly being blocked forever
>
> Thanks,
> Can Guo.
>
>>
>> But for ufshcd_SYSTEM_suspend/resume() callbacks (not runtime ones), my
>> understanding is that system suspend/resume may happen after probe (vendor
>> driver probe calls ufshcd_pltfrm_init()) starts but before
>> platform_set_drvdata()
>> is called, in this case hba is NULL.
>>
>> int ufshcd_pltfrm_init(struct platform_device *pdev,
>> const struct ufs_hba_variant_ops *vops)
>> {
>> ...
>> platform_set_drvdata(pdev, hba);
>>
>> pm_runtime_set_active(&pdev->dev);
>> pm_runtime_enable(&pdev->dev);
>> }
Hi Can
I expect probe and system suspend are synchronized e.g. by device_lock(), so
hba would not be NULL. Is there any example of it being NULL in system suspend?
Regards
Adrian
On 2021-01-15 21:07, Adrian Hunter wrote:
> On 2/01/21 3:10 pm, Can Guo wrote:
>> On 2021-01-02 20:29, Can Guo wrote:
>>> On 2021-01-02 00:05, Bart Van Assche wrote:
>>>> On 12/31/20 9:44 PM, Can Guo wrote:
>>>>> During system resume/suspend, hba could be NULL. In this case, do
>>>>> not touch
>>>>> eh_sem.
>>>>>
>>>>> Fixes: 88a92d6ae4fe ("scsi: ufs: Serialize eh_work with system PM
>>>>> events
>>>>> and async scan")
>>>>>
>>>>> Signed-off-by: Can Guo <[email protected]>
>>>>> ---
>>>>> drivers/scsi/ufs/ufshcd.c | 9 +++++----
>>>>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>>>> index e221add..34e2541 100644
>>>>> --- a/drivers/scsi/ufs/ufshcd.c
>>>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>>>> @@ -8896,8 +8896,11 @@ int ufshcd_system_suspend(struct ufs_hba
>>>>> *hba)
>>>>> int ret = 0;
>>>>> ktime_t start = ktime_get();
>>>>>
>>>>> + if (!hba)
>>>>> + return 0;
>>>>> +
>>>>> down(&hba->eh_sem);
>>>>> - if (!hba || !hba->is_powered)
>>>>> + if (!hba->is_powered)
>>>>> return 0;
>>>>>
>>>>> if ((ufs_get_pm_lvl_to_dev_pwr_mode(hba->spm_lvl) ==
>>>>> @@ -8945,10 +8948,8 @@ int ufshcd_system_resume(struct ufs_hba
>>>>> *hba)
>>>>> int ret = 0;
>>>>> ktime_t start = ktime_get();
>>>>>
>>>>> - if (!hba) {
>>>>> - up(&hba->eh_sem);
>>>>> + if (!hba)
>>>>> return -EINVAL;
>>>>> - }
>>>>>
>>>>> if (!hba->is_powered || pm_runtime_suspended(hba->dev))
>>>>> /*
>>>>
>>>> Hi Can,
>>>>
>>>> How can ufshcd_system_suspend() or ufshcd_system_resume() be called
>>>> with a
>>>> NULL argument? In ufshcd_pci_probe() I see that pci_set_drvdata() is
>>>> called
>>>> before pm_runtime_allow(). ufshcd_pci_remove() calls
>>>> pm_runtime_forbid().
>>>>
>>>> Thanks,
>>>>
>>>> Bart.
>>>
>>> Hi Bart,
>>>
>>> You are right about ufshcd_RUNTIME_suspend/resume() -
>>> platform_set_drvdata()
>>> is called before pm_runtime_enable(), so runtime suspend/resume
>>> cannot happen
>>> before pm_runtime_enable() is called. We can remove the sanity checks
>>> of
>>> !hba there, they are outdated.
>>
>> Add more history here - before Stanley's change (see below),
>> platform_set_drvdata()
>> is called AFTER pm_runtime_enable(), which was why we needed sanity
>> checks
>> of !hba.
>> But now the sanity checks are unnecessary in
>> ufshcd_RUNTIME_suspend/resume(), so
>> feel free to remove them.
>>
>> But still, things are a bit different for
>> ufshcd_SYSTEM_suspend/resume(), we
>> need
>> the sanity checks of !hba there if my understanding is correct.
>>
>> commit 24e2e7a19f7e4b83d0d5189040d997bce3596473
>> Author: Stanley Chu <[email protected]>
>> Date: Wed Jun 12 23:19:05 2019 +0800
>>
>> scsi: ufs: Avoid runtime suspend possibly being blocked forever
>>
>> Thanks,
>> Can Guo.
>>
>>>
>>> But for ufshcd_SYSTEM_suspend/resume() callbacks (not runtime ones),
>>> my
>>> understanding is that system suspend/resume may happen after probe
>>> (vendor
>>> driver probe calls ufshcd_pltfrm_init()) starts but before
>>> platform_set_drvdata()
>>> is called, in this case hba is NULL.
>>>
>>> int ufshcd_pltfrm_init(struct platform_device *pdev,
>>> const struct ufs_hba_variant_ops *vops)
>>> {
>>> ...
>>> platform_set_drvdata(pdev, hba);
>>>
>>> pm_runtime_set_active(&pdev->dev);
>>> pm_runtime_enable(&pdev->dev);
>>> }
>
> Hi Can
>
> I expect probe and system suspend are synchronized e.g. by
> device_lock(), so
> hba would not be NULL. Is there any example of it being NULL in system
> suspend?
>
> Regards
> Adrian
Hi Adrian,
Thanks for the remind - I didn't notice they are protected by
device_lock().
You are right, hba cannot be NULL in current code... Maybe if (!hba) was
there just for a sanity check. I will make a change to remove these
checks.
Thanks,
Can Guo.