| Message ID | 20180717142314.32337-2-alexandre.belloni@bootlin.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 66b19d762378785d1568b5650935205edfeb0503 |
| Headers | show |
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
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?
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.
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?
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.
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).
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 --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");
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(-)