diff mbox series

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

Message ID 1609595975-12219-2-git-send-email-cang@codeaurora.org (mailing list archive)
State Superseded
Headers show
Series Synchronize user layer access with system PM ops and error handling | expand

Commit Message

Can Guo Jan. 2, 2021, 1:59 p.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>

Comments

Stanley Chu Jan. 12, 2021, 6:35 a.m. UTC | #1 
Hi Can,

On Sat, 2021-01-02 at 05:59 -0800, 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>
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index e221add..9829c8d 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -94,6 +94,8 @@
>  		       16, 4, buf, __len, false);                        \
>  } while (0)
>  
> +static bool early_suspend;
> +
>  int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len,
>  		     const char *prefix)
>  {
> @@ -8896,8 +8898,14 @@ int ufshcd_system_suspend(struct ufs_hba *hba)
>  	int ret = 0;
>  	ktime_t start = ktime_get();
>  
> +	if (!hba) {
> +		early_suspend = true;
> +		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,9 +8953,12 @@ 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 (unlikely(early_suspend)) {
> +		early_suspend = false;
> +		down(&hba->eh_sem);
>  	}

I guess early_suspend here is to handle the case that hba is null during
ufshcd_system_suspend() but !null during ufshcd_system_resume(). If yes,
would it be possible? If no, may I know what is the purpose?

Thanks a lot.
Stanley Chu

>  
>  	if (!hba->is_powered || pm_runtime_suspended(hba->dev))
Can Guo Jan. 12, 2021, 6:52 a.m. UTC | #2 
On 2021-01-12 14:35, Stanley Chu wrote:
> Hi Can,
> 
> On Sat, 2021-01-02 at 05:59 -0800, 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>
>> 
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index e221add..9829c8d 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -94,6 +94,8 @@
>>  		       16, 4, buf, __len, false);                        \
>>  } while (0)
>> 
>> +static bool early_suspend;
>> +
>>  int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len,
>>  		     const char *prefix)
>>  {
>> @@ -8896,8 +8898,14 @@ int ufshcd_system_suspend(struct ufs_hba *hba)
>>  	int ret = 0;
>>  	ktime_t start = ktime_get();
>> 
>> +	if (!hba) {
>> +		early_suspend = true;
>> +		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,9 +8953,12 @@ 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 (unlikely(early_suspend)) {
>> +		early_suspend = false;
>> +		down(&hba->eh_sem);
>>  	}
> 
> I guess early_suspend here is to handle the case that hba is null 
> during
> ufshcd_system_suspend() but !null during ufshcd_system_resume(). If 
> yes,
> would it be possible? If no, may I know what is the purpose?
> 

Yes, you are right. I think it is possible. platform_set_drvdata()
is called in ufshcd_pltfrm_init(). Say suspend happens before
platform_set_drvdata() is called, but resume comes back after
platform_set_drvdata() is called. What do you think?

Thanks,
Can Guo.

> Thanks a lot.
> Stanley Chu
> 
>> 
>>  	if (!hba->is_powered || pm_runtime_suspended(hba->dev))
Stanley Chu Jan. 12, 2021, 9:17 a.m. UTC | #3 
Hi Can,

On Tue, 2021-01-12 at 14:52 +0800, Can Guo wrote:
> On 2021-01-12 14:35, Stanley Chu wrote:
> > Hi Can,
> > 
> > On Sat, 2021-01-02 at 05:59 -0800, 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>
> >> 
> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> >> index e221add..9829c8d 100644
> >> --- a/drivers/scsi/ufs/ufshcd.c
> >> +++ b/drivers/scsi/ufs/ufshcd.c
> >> @@ -94,6 +94,8 @@
> >>  		       16, 4, buf, __len, false);                        \
> >>  } while (0)
> >> 
> >> +static bool early_suspend;
> >> +
> >>  int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len,
> >>  		     const char *prefix)
> >>  {
> >> @@ -8896,8 +8898,14 @@ int ufshcd_system_suspend(struct ufs_hba *hba)
> >>  	int ret = 0;
> >>  	ktime_t start = ktime_get();
> >> 
> >> +	if (!hba) {
> >> +		early_suspend = true;
> >> +		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,9 +8953,12 @@ 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 (unlikely(early_suspend)) {
> >> +		early_suspend = false;
> >> +		down(&hba->eh_sem);
> >>  	}
> > 
> > I guess early_suspend here is to handle the case that hba is null 
> > during
> > ufshcd_system_suspend() but !null during ufshcd_system_resume(). If 
> > yes,
> > would it be possible? If no, may I know what is the purpose?
> > 
> 
> Yes, you are right. I think it is possible. platform_set_drvdata()
> is called in ufshcd_pltfrm_init(). Say suspend happens before
> platform_set_drvdata() is called, but resume comes back after
> platform_set_drvdata() is called. What do you think?

Thanks for remind. After looking into system suspend flow, kernel thread
may continue running even after UFS suspend callback is executed by
suspend flow.

Feel free to add
Acked-by: Stanley Chu <stanley.chu@mediatek.com>
diff mbox series

Patch

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index e221add..9829c8d 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -94,6 +94,8 @@ 
 		       16, 4, buf, __len, false);                        \
 } while (0)
 
+static bool early_suspend;
+
 int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len,
 		     const char *prefix)
 {
@@ -8896,8 +8898,14 @@  int ufshcd_system_suspend(struct ufs_hba *hba)
 	int ret = 0;
 	ktime_t start = ktime_get();
 
+	if (!hba) {
+		early_suspend = true;
+		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,9 +8953,12 @@  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 (unlikely(early_suspend)) {
+		early_suspend = false;
+		down(&hba->eh_sem);
 	}
 
 	if (!hba->is_powered || pm_runtime_suspended(hba->dev))