diff mbox series

[v2,1/2] scsi: ufs: Fix a possible NULL pointer issue

Message ID 1609479893-8889-2-git-send-email-cang@codeaurora.org
State Superseded
Headers show
Series [v2,1/2] scsi: ufs: Fix a possible NULL pointer issue | expand

Commit Message

Can Guo Jan. 1, 2021, 5:44 a.m. UTC 
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 <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Bart Van Assche Jan. 1, 2021, 4:05 p.m. UTC | #1 
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 <cang@codeaurora.org>
> ---
>  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.
Can Guo Jan. 2, 2021, 12:29 p.m. UTC | #2 
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 <cang@codeaurora.org>

>> ---

>>  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.
Can Guo Jan. 2, 2021, 1:10 p.m. UTC | #3 
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 <cang@codeaurora.org>

>>> ---

>>>  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 <stanley.chu@mediatek.com>
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.
Adrian Hunter Jan. 15, 2021, 1:07 p.m. UTC | #4 
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 <cang@codeaurora.org>

>>>> ---

>>>>  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 <stanley.chu@mediatek.com>

> 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
Can Guo Jan. 16, 2021, 1:27 p.m. UTC | #5 
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 <cang@codeaurora.org>

>>>>> ---

>>>>>  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 <stanley.chu@mediatek.com>

>> 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.
diff mbox series

Patch

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))
 		/*