Re: mxs_lradc_ts: Warning due to "0 is an invalid IRQ number"

From: Marc Zyngier
Date: Tue Jun 21 2022 - 11:42:15 EST

On Tue, 21 Jun 2022 15:00:35 +0100,
Andy Shevchenko <andriy.shevchenko@xxxxxxxxx> wrote:
>
> +Cc: Marc in case I'm utterly wrong in my conclusion below.
>
> On Wed, Jul 01, 2020 at 05:41:45PM -0500, Bjorn Helgaas wrote:

Two year old thread, not bad! ;-)

> > On Wed, Jul 01, 2020 at 12:32:48PM -0300, Fabio Estevam wrote:
> > > On Tue, Jun 16, 2020 at 8:52 PM Fabio Estevam <festevam@xxxxxxxxx> wrote:
> > > > I am seeing the following warning on a imx28-evk running linux-next:
> > > >
> > > > [ 7.625012] ------------[ cut here ]------------
> > > > [ 7.630111] WARNING: CPU: 0 PID: 1 at drivers/base/platform.c:317
> > > > __platform_get_irq_byname+0x74/0x90
> > > > [ 7.639692] 0 is an invalid IRQ number
> > > > [ 7.643540] Modules linked in:
> > > > [ 7.646961] CPU: 0 PID: 1 Comm: swapper Not tainted
> > > > 5.8.0-rc1-next-20200616-dirty #92
> > > > [ 7.654896] Hardware name: Freescale MXS (Device Tree)
> > > > [ 7.660434] [<c00105ec>] (unwind_backtrace) from [<c000e070>]
> > > > (show_stack+0x10/0x14)
> > > > [ 7.668591] [<c000e070>] (show_stack) from [<c001aa90>] (__warn+0xe4/0x108)
> > > > [ 7.675941] [<c001aa90>] (__warn) from [<c001ab20>]
> > > > (warn_slowpath_fmt+0x6c/0xb8)
> > > > [ 7.683575] [<c001ab20>] (warn_slowpath_fmt) from [<c0491b44>]
> > > > (__platform_get_irq_byname+0x74/0x90)
> > > > [ 7.693101] [<c0491b44>] (__platform_get_irq_byname) from
> > > > [<c0491b70>] (platform_get_irq_byname+0x10/0x48)
> > > > [ 7.703154] [<c0491b70>] (platform_get_irq_byname) from
> > > > [<c056e234>] (mxs_lradc_ts_probe+0x190/0x384)
> > > > [ 7.712771] [<c056e234>] (mxs_lradc_ts_probe) from [<c0491850>]
> > > > (platform_drv_probe+0x48/0x98)
> > > > [ 7.722806] [<c0491850>] (platform_drv_probe) from [<c048f7f0>]
> > > > (really_probe+0x218/0x348)
> > > > [ 7.731530] [<c048f7f0>] (really_probe) from [<c048fa28>]
> > > > (driver_probe_device+0x58/0xb4)
> > > > [ 7.740189] [<c048fa28>] (driver_probe_device) from [<c048fc2c>]
> > > > (device_driver_attach+0x58/0x60)
> > > > [ 7.749471] [<c048fc2c>] (device_driver_attach) from [<c048fcb8>]
> > > > (__driver_attach+0x84/0xc0)
> > > > [ 7.758394] [<c048fcb8>] (__driver_attach) from [<c048db28>]
> > > > (bus_for_each_dev+0x70/0xb4)
> > > > [ 7.766977] [<c048db28>] (bus_for_each_dev) from [<c048eb24>]
> > > > (bus_add_driver+0x154/0x1e0)
> > > > [ 7.775385] [<c048eb24>] (bus_add_driver) from [<c0490774>]
> > > > (driver_register+0x74/0x108)
> > > > [ 7.783872] [<c0490774>] (driver_register) from [<c000a2fc>]
> > > > (do_one_initcall+0x68/0x268)
> > > > [ 7.792467] [<c000a2fc>] (do_one_initcall) from [<c0a00fa0>]
> > > > (kernel_init_freeable+0x160/0x1f4)
> > > > [ 7.801661] [<c0a00fa0>] (kernel_init_freeable) from [<c0720c58>]
> > > > (kernel_init+0x8/0xf4)
> > > > [ 7.810165] [<c0720c58>] (kernel_init) from [<c0008510>]
> > > > (ret_from_fork+0x14/0x24)
> > > > [ 7.818101] Exception stack(0xc748dfb0 to 0xc748dff8)
> > > > [ 7.823273] dfa0: 00000000
> > > > 00000000 00000000 00000000
> > > > [ 7.831815] dfc0: 00000000 00000000 00000000 00000000 00000000
> > > > 00000000 00000000 00000000
> > > > [ 7.840351] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
> > > > [ 7.847322] irq event stamp: 273303
> > > > [ 7.850940] hardirqs last enabled at (273311): [<c0066ed8>]
> > > > console_unlock+0x390/0x534
> > > > [ 7.859314] hardirqs last disabled at (273328): [<c0066b8c>]
> > > > console_unlock+0x44/0x534
> > > > [ 7.867605] softirqs last enabled at (273344): [<c00097fc>]
> > > > __do_softirq+0x2d4/0x450
> > > > [ 7.875817] softirqs last disabled at (273355): [<c001fe48>]
> > > > irq_exit+0x150/0x174
> > > > [ 7.883472] ---[ end trace ddb222ada5cbf5cd ]---
> > > > [ 7.900004] input: mxs-lradc-ts as
> > > > /devices/soc0/80000000.apb/80040000.apbx/80050000.lradc/mxs-lradc-ts/input/input0
> > > >
> > > > The touchscreen irq is defined as:
> > > >
> > > > enum mx28_lradc_irqs {
> > > > MX28_LRADC_TS_IRQ = 0,
> > > >
> > > > Shouldn't we retrieve the IRQ number from the device tree instead?
> >
> > mxs_lradc_ts_probe()
> > {
> > ...
> > irq = platform_get_irq_byname(pdev, mxs_lradc_ts_irq_names[i]);
> > if (irq < 0)
> > return irq;
> >
> > virq = irq_of_parse_and_map(node, irq);
> > ...
> > }
> >
> > That's not right, is it? irq_of_parse_and_map() takes an *index*, but
> > we're passing an IRQ.
> >
> > mxs_lradc_adc_probe() also has the same pattern.
>
> Yeah, you are right. This code is broken by design. But the issue is that MFD
> driver supplies _index_ and not vIRQ from proper IRQ domain. I dunno how it's
> supposed to work without dirty tricks in the GIC (or whatever interrupt
> controller is there).

Not even close. This is totally broken, irrespective of the interrupt
controller. platform_get_irq_byname() does the mapping already,
mapping things *twice* is unlikely to lead to something useful.

The core of the issue is that there seem to be a bunch of MFD using
hardcoded resources (board-file style), and they randomly feed this
into the DT code.

> Since there is no response to this thread by the authors of the code, I would
> mark it as BROKEN and perhaps remove from the kernel if no-one steps in to fix
> this mess.

Something like the hack below could potentially make things less
broken, but I'm not holding my breath. If nobody cares, let's remove
the code altogether.

M.

diff --git a/drivers/input/touchscreen/mxs-lradc-ts.c b/drivers/input/touchscreen/mxs-lradc-ts.c
index 9e36fee38d61..93f21f00388f 100644
--- a/drivers/input/touchscreen/mxs-lradc-ts.c
+++ b/drivers/input/touchscreen/mxs-lradc-ts.c
@@ -606,7 +606,7 @@ static int mxs_lradc_ts_probe(struct platform_device *pdev)
struct device_node *node = dev->parent->of_node;
struct mxs_lradc *lradc = dev_get_drvdata(dev->parent);
struct mxs_lradc_ts *ts;
- int ret, irq, virq, i;
+ int ret, virq, i;
u32 ts_wires = 0, adapt;

ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
@@ -671,11 +671,16 @@ static int mxs_lradc_ts_probe(struct platform_device *pdev)
mxs_lradc_ts_hw_init(ts);

for (i = 0; i < 3; i++) {
- irq = platform_get_irq_byname(pdev, mxs_lradc_ts_irq_names[i]);
- if (irq < 0)
- return irq;
+ struct resource *r;

- virq = irq_of_parse_and_map(node, irq);
+ r = platform_get_resource_byname(pdev, IORESOURCE_IRQ,
+ mxs_lradc_ts_irq_names[i]);
+ if (!r)
+ return -ENXIO;
+
+ virq = irq_of_parse_and_map(node, r->start);
+ if (!virq)
+ return -ENXIO;

mxs_lradc_ts_stop(ts);


--
Without deviation from the norm, progress is not possible.