2019-05-23 22:01:35

by Rob Clark

[permalink] [raw]
Subject: [PATCH] PM / devfreq: try_then_request_governor should not return NULL

From: Rob Clark <[email protected]>

The two spots it is called expect either an IS_ERR() or a valid pointer,
but not NULL.

Fixes this crash that I came across:

Unable to handle kernel NULL pointer dereference at virtual address 0000000000000030
Mem abort info:
ESR = 0x96000005
Exception class = DABT (current EL), IL = 32 bits
SET = 0, FnV = 0
EA = 0, S1PTW = 0
Data abort info:
ISV = 0, ISS = 0x00000005
CM = 0, WnR = 0
[0000000000000030] user address but active_mm is swapper
Internal error: Oops: 96000005 [#1] PREEMPT SMP
Modules linked in:
Process kworker/2:1 (pid: 212, stack limit = 0x(____ptrval____))
CPU: 2 PID: 212 Comm: kworker/2:1 Not tainted 5.1.0-43338-g460e6984675c-dirty #54
Hardware name: Google Cheza (rev3+) (DT)
Workqueue: events deferred_probe_work_func
pstate: 00c00009 (nzcv daif +PAN +UAO)
pc : devfreq_add_device+0x2e4/0x410
lr : devfreq_add_device+0x2d4/0x410
sp : ffffff8013d93740
x29: ffffff8013d93790 x28: ffffffc0f54f8670
x27: 0000000000000001 x26: 0000000000000007
x25: ffffff80124abfd8 x24: 0000000000000000
x23: ffffffc0fabc4048 x22: ffffffc0fabc4388
x21: ffffffc0fabc4010 x20: ffffffc0fa243010
x19: ffffffc0fabc4000 x18: 0000000091c3d373
x17: 0000000000000400 x16: 000000000000001a
x15: 000000019e06d400 x14: 0000000000000001
x13: 0000000000000000 x12: 00000000000006b6
x11: 0000000000000000 x10: 0000000000000000
x9 : ffffffc0fa18ba00 x8 : 0000000000000000
x7 : 0000000000000000 x6 : ffffff80127a3d9a
x5 : ffffff8013d93550 x4 : 0000000000000000
x3 : 0000000000000000 x2 : 0000000000000000
x1 : 00000000000000fe x0 : 0000000000000000
Call trace:
devfreq_add_device+0x2e4/0x410
devm_devfreq_add_device+0x64/0xac
msm_gpu_init+0x320/0x5c0
adreno_gpu_init+0x21c/0x274
a6xx_gpu_init+0x68/0xf4
adreno_bind+0x158/0x284
component_bind_all+0x110/0x204
msm_drm_bind+0x118/0x5b8
try_to_bring_up_master+0x15c/0x19c
component_master_add_with_match+0xb4/0xec
msm_pdev_probe+0x1f0/0x27c
platform_drv_probe+0x90/0xb0
really_probe+0x120/0x298
driver_probe_device+0x64/0xfc
__device_attach_driver+0x8c/0xa4
bus_for_each_drv+0x88/0xd0
__device_attach+0xac/0x134
device_initial_probe+0x20/0x2c
bus_probe_device+0x34/0x90
deferred_probe_work_func+0x74/0xac
process_one_work+0x210/0x428
worker_thread+0x278/0x3e4
kthread+0x120/0x130
ret_from_fork+0x10/0x18
Code: aa0003f8 b13ffc1f 54000762 f901c278 (f9401b08)
---[ end trace a6ecc18ce5894375 ]---
Kernel panic - not syncing: Fatal exception

Signed-off-by: Rob Clark <[email protected]>
---
drivers/devfreq/devfreq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 0ae3de76833b..d29f66f0e52a 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -254,7 +254,7 @@ static struct devfreq_governor *try_then_request_governor(const char *name)
/* Restore previous state before return */
mutex_lock(&devfreq_list_lock);
if (err)
- return NULL;
+ return ERR_PTR(err);

governor = find_devfreq_governor(name);
}
--
2.20.1


2019-05-24 07:17:04

by Chanwoo Choi

[permalink] [raw]
Subject: Re: [PATCH] PM / devfreq: try_then_request_governor should not return NULL

Hi,

This issue[1] is already fixed on latest linux.git
You can check it. Thanks.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b53b0128052ffd687797d5f4deeb76327e7b5711

Regards,
Chanwoo Choi


On 19. 5. 24. 오전 6:58, Rob Clark wrote:
> From: Rob Clark <[email protected]>
>
> The two spots it is called expect either an IS_ERR() or a valid pointer,
> but not NULL.
>
> Fixes this crash that I came across:
>
> Unable to handle kernel NULL pointer dereference at virtual address 0000000000000030
> Mem abort info:
> ESR = 0x96000005
> Exception class = DABT (current EL), IL = 32 bits
> SET = 0, FnV = 0
> EA = 0, S1PTW = 0
> Data abort info:
> ISV = 0, ISS = 0x00000005
> CM = 0, WnR = 0
> [0000000000000030] user address but active_mm is swapper
> Internal error: Oops: 96000005 [#1] PREEMPT SMP
> Modules linked in:
> Process kworker/2:1 (pid: 212, stack limit = 0x(____ptrval____))
> CPU: 2 PID: 212 Comm: kworker/2:1 Not tainted 5.1.0-43338-g460e6984675c-dirty #54
> Hardware name: Google Cheza (rev3+) (DT)
> Workqueue: events deferred_probe_work_func
> pstate: 00c00009 (nzcv daif +PAN +UAO)
> pc : devfreq_add_device+0x2e4/0x410
> lr : devfreq_add_device+0x2d4/0x410
> sp : ffffff8013d93740
> x29: ffffff8013d93790 x28: ffffffc0f54f8670
> x27: 0000000000000001 x26: 0000000000000007
> x25: ffffff80124abfd8 x24: 0000000000000000
> x23: ffffffc0fabc4048 x22: ffffffc0fabc4388
> x21: ffffffc0fabc4010 x20: ffffffc0fa243010
> x19: ffffffc0fabc4000 x18: 0000000091c3d373
> x17: 0000000000000400 x16: 000000000000001a
> x15: 000000019e06d400 x14: 0000000000000001
> x13: 0000000000000000 x12: 00000000000006b6
> x11: 0000000000000000 x10: 0000000000000000
> x9 : ffffffc0fa18ba00 x8 : 0000000000000000
> x7 : 0000000000000000 x6 : ffffff80127a3d9a
> x5 : ffffff8013d93550 x4 : 0000000000000000
> x3 : 0000000000000000 x2 : 0000000000000000
> x1 : 00000000000000fe x0 : 0000000000000000
> Call trace:
> devfreq_add_device+0x2e4/0x410
> devm_devfreq_add_device+0x64/0xac
> msm_gpu_init+0x320/0x5c0
> adreno_gpu_init+0x21c/0x274
> a6xx_gpu_init+0x68/0xf4
> adreno_bind+0x158/0x284
> component_bind_all+0x110/0x204
> msm_drm_bind+0x118/0x5b8
> try_to_bring_up_master+0x15c/0x19c
> component_master_add_with_match+0xb4/0xec
> msm_pdev_probe+0x1f0/0x27c
> platform_drv_probe+0x90/0xb0
> really_probe+0x120/0x298
> driver_probe_device+0x64/0xfc
> __device_attach_driver+0x8c/0xa4
> bus_for_each_drv+0x88/0xd0
> __device_attach+0xac/0x134
> device_initial_probe+0x20/0x2c
> bus_probe_device+0x34/0x90
> deferred_probe_work_func+0x74/0xac
> process_one_work+0x210/0x428
> worker_thread+0x278/0x3e4
> kthread+0x120/0x130
> ret_from_fork+0x10/0x18
> Code: aa0003f8 b13ffc1f 54000762 f901c278 (f9401b08)
> ---[ end trace a6ecc18ce5894375 ]---
> Kernel panic - not syncing: Fatal exception
>
> Signed-off-by: Rob Clark <[email protected]>
> ---
> drivers/devfreq/devfreq.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 0ae3de76833b..d29f66f0e52a 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -254,7 +254,7 @@ static struct devfreq_governor *try_then_request_governor(const char *name)
> /* Restore previous state before return */
> mutex_lock(&devfreq_list_lock);
> if (err)
> - return NULL;
> + return ERR_PTR(err);
>
> governor = find_devfreq_governor(name);
> }
>


--
Best Regards,
Chanwoo Choi
Samsung Electronics

2019-05-24 13:19:16

by Rob Clark

[permalink] [raw]
Subject: Re: [PATCH] PM / devfreq: try_then_request_governor should not return NULL

Ahh, thanks, I've not moved to the latest -rc yet..

That commit would be a good candidate for 5.1.y stable branch

BR,
-R

On Fri, May 24, 2019 at 12:13 AM Chanwoo Choi <[email protected]> wrote:
>
> Hi,
>
> This issue[1] is already fixed on latest linux.git
> You can check it. Thanks.
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b53b0128052ffd687797d5f4deeb76327e7b5711
>
> Regards,
> Chanwoo Choi
>
>
> On 19. 5. 24. 오전 6:58, Rob Clark wrote:
> > From: Rob Clark <[email protected]>
> >
> > The two spots it is called expect either an IS_ERR() or a valid pointer,
> > but not NULL.
> >
> > Fixes this crash that I came across:
> >
> > Unable to handle kernel NULL pointer dereference at virtual address 0000000000000030
> > Mem abort info:
> > ESR = 0x96000005
> > Exception class = DABT (current EL), IL = 32 bits
> > SET = 0, FnV = 0
> > EA = 0, S1PTW = 0
> > Data abort info:
> > ISV = 0, ISS = 0x00000005
> > CM = 0, WnR = 0
> > [0000000000000030] user address but active_mm is swapper
> > Internal error: Oops: 96000005 [#1] PREEMPT SMP
> > Modules linked in:
> > Process kworker/2:1 (pid: 212, stack limit = 0x(____ptrval____))
> > CPU: 2 PID: 212 Comm: kworker/2:1 Not tainted 5.1.0-43338-g460e6984675c-dirty #54
> > Hardware name: Google Cheza (rev3+) (DT)
> > Workqueue: events deferred_probe_work_func
> > pstate: 00c00009 (nzcv daif +PAN +UAO)
> > pc : devfreq_add_device+0x2e4/0x410
> > lr : devfreq_add_device+0x2d4/0x410
> > sp : ffffff8013d93740
> > x29: ffffff8013d93790 x28: ffffffc0f54f8670
> > x27: 0000000000000001 x26: 0000000000000007
> > x25: ffffff80124abfd8 x24: 0000000000000000
> > x23: ffffffc0fabc4048 x22: ffffffc0fabc4388
> > x21: ffffffc0fabc4010 x20: ffffffc0fa243010
> > x19: ffffffc0fabc4000 x18: 0000000091c3d373
> > x17: 0000000000000400 x16: 000000000000001a
> > x15: 000000019e06d400 x14: 0000000000000001
> > x13: 0000000000000000 x12: 00000000000006b6
> > x11: 0000000000000000 x10: 0000000000000000
> > x9 : ffffffc0fa18ba00 x8 : 0000000000000000
> > x7 : 0000000000000000 x6 : ffffff80127a3d9a
> > x5 : ffffff8013d93550 x4 : 0000000000000000
> > x3 : 0000000000000000 x2 : 0000000000000000
> > x1 : 00000000000000fe x0 : 0000000000000000
> > Call trace:
> > devfreq_add_device+0x2e4/0x410
> > devm_devfreq_add_device+0x64/0xac
> > msm_gpu_init+0x320/0x5c0
> > adreno_gpu_init+0x21c/0x274
> > a6xx_gpu_init+0x68/0xf4
> > adreno_bind+0x158/0x284
> > component_bind_all+0x110/0x204
> > msm_drm_bind+0x118/0x5b8
> > try_to_bring_up_master+0x15c/0x19c
> > component_master_add_with_match+0xb4/0xec
> > msm_pdev_probe+0x1f0/0x27c
> > platform_drv_probe+0x90/0xb0
> > really_probe+0x120/0x298
> > driver_probe_device+0x64/0xfc
> > __device_attach_driver+0x8c/0xa4
> > bus_for_each_drv+0x88/0xd0
> > __device_attach+0xac/0x134
> > device_initial_probe+0x20/0x2c
> > bus_probe_device+0x34/0x90
> > deferred_probe_work_func+0x74/0xac
> > process_one_work+0x210/0x428
> > worker_thread+0x278/0x3e4
> > kthread+0x120/0x130
> > ret_from_fork+0x10/0x18
> > Code: aa0003f8 b13ffc1f 54000762 f901c278 (f9401b08)
> > ---[ end trace a6ecc18ce5894375 ]---
> > Kernel panic - not syncing: Fatal exception
> >
> > Signed-off-by: Rob Clark <[email protected]>
> > ---
> > drivers/devfreq/devfreq.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> > index 0ae3de76833b..d29f66f0e52a 100644
> > --- a/drivers/devfreq/devfreq.c
> > +++ b/drivers/devfreq/devfreq.c
> > @@ -254,7 +254,7 @@ static struct devfreq_governor *try_then_request_governor(const char *name)
> > /* Restore previous state before return */
> > mutex_lock(&devfreq_list_lock);
> > if (err)
> > - return NULL;
> > + return ERR_PTR(err);
> >
> > governor = find_devfreq_governor(name);
> > }
> >
>
>
> --
> Best Regards,
> Chanwoo Choi
> Samsung Electronics

2019-05-28 01:18:13

by Chanwoo Choi

[permalink] [raw]
Subject: Re: [PATCH] PM / devfreq: try_then_request_governor should not return NULL

Hi Rob,

You're right. It have to be posted to [email protected].
As you recommended, I send it[1] to [email protected] for fixup.
[1]https://lkml.org/lkml/2019/5/27/547

Best Regards,
Chanwoo Choi

On 19. 5. 24. 오후 10:15, Rob Clark wrote:
> Ahh, thanks, I've not moved to the latest -rc yet..
>
> That commit would be a good candidate for 5.1.y stable branch
>
> BR,
> -R
>
> On Fri, May 24, 2019 at 12:13 AM Chanwoo Choi <[email protected]> wrote:
>>
>> Hi,
>>
>> This issue[1] is already fixed on latest linux.git
>> You can check it. Thanks.
>>
>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b53b0128052ffd687797d5f4deeb76327e7b5711
>>
>> Regards,
>> Chanwoo Choi
>>
>>
>> On 19. 5. 24. 오전 6:58, Rob Clark wrote:
>>> From: Rob Clark <[email protected]>
>>>
>>> The two spots it is called expect either an IS_ERR() or a valid pointer,
>>> but not NULL.
>>>
>>> Fixes this crash that I came across:
>>>
>>> Unable to handle kernel NULL pointer dereference at virtual address 0000000000000030
>>> Mem abort info:
>>> ESR = 0x96000005
>>> Exception class = DABT (current EL), IL = 32 bits
>>> SET = 0, FnV = 0
>>> EA = 0, S1PTW = 0
>>> Data abort info:
>>> ISV = 0, ISS = 0x00000005
>>> CM = 0, WnR = 0
>>> [0000000000000030] user address but active_mm is swapper
>>> Internal error: Oops: 96000005 [#1] PREEMPT SMP
>>> Modules linked in:
>>> Process kworker/2:1 (pid: 212, stack limit = 0x(____ptrval____))
>>> CPU: 2 PID: 212 Comm: kworker/2:1 Not tainted 5.1.0-43338-g460e6984675c-dirty #54
>>> Hardware name: Google Cheza (rev3+) (DT)
>>> Workqueue: events deferred_probe_work_func
>>> pstate: 00c00009 (nzcv daif +PAN +UAO)
>>> pc : devfreq_add_device+0x2e4/0x410
>>> lr : devfreq_add_device+0x2d4/0x410
>>> sp : ffffff8013d93740
>>> x29: ffffff8013d93790 x28: ffffffc0f54f8670
>>> x27: 0000000000000001 x26: 0000000000000007
>>> x25: ffffff80124abfd8 x24: 0000000000000000
>>> x23: ffffffc0fabc4048 x22: ffffffc0fabc4388
>>> x21: ffffffc0fabc4010 x20: ffffffc0fa243010
>>> x19: ffffffc0fabc4000 x18: 0000000091c3d373
>>> x17: 0000000000000400 x16: 000000000000001a
>>> x15: 000000019e06d400 x14: 0000000000000001
>>> x13: 0000000000000000 x12: 00000000000006b6
>>> x11: 0000000000000000 x10: 0000000000000000
>>> x9 : ffffffc0fa18ba00 x8 : 0000000000000000
>>> x7 : 0000000000000000 x6 : ffffff80127a3d9a
>>> x5 : ffffff8013d93550 x4 : 0000000000000000
>>> x3 : 0000000000000000 x2 : 0000000000000000
>>> x1 : 00000000000000fe x0 : 0000000000000000
>>> Call trace:
>>> devfreq_add_device+0x2e4/0x410
>>> devm_devfreq_add_device+0x64/0xac
>>> msm_gpu_init+0x320/0x5c0
>>> adreno_gpu_init+0x21c/0x274
>>> a6xx_gpu_init+0x68/0xf4
>>> adreno_bind+0x158/0x284
>>> component_bind_all+0x110/0x204
>>> msm_drm_bind+0x118/0x5b8
>>> try_to_bring_up_master+0x15c/0x19c
>>> component_master_add_with_match+0xb4/0xec
>>> msm_pdev_probe+0x1f0/0x27c
>>> platform_drv_probe+0x90/0xb0
>>> really_probe+0x120/0x298
>>> driver_probe_device+0x64/0xfc
>>> __device_attach_driver+0x8c/0xa4
>>> bus_for_each_drv+0x88/0xd0
>>> __device_attach+0xac/0x134
>>> device_initial_probe+0x20/0x2c
>>> bus_probe_device+0x34/0x90
>>> deferred_probe_work_func+0x74/0xac
>>> process_one_work+0x210/0x428
>>> worker_thread+0x278/0x3e4
>>> kthread+0x120/0x130
>>> ret_from_fork+0x10/0x18
>>> Code: aa0003f8 b13ffc1f 54000762 f901c278 (f9401b08)
>>> ---[ end trace a6ecc18ce5894375 ]---
>>> Kernel panic - not syncing: Fatal exception
>>>
>>> Signed-off-by: Rob Clark <[email protected]>
>>> ---
>>> drivers/devfreq/devfreq.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index 0ae3de76833b..d29f66f0e52a 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -254,7 +254,7 @@ static struct devfreq_governor *try_then_request_governor(const char *name)
>>> /* Restore previous state before return */
>>> mutex_lock(&devfreq_list_lock);
>>> if (err)
>>> - return NULL;
>>> + return ERR_PTR(err);
>>>
>>> governor = find_devfreq_governor(name);
>>> }
>>>
>>
>>
>> --
>> Best Regards,
>> Chanwoo Choi
>> Samsung Electronics
>
>


--
Best Regards,
Chanwoo Choi
Samsung Electronics