After checking all possible call chains to acpi_os_execute here,
my tool finds that acpi_os_execute is never called in atomic context.
And acpi_os_execute calls acpi_debugger_create_thread
which calls mutex_lock,
thus it proves again that acpi_os_execute can
call functions which may sleep.
Thus GFP_ATOMIC is not necessary, and it can be replaced with GFP_KERNEL.
This is found by a static analysis tool named DCNS written by myself.
Signed-off-by: Jia-Ju Bai <[email protected]>
---
drivers/acpi/osl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 3bb46cb..8ee605e 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -1066,7 +1066,7 @@ acpi_status acpi_os_execute(acpi_execute_type type,
* having a static work_struct.
*/
- dpc = kzalloc(sizeof(struct acpi_os_dpc), GFP_ATOMIC);
+ dpc = kzalloc(sizeof(struct acpi_os_dpc), GFP_KERNEL);
if (!dpc)
return AE_NO_MEMORY;
--
1.7.9.5
On Thursday, January 25, 2018 11:13:41 AM CET Jia-Ju Bai wrote:
> After checking all possible call chains to acpi_os_execute here,
> my tool finds that acpi_os_execute is never called in atomic context.
> And acpi_os_execute calls acpi_debugger_create_thread
> which calls mutex_lock,
> thus it proves again that acpi_os_execute can
> call functions which may sleep.
> Thus GFP_ATOMIC is not necessary, and it can be replaced with GFP_KERNEL.
>
> This is found by a static analysis tool named DCNS written by myself.
>
> Signed-off-by: Jia-Ju Bai <[email protected]>
> ---
> drivers/acpi/osl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 3bb46cb..8ee605e 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -1066,7 +1066,7 @@ acpi_status acpi_os_execute(acpi_execute_type type,
> * having a static work_struct.
> */
>
> - dpc = kzalloc(sizeof(struct acpi_os_dpc), GFP_ATOMIC);
> + dpc = kzalloc(sizeof(struct acpi_os_dpc), GFP_KERNEL);
> if (!dpc)
> return AE_NO_MEMORY;
>
>
Applied, thanks!
Quoting Rafael J. Wysocki (2018-02-08 09:51:41)
> On Thursday, January 25, 2018 11:13:41 AM CET Jia-Ju Bai wrote:
> > After checking all possible call chains to acpi_os_execute here,
> > my tool finds that acpi_os_execute is never called in atomic context.
> > And acpi_os_execute calls acpi_debugger_create_thread
> > which calls mutex_lock,
> > thus it proves again that acpi_os_execute can
> > call functions which may sleep.
> > Thus GFP_ATOMIC is not necessary, and it can be replaced with GFP_KERNEL.
> >
> > This is found by a static analysis tool named DCNS written by myself.
> >
> > Signed-off-by: Jia-Ju Bai <[email protected]>
> > ---
> > drivers/acpi/osl.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> > index 3bb46cb..8ee605e 100644
> > --- a/drivers/acpi/osl.c
> > +++ b/drivers/acpi/osl.c
> > @@ -1066,7 +1066,7 @@ acpi_status acpi_os_execute(acpi_execute_type type,
> > * having a static work_struct.
> > */
> >
> > - dpc = kzalloc(sizeof(struct acpi_os_dpc), GFP_ATOMIC);
> > + dpc = kzalloc(sizeof(struct acpi_os_dpc), GFP_KERNEL);
> > if (!dpc)
> > return AE_NO_MEMORY;
> >
> >
>
> Applied, thanks!
Hmm, not this patch per se, but
https://intel-gfx-ci.01.org/tree/next/next-20180207/fi-bxt-dsi/dmesg0.log
[ 111.378236] BUG: sleeping function called from invalid context at mm/slab.h:420
[ 111.378259] in_atomic(): 1, irqs_disabled(): 1, pid: 1701, name: gem_exec_flush
[ 111.378275] INFO: lockdep is turned off.
[ 111.378277] irq event stamp: 0
[ 111.378280] hardirqs last enabled at (0): [< (null)>] (null)
[ 111.378286] hardirqs last disabled at (0): [<00000000a01fa473>] copy_process.part.7+0x2f1/0x1db0
[ 111.378290] softirqs last enabled at (0): [<00000000a01fa473>] copy_process.part.7+0x2f1/0x1db0
[ 111.378292] softirqs last disabled at (0): [< (null)>] (null)
[ 111.378293] Preemption disabled at:
[ 111.378298] [<ffffffffa18f14f6>] __mutex_lock+0x56/0x9b0
[ 111.378311] CPU: 1 PID: 1701 Comm: gem_exec_flush Tainted: G U W 4.15.0-next-20180207-g5d1c98967100-next-20180207 #1
[ 111.378313] Hardware name: Intel Corp. Broxton P/Apollolake RVP1A, BIOS APLKRVPA.X64.0150.B11.1608081044 08/08/2016
[ 111.378314] Call Trace:
[ 111.378318] <IRQ>
[ 111.378323] dump_stack+0x5f/0x86
[ 111.378328] ___might_sleep+0x1d9/0x240
[ 111.378334] ? acpi_os_execute+0x2d/0x130
[ 111.378338] kmem_cache_alloc_trace+0x1ae/0x2b0
[ 111.378344] ? acpi_ev_asynch_enable_gpe+0x2d/0x2d
[ 111.378347] acpi_os_execute+0x2d/0x130
[ 111.378351] acpi_ev_gpe_dispatch+0xd7/0x120
[ 111.378355] acpi_ev_gpe_detect+0x156/0x195
[ 111.378362] acpi_ev_sci_xrupt_handler+0x16/0x28
[ 111.378365] acpi_irq+0xd/0x30
[ 111.378369] __handle_irq_event_percpu+0x3c/0x340
[ 111.378374] handle_irq_event_percpu+0x1b/0x50
[ 111.378378] handle_irq_event+0x2f/0x50
[ 111.378381] handle_fasteoi_irq+0x93/0x150
[ 111.378386] handle_irq+0x11/0x20
[ 111.378390] do_IRQ+0x5e/0x120
[ 111.378395] common_interrupt+0xbb/0xbb
[ 111.378397] </IRQ>
does tell us that acpi_os_execute() is called in irq context.
-Chris
On Thursday, February 8, 2018 11:13:10 AM CET Chris Wilson wrote:
> Quoting Rafael J. Wysocki (2018-02-08 09:51:41)
> > On Thursday, January 25, 2018 11:13:41 AM CET Jia-Ju Bai wrote:
> > > After checking all possible call chains to acpi_os_execute here,
> > > my tool finds that acpi_os_execute is never called in atomic context.
> > > And acpi_os_execute calls acpi_debugger_create_thread
> > > which calls mutex_lock,
> > > thus it proves again that acpi_os_execute can
> > > call functions which may sleep.
> > > Thus GFP_ATOMIC is not necessary, and it can be replaced with GFP_KERNEL.
> > >
> > > This is found by a static analysis tool named DCNS written by myself.
> > >
> > > Signed-off-by: Jia-Ju Bai <[email protected]>
> > > ---
> > > drivers/acpi/osl.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> > > index 3bb46cb..8ee605e 100644
> > > --- a/drivers/acpi/osl.c
> > > +++ b/drivers/acpi/osl.c
> > > @@ -1066,7 +1066,7 @@ acpi_status acpi_os_execute(acpi_execute_type type,
> > > * having a static work_struct.
> > > */
> > >
> > > - dpc = kzalloc(sizeof(struct acpi_os_dpc), GFP_ATOMIC);
> > > + dpc = kzalloc(sizeof(struct acpi_os_dpc), GFP_KERNEL);
> > > if (!dpc)
> > > return AE_NO_MEMORY;
> > >
> > >
> >
> > Applied, thanks!
>
> Hmm, not this patch per se, but
>
> https://intel-gfx-ci.01.org/tree/next/next-20180207/fi-bxt-dsi/dmesg0.log
> [ 111.378236] BUG: sleeping function called from invalid context at mm/slab.h:420
> [ 111.378259] in_atomic(): 1, irqs_disabled(): 1, pid: 1701, name: gem_exec_flush
> [ 111.378275] INFO: lockdep is turned off.
> [ 111.378277] irq event stamp: 0
> [ 111.378280] hardirqs last enabled at (0): [< (null)>] (null)
> [ 111.378286] hardirqs last disabled at (0): [<00000000a01fa473>] copy_process.part.7+0x2f1/0x1db0
> [ 111.378290] softirqs last enabled at (0): [<00000000a01fa473>] copy_process.part.7+0x2f1/0x1db0
> [ 111.378292] softirqs last disabled at (0): [< (null)>] (null)
> [ 111.378293] Preemption disabled at:
> [ 111.378298] [<ffffffffa18f14f6>] __mutex_lock+0x56/0x9b0
> [ 111.378311] CPU: 1 PID: 1701 Comm: gem_exec_flush Tainted: G U W 4.15.0-next-20180207-g5d1c98967100-next-20180207 #1
> [ 111.378313] Hardware name: Intel Corp. Broxton P/Apollolake RVP1A, BIOS APLKRVPA.X64.0150.B11.1608081044 08/08/2016
> [ 111.378314] Call Trace:
> [ 111.378318] <IRQ>
> [ 111.378323] dump_stack+0x5f/0x86
> [ 111.378328] ___might_sleep+0x1d9/0x240
> [ 111.378334] ? acpi_os_execute+0x2d/0x130
> [ 111.378338] kmem_cache_alloc_trace+0x1ae/0x2b0
> [ 111.378344] ? acpi_ev_asynch_enable_gpe+0x2d/0x2d
> [ 111.378347] acpi_os_execute+0x2d/0x130
> [ 111.378351] acpi_ev_gpe_dispatch+0xd7/0x120
> [ 111.378355] acpi_ev_gpe_detect+0x156/0x195
> [ 111.378362] acpi_ev_sci_xrupt_handler+0x16/0x28
> [ 111.378365] acpi_irq+0xd/0x30
> [ 111.378369] __handle_irq_event_percpu+0x3c/0x340
> [ 111.378374] handle_irq_event_percpu+0x1b/0x50
> [ 111.378378] handle_irq_event+0x2f/0x50
> [ 111.378381] handle_fasteoi_irq+0x93/0x150
> [ 111.378386] handle_irq+0x11/0x20
> [ 111.378390] do_IRQ+0x5e/0x120
> [ 111.378395] common_interrupt+0xbb/0xbb
> [ 111.378397] </IRQ>
>
> does tell us that acpi_os_execute() is called in irq context.
Well, right, thanks!
I overlooked this instance, so dropping the patch.
Thanks,
Rafael
On 2018/2/8 18:20, Rafael J. Wysocki wrote:
> On Thursday, February 8, 2018 11:13:10 AM CET Chris Wilson wrote:
>> Quoting Rafael J. Wysocki (2018-02-08 09:51:41)
>>> On Thursday, January 25, 2018 11:13:41 AM CET Jia-Ju Bai wrote:
>>>> After checking all possible call chains to acpi_os_execute here,
>>>> my tool finds that acpi_os_execute is never called in atomic context.
>>>> And acpi_os_execute calls acpi_debugger_create_thread
>>>> which calls mutex_lock,
>>>> thus it proves again that acpi_os_execute can
>>>> call functions which may sleep.
>>>> Thus GFP_ATOMIC is not necessary, and it can be replaced with GFP_KERNEL.
>>>>
>>>> This is found by a static analysis tool named DCNS written by myself.
>>>>
>>>> Signed-off-by: Jia-Ju Bai <[email protected]>
>>>> ---
>>>> drivers/acpi/osl.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
>>>> index 3bb46cb..8ee605e 100644
>>>> --- a/drivers/acpi/osl.c
>>>> +++ b/drivers/acpi/osl.c
>>>> @@ -1066,7 +1066,7 @@ acpi_status acpi_os_execute(acpi_execute_type type,
>>>> * having a static work_struct.
>>>> */
>>>>
>>>> - dpc = kzalloc(sizeof(struct acpi_os_dpc), GFP_ATOMIC);
>>>> + dpc = kzalloc(sizeof(struct acpi_os_dpc), GFP_KERNEL);
>>>> if (!dpc)
>>>> return AE_NO_MEMORY;
>>>>
>>>>
>>> Applied, thanks!
>> Hmm, not this patch per se, but
>>
>> https://intel-gfx-ci.01.org/tree/next/next-20180207/fi-bxt-dsi/dmesg0.log
>> [ 111.378236] BUG: sleeping function called from invalid context at mm/slab.h:420
>> [ 111.378259] in_atomic(): 1, irqs_disabled(): 1, pid: 1701, name: gem_exec_flush
>> [ 111.378275] INFO: lockdep is turned off.
>> [ 111.378277] irq event stamp: 0
>> [ 111.378280] hardirqs last enabled at (0): [< (null)>] (null)
>> [ 111.378286] hardirqs last disabled at (0): [<00000000a01fa473>] copy_process.part.7+0x2f1/0x1db0
>> [ 111.378290] softirqs last enabled at (0): [<00000000a01fa473>] copy_process.part.7+0x2f1/0x1db0
>> [ 111.378292] softirqs last disabled at (0): [< (null)>] (null)
>> [ 111.378293] Preemption disabled at:
>> [ 111.378298] [<ffffffffa18f14f6>] __mutex_lock+0x56/0x9b0
>> [ 111.378311] CPU: 1 PID: 1701 Comm: gem_exec_flush Tainted: G U W 4.15.0-next-20180207-g5d1c98967100-next-20180207 #1
>> [ 111.378313] Hardware name: Intel Corp. Broxton P/Apollolake RVP1A, BIOS APLKRVPA.X64.0150.B11.1608081044 08/08/2016
>> [ 111.378314] Call Trace:
>> [ 111.378318] <IRQ>
>> [ 111.378323] dump_stack+0x5f/0x86
>> [ 111.378328] ___might_sleep+0x1d9/0x240
>> [ 111.378334] ? acpi_os_execute+0x2d/0x130
>> [ 111.378338] kmem_cache_alloc_trace+0x1ae/0x2b0
>> [ 111.378344] ? acpi_ev_asynch_enable_gpe+0x2d/0x2d
>> [ 111.378347] acpi_os_execute+0x2d/0x130
>> [ 111.378351] acpi_ev_gpe_dispatch+0xd7/0x120
>> [ 111.378355] acpi_ev_gpe_detect+0x156/0x195
>> [ 111.378362] acpi_ev_sci_xrupt_handler+0x16/0x28
>> [ 111.378365] acpi_irq+0xd/0x30
>> [ 111.378369] __handle_irq_event_percpu+0x3c/0x340
>> [ 111.378374] handle_irq_event_percpu+0x1b/0x50
>> [ 111.378378] handle_irq_event+0x2f/0x50
>> [ 111.378381] handle_fasteoi_irq+0x93/0x150
>> [ 111.378386] handle_irq+0x11/0x20
>> [ 111.378390] do_IRQ+0x5e/0x120
>> [ 111.378395] common_interrupt+0xbb/0xbb
>> [ 111.378397] </IRQ>
>>
>> does tell us that acpi_os_execute() is called in irq context.
> Well, right, thanks!
>
> I overlooked this instance, so dropping the patch.
Sorry for my false positive.
My tool missed that acpi_ev_sci_xrupt_handler() is an interrupt handler,
sorry.
Thank Chris for testing my patch.
Best wishes,
Jia-Ju Bai
On Thu, Feb 8, 2018 at 2:41 PM, Jia-Ju Bai <[email protected]> wrote:
>
>
> On 2018/2/8 18:20, Rafael J. Wysocki wrote:
>>
>> On Thursday, February 8, 2018 11:13:10 AM CET Chris Wilson wrote:
>>>
>>> Quoting Rafael J. Wysocki (2018-02-08 09:51:41)
>>>>
>>>> On Thursday, January 25, 2018 11:13:41 AM CET Jia-Ju Bai wrote:
>>>>>
>>>>> After checking all possible call chains to acpi_os_execute here,
>>>>> my tool finds that acpi_os_execute is never called in atomic context.
>>>>> And acpi_os_execute calls acpi_debugger_create_thread
>>>>> which calls mutex_lock,
>>>>> thus it proves again that acpi_os_execute can
>>>>> call functions which may sleep.
>>>>> Thus GFP_ATOMIC is not necessary, and it can be replaced with
>>>>> GFP_KERNEL.
>>>>>
>>>>> This is found by a static analysis tool named DCNS written by myself.
>>>>>
>>>>> Signed-off-by: Jia-Ju Bai <[email protected]>
>>>>> ---
>>>>> drivers/acpi/osl.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
>>>>> index 3bb46cb..8ee605e 100644
>>>>> --- a/drivers/acpi/osl.c
>>>>> +++ b/drivers/acpi/osl.c
>>>>> @@ -1066,7 +1066,7 @@ acpi_status acpi_os_execute(acpi_execute_type
>>>>> type,
>>>>> * having a static work_struct.
>>>>> */
>>>>> - dpc = kzalloc(sizeof(struct acpi_os_dpc), GFP_ATOMIC);
>>>>> + dpc = kzalloc(sizeof(struct acpi_os_dpc), GFP_KERNEL);
>>>>> if (!dpc)
>>>>> return AE_NO_MEMORY;
>>>>>
>>>>
>>>> Applied, thanks!
>>>
>>> Hmm, not this patch per se, but
>>>
>>> https://intel-gfx-ci.01.org/tree/next/next-20180207/fi-bxt-dsi/dmesg0.log
>>> [ 111.378236] BUG: sleeping function called from invalid context at
>>> mm/slab.h:420
>>> [ 111.378259] in_atomic(): 1, irqs_disabled(): 1, pid: 1701, name:
>>> gem_exec_flush
>>> [ 111.378275] INFO: lockdep is turned off.
>>> [ 111.378277] irq event stamp: 0
>>> [ 111.378280] hardirqs last enabled at (0): [< (null)>]
>>> (null)
>>> [ 111.378286] hardirqs last disabled at (0): [<00000000a01fa473>]
>>> copy_process.part.7+0x2f1/0x1db0
>>> [ 111.378290] softirqs last enabled at (0): [<00000000a01fa473>]
>>> copy_process.part.7+0x2f1/0x1db0
>>> [ 111.378292] softirqs last disabled at (0): [< (null)>]
>>> (null)
>>> [ 111.378293] Preemption disabled at:
>>> [ 111.378298] [<ffffffffa18f14f6>] __mutex_lock+0x56/0x9b0
>>> [ 111.378311] CPU: 1 PID: 1701 Comm: gem_exec_flush Tainted: G U W
>>> 4.15.0-next-20180207-g5d1c98967100-next-20180207 #1
>>> [ 111.378313] Hardware name: Intel Corp. Broxton P/Apollolake RVP1A,
>>> BIOS APLKRVPA.X64.0150.B11.1608081044 08/08/2016
>>> [ 111.378314] Call Trace:
>>> [ 111.378318] <IRQ>
>>> [ 111.378323] dump_stack+0x5f/0x86
>>> [ 111.378328] ___might_sleep+0x1d9/0x240
>>> [ 111.378334] ? acpi_os_execute+0x2d/0x130
>>> [ 111.378338] kmem_cache_alloc_trace+0x1ae/0x2b0
>>> [ 111.378344] ? acpi_ev_asynch_enable_gpe+0x2d/0x2d
>>> [ 111.378347] acpi_os_execute+0x2d/0x130
>>> [ 111.378351] acpi_ev_gpe_dispatch+0xd7/0x120
>>> [ 111.378355] acpi_ev_gpe_detect+0x156/0x195
>>> [ 111.378362] acpi_ev_sci_xrupt_handler+0x16/0x28
>>> [ 111.378365] acpi_irq+0xd/0x30
>>> [ 111.378369] __handle_irq_event_percpu+0x3c/0x340
>>> [ 111.378374] handle_irq_event_percpu+0x1b/0x50
>>> [ 111.378378] handle_irq_event+0x2f/0x50
>>> [ 111.378381] handle_fasteoi_irq+0x93/0x150
>>> [ 111.378386] handle_irq+0x11/0x20
>>> [ 111.378390] do_IRQ+0x5e/0x120
>>> [ 111.378395] common_interrupt+0xbb/0xbb
>>> [ 111.378397] </IRQ>
>>>
>>> does tell us that acpi_os_execute() is called in irq context.
>>
>> Well, right, thanks!
>>
>> I overlooked this instance, so dropping the patch.
>
>
> Sorry for my false positive.
No worries.
> My tool missed that acpi_ev_sci_xrupt_handler() is an interrupt handler, sorry.
That just means that the tool is not perfect, which is nothing unusual. :-)
I only would suggest double checking its findings before sending out
patches next time.