diff mbox

[1/5] spi: dw: fix possible race condition

Message ID 20180717142314.32337-2-alexandre.belloni@bootlin.com (mailing list archive)
State Accepted
Commit 66b19d762378785d1568b5650935205edfeb0503
Headers show

Commit Message

Alexandre Belloni July 17, 2018, 2:23 p.m. UTC 
It is possible to get an interrupt as soon as it is requested.  dw_spi_irq
does spi_controller_get_devdata(master) and expects it to be different than
NULL. However, spi_controller_set_devdata() is called after request_irq(),
resulting in the following crash:

CPU 0 Unable to handle kernel paging request at virtual address 00000030, epc == 8058e09c, ra == 8018ff90
[...]
Call Trace:
[<8058e09c>] dw_spi_irq+0x8/0x64
[<8018ff90>] __handle_irq_event_percpu+0x70/0x1d4
[<80190128>] handle_irq_event_percpu+0x34/0x8c
[<801901c4>] handle_irq_event+0x44/0x80
[<801951a8>] handle_level_irq+0xdc/0x194
[<8018f580>] generic_handle_irq+0x38/0x50
[<804c6924>] ocelot_irq_handler+0x104/0x1c0
[<8018f580>] generic_handle_irq+0x38/0x50
[<8075c1d8>] do_IRQ+0x18/0x24
[<804c4714>] plat_irq_dispatch+0xa4/0x150
[<80106ba8>] except_vec_vi_end+0xb8/0xc4
[<8075ba5c>] _raw_spin_unlock_irqrestore+0x14/0x20
[<801926c8>] __setup_irq+0x53c/0x8e0
[<80192e28>] request_threaded_irq+0xf4/0x1e8
[<8058ed18>] dw_spi_add_host+0x264/0x2c4
[<8058f2ec>] dw_spi_mmio_probe+0x258/0x27c
[<8051f4a4>] platform_drv_probe+0x58/0xbc
[<8051daa8>] driver_probe_device+0x308/0x40c
[<8051dc9c>] __driver_attach+0xf0/0xf8
[<8051b698>] bus_for_each_dev+0x78/0xcc
[<8051c2c0>] bus_add_driver+0x1b4/0x228
[<8051e840>] driver_register+0x84/0x154
[<801001d8>] do_one_initcall+0x54/0x1ac
[<80880e90>] kernel_init_freeable+0x1ec/0x2ac
[<80755310>] kernel_init+0x14/0x110
[<80106698>] ret_from_kernel_thread+0x14/0x1c
Code: 00000000  8ca40050  8c820008 <8c420030> 0000000f  3042003f  10400007  00000000  8ca20230

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/spi/spi-dw.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Andy Shevchenko July 17, 2018, 9:30 p.m. UTC | #1 
On Tue, Jul 17, 2018 at 5:23 PM, Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
> It is possible to get an interrupt as soon as it is requested.  dw_spi_irq
> does spi_controller_get_devdata(master) and expects it to be different than
> NULL. However, spi_controller_set_devdata() is called after request_irq(),
> resulting in the following crash:
>
> CPU 0 Unable to handle kernel paging request at virtual address 00000030, epc == 8058e09c, ra == 8018ff90
> [...]
> Call Trace:
> [<8058e09c>] dw_spi_irq+0x8/0x64
> [<8018ff90>] __handle_irq_event_percpu+0x70/0x1d4
> [<80190128>] handle_irq_event_percpu+0x34/0x8c
> [<801901c4>] handle_irq_event+0x44/0x80
> [<801951a8>] handle_level_irq+0xdc/0x194
> [<8018f580>] generic_handle_irq+0x38/0x50
> [<804c6924>] ocelot_irq_handler+0x104/0x1c0
> [<8018f580>] generic_handle_irq+0x38/0x50
> [<8075c1d8>] do_IRQ+0x18/0x24
> [<804c4714>] plat_irq_dispatch+0xa4/0x150
> [<80106ba8>] except_vec_vi_end+0xb8/0xc4
> [<8075ba5c>] _raw_spin_unlock_irqrestore+0x14/0x20
> [<801926c8>] __setup_irq+0x53c/0x8e0
> [<80192e28>] request_threaded_irq+0xf4/0x1e8
> [<8058ed18>] dw_spi_add_host+0x264/0x2c4
> [<8058f2ec>] dw_spi_mmio_probe+0x258/0x27c
> [<8051f4a4>] platform_drv_probe+0x58/0xbc
> [<8051daa8>] driver_probe_device+0x308/0x40c
> [<8051dc9c>] __driver_attach+0xf0/0xf8
> [<8051b698>] bus_for_each_dev+0x78/0xcc
> [<8051c2c0>] bus_add_driver+0x1b4/0x228
> [<8051e840>] driver_register+0x84/0x154
> [<801001d8>] do_one_initcall+0x54/0x1ac
> [<80880e90>] kernel_init_freeable+0x1ec/0x2ac
> [<80755310>] kernel_init+0x14/0x110
> [<80106698>] ret_from_kernel_thread+0x14/0x1c
> Code: 00000000  8ca40050  8c820008 <8c420030> 0000000f  3042003f  10400007  00000000  8ca20230
>

Good catch!

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---
>  drivers/spi/spi-dw.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
> index f693bfe95ab9..a087464efdd7 100644
> --- a/drivers/spi/spi-dw.c
> +++ b/drivers/spi/spi-dw.c
> @@ -485,6 +485,8 @@ int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
>         dws->dma_inited = 0;
>         dws->dma_addr = (dma_addr_t)(dws->paddr + DW_SPI_DR);
>
> +       spi_controller_set_devdata(master, dws);
> +
>         ret = request_irq(dws->irq, dw_spi_irq, IRQF_SHARED, dev_name(dev),
>                           master);
>         if (ret < 0) {
> @@ -518,7 +520,6 @@ int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
>                 }
>         }
>
> -       spi_controller_set_devdata(master, dws);
>         ret = devm_spi_register_controller(dev, master);
>         if (ret) {
>                 dev_err(&master->dev, "problem registering spi master\n");
> --
> 2.18.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-spi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Shevchenko July 17, 2018, 9:30 p.m. UTC | #2 
On Wed, Jul 18, 2018 at 12:30 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Jul 17, 2018 at 5:23 PM, Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:

> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>
>> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

Shouldn't it have a Fixes tag?
Alexandre Belloni July 17, 2018, 9:42 p.m. UTC | #3 
On 18/07/2018 00:30:49+0300, Andy Shevchenko wrote:
> On Wed, Jul 18, 2018 at 12:30 AM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Tue, Jul 17, 2018 at 5:23 PM, Alexandre Belloni
> > <alexandre.belloni@bootlin.com> wrote:
> 
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> >
> >> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> 
> Shouldn't it have a Fixes tag?
> 

Well, I'm not sure how far this can be backported. It also seems nobody
ever hit that while our hardware will hit it at every boot.

I'll try to find out.
Andy Shevchenko July 17, 2018, 9:54 p.m. UTC | #4 
On Wed, Jul 18, 2018 at 12:42 AM, Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
> On 18/07/2018 00:30:49+0300, Andy Shevchenko wrote:
>> On Wed, Jul 18, 2018 at 12:30 AM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>> > On Tue, Jul 17, 2018 at 5:23 PM, Alexandre Belloni
>> > <alexandre.belloni@bootlin.com> wrote:
>>
>> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>> >
>> >> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
>>
>> Shouldn't it have a Fixes tag?
>>
>
> Well, I'm not sure how far this can be backported. It also seems nobody
> ever hit that while our hardware will hit it at every boot.
>
> I'll try to find out.

No-one enabled CONFIG_DEBUG_SHIRQ?
Alexandre Belloni July 17, 2018, 10:13 p.m. UTC | #5 
On 18/07/2018 00:54:21+0300, Andy Shevchenko wrote:
> On Wed, Jul 18, 2018 at 12:42 AM, Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> > On 18/07/2018 00:30:49+0300, Andy Shevchenko wrote:
> >> On Wed, Jul 18, 2018 at 12:30 AM, Andy Shevchenko
> >> <andy.shevchenko@gmail.com> wrote:
> >> > On Tue, Jul 17, 2018 at 5:23 PM, Alexandre Belloni
> >> > <alexandre.belloni@bootlin.com> wrote:
> >>
> >> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> >> >
> >> >> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> >>
> >> Shouldn't it have a Fixes tag?
> >>
> >
> > Well, I'm not sure how far this can be backported. It also seems nobody
> > ever hit that while our hardware will hit it at every boot.
> >
> > I'll try to find out.
> 
> No-one enabled CONFIG_DEBUG_SHIRQ?
> 

Nope, this is a real HW IRQ. I meant find out up to when it can be
sanely backported.
Andy Shevchenko July 18, 2018, 10:33 a.m. UTC | #6 
On Wed, Jul 18, 2018 at 1:13 AM, Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
> On 18/07/2018 00:54:21+0300, Andy Shevchenko wrote:
>> On Wed, Jul 18, 2018 at 12:42 AM, Alexandre Belloni
>> <alexandre.belloni@bootlin.com> wrote:
>> > On 18/07/2018 00:30:49+0300, Andy Shevchenko wrote:

>> > Well, I'm not sure how far this can be backported. It also seems nobody
>> > ever hit that while our hardware will hit it at every boot.

>> No-one enabled CONFIG_DEBUG_SHIRQ?

> Nope, this is a real HW IRQ. I meant find out up to when it can be
> sanely backported.

I meant that before your case no-one tested with that option enabled
which will behave like you describe (IRQ gets fired as soon as being
registered).
Mark Brown July 18, 2018, 10:51 a.m. UTC | #7 
On Tue, Jul 17, 2018 at 04:23:10PM +0200, Alexandre Belloni wrote:
> It is possible to get an interrupt as soon as it is requested.  dw_spi_irq
> does spi_controller_get_devdata(master) and expects it to be different than
> NULL. However, spi_controller_set_devdata() is called after request_irq(),
> resulting in the following crash:
> 
> CPU 0 Unable to handle kernel paging request at virtual address 00000030, epc == 8058e09c, ra == 8018ff90
> [...]
> Call Trace:
> [<8058e09c>] dw_spi_irq+0x8/0x64
> [<8018ff90>] __handle_irq_event_percpu+0x70/0x1d4
> [<80190128>] handle_irq_event_percpu+0x34/0x8c
> [<801901c4>] handle_irq_event+0x44/0x80
> [<801951a8>] handle_level_irq+0xdc/0x194
> [<8018f580>] generic_handle_irq+0x38/0x50
> [<804c6924>] ocelot_irq_handler+0x104/0x1c0
> [<8018f580>] generic_handle_irq+0x38/0x50

Please think hard before including complete backtraces in upstream
reports, they are very large and contain almost no useful information
relative to their size so often obscure the relevant content in your
message. If part of the backtrace is usefully illustrative then it's
usually better to pull out the relevant sections.
diff mbox

Patch

diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
index f693bfe95ab9..a087464efdd7 100644
--- a/drivers/spi/spi-dw.c
+++ b/drivers/spi/spi-dw.c
@@ -485,6 +485,8 @@  int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
 	dws->dma_inited = 0;
 	dws->dma_addr = (dma_addr_t)(dws->paddr + DW_SPI_DR);
 
+	spi_controller_set_devdata(master, dws);
+
 	ret = request_irq(dws->irq, dw_spi_irq, IRQF_SHARED, dev_name(dev),
 			  master);
 	if (ret < 0) {
@@ -518,7 +520,6 @@  int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
 		}
 	}
 
-	spi_controller_set_devdata(master, dws);
 	ret = devm_spi_register_controller(dev, master);
 	if (ret) {
 		dev_err(&master->dev, "problem registering spi master\n");