Patchwork [BUG] atmel: spi: scheduling while atomic

login
register
mail settings
Submitter Wenyou Yang
Date March 10, 2014, 9:08 a.m.
Message ID <B256D81BAE5131468A838E5D7A24364172FF9685@penmbx01>
Download mbox | patch
Permalink /patch/328520/
State New
Headers show

Comments

Wenyou Yang - March 10, 2014, 9:08 a.m.
> -----Original Message-----

> From: Jiří Prchal [mailto:jiri.prchal@aksignal.cz]

> Sent: Monday, March 10, 2014 4:12 PM

> To: Rabin Vincent; Alexandre Belloni; Yang, Wenyou

> Cc: Ferre, Nicolas; linux-arm-kernel@lists.infradead.org;

> sboyd@codeaurora.org

> Subject: Re: [BUG] atmel: spi: scheduling while atomic

> 

> Hi all,

> at first: warning is on all spi devices.

> at second: its with CONFIG_PREEMPT=y, without (# CONFIG_PREEMPT_RCU is

> not set) is OK, but I wonder if this config will be fast enough for our

> usage (nearly RT)?

> 

> 

> Dne 8.3.2014 01:41, Rabin Vincent napsal(a):

> > 2014-03-07 23:58 GMT+01:00 Alexandre Belloni

> > <alexandre.belloni@free-electrons.com>:

> >> On 05/03/2014 at 07:56:04 +0100, Jiří Prchal wrote :

> >>> [    0.902343] BUG: scheduling while atomic: spi0/383/0x00000002

> >>> [    0.906250] Modules linked in:

> >>> [    0.906250] CPU: 0 PID: 383 Comm: spi0 Not tainted 3.14.0-

> rc4_cpm9g25+ #1

> >>> [    0.906250] [<c000d9f4>] (unwind_backtrace) from [<c000bdc0>]

> (show_stack+0x10/0x14)

> >>> [    0.906250] [<c000bdc0>] (show_stack) from [<c003a224>]

> (__schedule_bug+0x48/0x60)

> >>> [    0.906250] [<c003a224>] (__schedule_bug) from [<c0390294>]

> (__schedule+0x60/0x484)

> >>> [    0.906250] [<c0390294>] (__schedule) from [<c038feac>]

> (schedule_timeout+0x17c/0x1ac)

> >>> [    0.906250] [<c038feac>] (schedule_timeout) from [<c039111c>]

> (wait_for_common+0x10c/0x1f0)

> >>> [    0.906250] [<c039111c>] (wait_for_common) from [<c0249228>]

> (atmel_spi_transfer_one_message+0x71c/0xa48)

> >>> [    0.906250] [<c0249228>] (atmel_spi_transfer_one_message) from

> [<c0246624>] (spi_pump_messages+0x210/0x238)

> >>> [    0.906250] [<c0246624>] (spi_pump_messages) from [<c00343d0>]

> (kthread_worker_fn+0x15c/0x1b4)

> >>> [    0.906250] [<c00343d0>] (kthread_worker_fn) from [<c0034534>]

> (kthread+0xb8/0xcc)

> >>> [    0.906250] [<c0034534>] (kthread) from [<c0009510>]

> (ret_from_fork+0x14/0x24)

> >>> [    0.906250] at25 spi0.0: 128 KByte at25 eeprom, pagesize 512

> >>

> >> Actually, I'm not quite sure how spi_pump_messages can be called from

> an

> >> atomic context.

> >

> > spi_pump_messages() is not called from atomic context.  Rather,

> > atmel_spi_transfer_one_message() does a spin_lock_irqsave() (via

> > atmel_spi_lock()) and then calls atmel_spi_one_transfer(), which calls

> > wait_for_completion_timeout().  That's why schedule is being called

> > from atomic context.  This was introduced by 8090d6d1a4 ("spi: atmel:

> > Refactor spi-atmel to use SPI framework queue"). I think you should

> > see the warning with CONFIG_PREEMPT=y.

> >


Thanks a lot for the report and analysis.

--->8-------- 
From ba2dd7aee12cb09b9ef159251859443e1b055669 Mon Sep 17 00:00:00 2001
From: Wenyou Yang <wenyou.yang@atmel.com>

Date: Mon, 10 Mar 2014 16:44:33 +0800
Subject: [PATCH] spi: atmel: fix BUG: scheduling while atomic with
 CONFIG_PREEMPT=y

introduced by 8090d6d1a415d3ae1a7208995decfab8f60f4f36
("spi: atmel: Refactor spi-atmel to use SPI framework queue")

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>

---
 drivers/spi/spi-atmel.c |    4 ++++
 1 file changed, 4 insertions(+)

-- 
1.7.9.5
---<8----

This patch should fix this issue.
I know it is not good one, the lock usage in the driver code should be double checked.

Thanks.

Best Regards,
Wenyou Yang.
Prchal Jiří - March 11, 2014, 9:45 a.m.
Hi,
patch applied, it helps.
Is this final solution?
Thanks

Dne 10.3.2014 10:08, Yang, Wenyou napsal(a):
>
>
>> -----Original Message-----
>> From: Jiří Prchal [mailto:jiri.prchal@aksignal.cz]
>> Sent: Monday, March 10, 2014 4:12 PM
>> To: Rabin Vincent; Alexandre Belloni; Yang, Wenyou
>> Cc: Ferre, Nicolas; linux-arm-kernel@lists.infradead.org;
>> sboyd@codeaurora.org
>> Subject: Re: [BUG] atmel: spi: scheduling while atomic
>>
>> Hi all,
>> at first: warning is on all spi devices.
>> at second: its with CONFIG_PREEMPT=y, without (# CONFIG_PREEMPT_RCU is
>> not set) is OK, but I wonder if this config will be fast enough for our
>> usage (nearly RT)?
>>
>>
>> Dne 8.3.2014 01:41, Rabin Vincent napsal(a):
>>> 2014-03-07 23:58 GMT+01:00 Alexandre Belloni
>>> <alexandre.belloni@free-electrons.com>:
>>>> On 05/03/2014 at 07:56:04 +0100, Jiří Prchal wrote :
>>>>> [    0.902343] BUG: scheduling while atomic: spi0/383/0x00000002
>>>>> [    0.906250] Modules linked in:
>>>>> [    0.906250] CPU: 0 PID: 383 Comm: spi0 Not tainted 3.14.0-
>> rc4_cpm9g25+ #1
>>>>> [    0.906250] [<c000d9f4>] (unwind_backtrace) from [<c000bdc0>]
>> (show_stack+0x10/0x14)
>>>>> [    0.906250] [<c000bdc0>] (show_stack) from [<c003a224>]
>> (__schedule_bug+0x48/0x60)
>>>>> [    0.906250] [<c003a224>] (__schedule_bug) from [<c0390294>]
>> (__schedule+0x60/0x484)
>>>>> [    0.906250] [<c0390294>] (__schedule) from [<c038feac>]
>> (schedule_timeout+0x17c/0x1ac)
>>>>> [    0.906250] [<c038feac>] (schedule_timeout) from [<c039111c>]
>> (wait_for_common+0x10c/0x1f0)
>>>>> [    0.906250] [<c039111c>] (wait_for_common) from [<c0249228>]
>> (atmel_spi_transfer_one_message+0x71c/0xa48)
>>>>> [    0.906250] [<c0249228>] (atmel_spi_transfer_one_message) from
>> [<c0246624>] (spi_pump_messages+0x210/0x238)
>>>>> [    0.906250] [<c0246624>] (spi_pump_messages) from [<c00343d0>]
>> (kthread_worker_fn+0x15c/0x1b4)
>>>>> [    0.906250] [<c00343d0>] (kthread_worker_fn) from [<c0034534>]
>> (kthread+0xb8/0xcc)
>>>>> [    0.906250] [<c0034534>] (kthread) from [<c0009510>]
>> (ret_from_fork+0x14/0x24)
>>>>> [    0.906250] at25 spi0.0: 128 KByte at25 eeprom, pagesize 512
>>>>
>>>> Actually, I'm not quite sure how spi_pump_messages can be called from
>> an
>>>> atomic context.
>>>
>>> spi_pump_messages() is not called from atomic context.  Rather,
>>> atmel_spi_transfer_one_message() does a spin_lock_irqsave() (via
>>> atmel_spi_lock()) and then calls atmel_spi_one_transfer(), which calls
>>> wait_for_completion_timeout().  That's why schedule is being called
>>> from atomic context.  This was introduced by 8090d6d1a4 ("spi: atmel:
>>> Refactor spi-atmel to use SPI framework queue"). I think you should
>>> see the warning with CONFIG_PREEMPT=y.
>>>
>
> Thanks a lot for the report and analysis.
>
> --->8--------
>  From ba2dd7aee12cb09b9ef159251859443e1b055669 Mon Sep 17 00:00:00 2001
> From: Wenyou Yang <wenyou.yang@atmel.com>
> Date: Mon, 10 Mar 2014 16:44:33 +0800
> Subject: [PATCH] spi: atmel: fix BUG: scheduling while atomic with
>   CONFIG_PREEMPT=y
>
> introduced by 8090d6d1a415d3ae1a7208995decfab8f60f4f36
> ("spi: atmel: Refactor spi-atmel to use SPI framework queue")
>
> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> ---
>   drivers/spi/spi-atmel.c |    4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
> index b0842f7..e05c3f2 100644
> --- a/drivers/spi/spi-atmel.c
> +++ b/drivers/spi/spi-atmel.c
> @@ -1130,8 +1130,12 @@ static int atmel_spi_one_transfer(struct spi_master *master,
>   			atmel_spi_next_xfer_pio(master, xfer);
>   		}
>
> +		atmel_spi_unlock(as);
> +
>   		ret = wait_for_completion_timeout(&as->xfer_completion,
>   							SPI_DMA_TIMEOUT);
> +		atmel_spi_lock(as);
> +
>   		if (WARN_ON(ret == 0)) {
>   			dev_err(&spi->dev,
>   				"spi trasfer timeout, err %d\n", ret);
>
Wenyou Yang - March 11, 2014, 9:54 a.m.
> -----Original Message-----

> From: Jiří Prchal [mailto:jiri.prchal@aksignal.cz]

> Sent: Tuesday, March 11, 2014 5:46 PM

> To: Yang, Wenyou; Rabin Vincent; Alexandre Belloni

> Cc: Ferre, Nicolas; linux-arm-kernel@lists.infradead.org;

> sboyd@codeaurora.org

> Subject: Re: [BUG] atmel: spi: scheduling while atomic

> 

> Hi,

> patch applied, it helps.

> Is this final solution?

> Thanks


Thank for your feedback.
There isn't a better solution so far.

Best Regards,
Wenyou Yang

> 

> Dne 10.3.2014 10:08, Yang, Wenyou napsal(a):

> >

> >

> >> -----Original Message-----

> >> From: Jiří Prchal [mailto:jiri.prchal@aksignal.cz]

> >> Sent: Monday, March 10, 2014 4:12 PM

> >> To: Rabin Vincent; Alexandre Belloni; Yang, Wenyou

> >> Cc: Ferre, Nicolas; linux-arm-kernel@lists.infradead.org;

> >> sboyd@codeaurora.org

> >> Subject: Re: [BUG] atmel: spi: scheduling while atomic

> >>

> >> Hi all,

> >> at first: warning is on all spi devices.

> >> at second: its with CONFIG_PREEMPT=y, without (# CONFIG_PREEMPT_RCU

> >> is not set) is OK, but I wonder if this config will be fast enough

> >> for our usage (nearly RT)?

> >>

> >>

> >> Dne 8.3.2014 01:41, Rabin Vincent napsal(a):

> >>> 2014-03-07 23:58 GMT+01:00 Alexandre Belloni

> >>> <alexandre.belloni@free-electrons.com>:

> >>>> On 05/03/2014 at 07:56:04 +0100, Jiří Prchal wrote :

> >>>>> [    0.902343] BUG: scheduling while atomic: spi0/383/0x00000002

> >>>>> [    0.906250] Modules linked in:

> >>>>> [    0.906250] CPU: 0 PID: 383 Comm: spi0 Not tainted 3.14.0-

> >> rc4_cpm9g25+ #1

> >>>>> [    0.906250] [<c000d9f4>] (unwind_backtrace) from [<c000bdc0>]

> >> (show_stack+0x10/0x14)

> >>>>> [    0.906250] [<c000bdc0>] (show_stack) from [<c003a224>]

> >> (__schedule_bug+0x48/0x60)

> >>>>> [    0.906250] [<c003a224>] (__schedule_bug) from [<c0390294>]

> >> (__schedule+0x60/0x484)

> >>>>> [    0.906250] [<c0390294>] (__schedule) from [<c038feac>]

> >> (schedule_timeout+0x17c/0x1ac)

> >>>>> [    0.906250] [<c038feac>] (schedule_timeout) from [<c039111c>]

> >> (wait_for_common+0x10c/0x1f0)

> >>>>> [    0.906250] [<c039111c>] (wait_for_common) from [<c0249228>]

> >> (atmel_spi_transfer_one_message+0x71c/0xa48)

> >>>>> [    0.906250] [<c0249228>] (atmel_spi_transfer_one_message) from

> >> [<c0246624>] (spi_pump_messages+0x210/0x238)

> >>>>> [    0.906250] [<c0246624>] (spi_pump_messages) from [<c00343d0>]

> >> (kthread_worker_fn+0x15c/0x1b4)

> >>>>> [    0.906250] [<c00343d0>] (kthread_worker_fn) from [<c0034534>]

> >> (kthread+0xb8/0xcc)

> >>>>> [    0.906250] [<c0034534>] (kthread) from [<c0009510>]

> >> (ret_from_fork+0x14/0x24)

> >>>>> [    0.906250] at25 spi0.0: 128 KByte at25 eeprom, pagesize 512

> >>>>

> >>>> Actually, I'm not quite sure how spi_pump_messages can be called

> >>>> from

> >> an

> >>>> atomic context.

> >>>

> >>> spi_pump_messages() is not called from atomic context.  Rather,

> >>> atmel_spi_transfer_one_message() does a spin_lock_irqsave() (via

> >>> atmel_spi_lock()) and then calls atmel_spi_one_transfer(), which

> >>> calls wait_for_completion_timeout().  That's why schedule is being

> >>> called from atomic context.  This was introduced by 8090d6d1a4 ("spi:

> atmel:

> >>> Refactor spi-atmel to use SPI framework queue"). I think you should

> >>> see the warning with CONFIG_PREEMPT=y.

> >>>

> >

> > Thanks a lot for the report and analysis.

> >

> > --->8--------

> >  From ba2dd7aee12cb09b9ef159251859443e1b055669 Mon Sep 17 00:00:00

> > 2001

> > From: Wenyou Yang <wenyou.yang@atmel.com>

> > Date: Mon, 10 Mar 2014 16:44:33 +0800

> > Subject: [PATCH] spi: atmel: fix BUG: scheduling while atomic with

> >   CONFIG_PREEMPT=y

> >

> > introduced by 8090d6d1a415d3ae1a7208995decfab8f60f4f36

> > ("spi: atmel: Refactor spi-atmel to use SPI framework queue")

> >

> > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>

> > ---

> >   drivers/spi/spi-atmel.c |    4 ++++

> >   1 file changed, 4 insertions(+)

> >

> > diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c index

> > b0842f7..e05c3f2 100644

> > --- a/drivers/spi/spi-atmel.c

> > +++ b/drivers/spi/spi-atmel.c

> > @@ -1130,8 +1130,12 @@ static int atmel_spi_one_transfer(struct

> spi_master *master,

> >   			atmel_spi_next_xfer_pio(master, xfer);

> >   		}

> >

> > +		atmel_spi_unlock(as);

> > +

> >   		ret = wait_for_completion_timeout(&as->xfer_completion,

> >   							SPI_DMA_TIMEOUT);

> > +		atmel_spi_lock(as);

> > +

> >   		if (WARN_ON(ret == 0)) {

> >   			dev_err(&spi->dev,

> >   				"spi trasfer timeout, err %d\n", ret);

> >
Prchal Jiří - March 11, 2014, 10:01 a.m.
So shouldn't we (you) apply it to mainstream?

Dne 11.3.2014 10:54, Yang, Wenyou napsal(a):
>
>> -----Original Message-----
>> From: Jiří Prchal [mailto:jiri.prchal@aksignal.cz]
>> Sent: Tuesday, March 11, 2014 5:46 PM
>> To: Yang, Wenyou; Rabin Vincent; Alexandre Belloni
>> Cc: Ferre, Nicolas; linux-arm-kernel@lists.infradead.org;
>> sboyd@codeaurora.org
>> Subject: Re: [BUG] atmel: spi: scheduling while atomic
>>
>> Hi,
>> patch applied, it helps.
>> Is this final solution?
>> Thanks
>
> Thank for your feedback.
> There isn't a better solution so far.
>
> Best Regards,
> Wenyou Yang
>
>>
>> Dne 10.3.2014 10:08, Yang, Wenyou napsal(a):
>>>
>>>
>>>> -----Original Message-----
>>>> From: Jiří Prchal [mailto:jiri.prchal@aksignal.cz]
>>>> Sent: Monday, March 10, 2014 4:12 PM
>>>> To: Rabin Vincent; Alexandre Belloni; Yang, Wenyou
>>>> Cc: Ferre, Nicolas; linux-arm-kernel@lists.infradead.org;
>>>> sboyd@codeaurora.org
>>>> Subject: Re: [BUG] atmel: spi: scheduling while atomic
>>>>
>>>> Hi all,
>>>> at first: warning is on all spi devices.
>>>> at second: its with CONFIG_PREEMPT=y, without (# CONFIG_PREEMPT_RCU
>>>> is not set) is OK, but I wonder if this config will be fast enough
>>>> for our usage (nearly RT)?
>>>>
>>>>
>>>> Dne 8.3.2014 01:41, Rabin Vincent napsal(a):
>>>>> 2014-03-07 23:58 GMT+01:00 Alexandre Belloni
>>>>> <alexandre.belloni@free-electrons.com>:
>>>>>> On 05/03/2014 at 07:56:04 +0100, Jiří Prchal wrote :
>>>>>>> [    0.902343] BUG: scheduling while atomic: spi0/383/0x00000002
>>>>>>> [    0.906250] Modules linked in:
>>>>>>> [    0.906250] CPU: 0 PID: 383 Comm: spi0 Not tainted 3.14.0-
>>>> rc4_cpm9g25+ #1
>>>>>>> [    0.906250] [<c000d9f4>] (unwind_backtrace) from [<c000bdc0>]
>>>> (show_stack+0x10/0x14)
>>>>>>> [    0.906250] [<c000bdc0>] (show_stack) from [<c003a224>]
>>>> (__schedule_bug+0x48/0x60)
>>>>>>> [    0.906250] [<c003a224>] (__schedule_bug) from [<c0390294>]
>>>> (__schedule+0x60/0x484)
>>>>>>> [    0.906250] [<c0390294>] (__schedule) from [<c038feac>]
>>>> (schedule_timeout+0x17c/0x1ac)
>>>>>>> [    0.906250] [<c038feac>] (schedule_timeout) from [<c039111c>]
>>>> (wait_for_common+0x10c/0x1f0)
>>>>>>> [    0.906250] [<c039111c>] (wait_for_common) from [<c0249228>]
>>>> (atmel_spi_transfer_one_message+0x71c/0xa48)
>>>>>>> [    0.906250] [<c0249228>] (atmel_spi_transfer_one_message) from
>>>> [<c0246624>] (spi_pump_messages+0x210/0x238)
>>>>>>> [    0.906250] [<c0246624>] (spi_pump_messages) from [<c00343d0>]
>>>> (kthread_worker_fn+0x15c/0x1b4)
>>>>>>> [    0.906250] [<c00343d0>] (kthread_worker_fn) from [<c0034534>]
>>>> (kthread+0xb8/0xcc)
>>>>>>> [    0.906250] [<c0034534>] (kthread) from [<c0009510>]
>>>> (ret_from_fork+0x14/0x24)
>>>>>>> [    0.906250] at25 spi0.0: 128 KByte at25 eeprom, pagesize 512
>>>>>>
>>>>>> Actually, I'm not quite sure how spi_pump_messages can be called
>>>>>> from
>>>> an
>>>>>> atomic context.
>>>>>
>>>>> spi_pump_messages() is not called from atomic context.  Rather,
>>>>> atmel_spi_transfer_one_message() does a spin_lock_irqsave() (via
>>>>> atmel_spi_lock()) and then calls atmel_spi_one_transfer(), which
>>>>> calls wait_for_completion_timeout().  That's why schedule is being
>>>>> called from atomic context.  This was introduced by 8090d6d1a4 ("spi:
>> atmel:
>>>>> Refactor spi-atmel to use SPI framework queue"). I think you should
>>>>> see the warning with CONFIG_PREEMPT=y.
>>>>>
>>>
>>> Thanks a lot for the report and analysis.
>>>
>>> --->8--------
>>>   From ba2dd7aee12cb09b9ef159251859443e1b055669 Mon Sep 17 00:00:00
>>> 2001
>>> From: Wenyou Yang <wenyou.yang@atmel.com>
>>> Date: Mon, 10 Mar 2014 16:44:33 +0800
>>> Subject: [PATCH] spi: atmel: fix BUG: scheduling while atomic with
>>>    CONFIG_PREEMPT=y
>>>
>>> introduced by 8090d6d1a415d3ae1a7208995decfab8f60f4f36
>>> ("spi: atmel: Refactor spi-atmel to use SPI framework queue")
>>>
>>> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
>>> ---
>>>    drivers/spi/spi-atmel.c |    4 ++++
>>>    1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c index
>>> b0842f7..e05c3f2 100644
>>> --- a/drivers/spi/spi-atmel.c
>>> +++ b/drivers/spi/spi-atmel.c
>>> @@ -1130,8 +1130,12 @@ static int atmel_spi_one_transfer(struct
>> spi_master *master,
>>>    			atmel_spi_next_xfer_pio(master, xfer);
>>>    		}
>>>
>>> +		atmel_spi_unlock(as);
>>> +
>>>    		ret = wait_for_completion_timeout(&as->xfer_completion,
>>>    							SPI_DMA_TIMEOUT);
>>> +		atmel_spi_lock(as);
>>> +
>>>    		if (WARN_ON(ret == 0)) {
>>>    			dev_err(&spi->dev,
>>>    				"spi trasfer timeout, err %d\n", ret);
>>>

Patch

diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c

index b0842f7..e05c3f2 100644

--- a/drivers/spi/spi-atmel.c

+++ b/drivers/spi/spi-atmel.c

@@ -1130,8 +1130,12 @@  static int atmel_spi_one_transfer(struct spi_master *master,

 			atmel_spi_next_xfer_pio(master, xfer);
 		}
 
+		atmel_spi_unlock(as);

+

 		ret = wait_for_completion_timeout(&as->xfer_completion,
 							SPI_DMA_TIMEOUT);
+		atmel_spi_lock(as);

+

 		if (WARN_ON(ret == 0)) {
 			dev_err(&spi->dev,
 				"spi trasfer timeout, err %d\n", ret);