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

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

[Bjorn Helgaas]

[+cc Rob, LKML for visibility & archive]

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@gmail.com> 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.

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

[Andy Shevchenko]

+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:
> 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@gmail.com> 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).

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.

-- 
With Best Regards,
Andy Shevchenko

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

[Marc Zyngier]

On Tue, 21 Jun 2022 15:00:35 +0100,
Andy Shevchenko <andriy.shevchenko@intel.com> 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@gmail.com> 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.

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

[Fabio Estevam]

Hi Marc,

On Tue, Jun 21, 2022 at 12:41 PM Marc Zyngier <maz@kernel.org> wrote:

> 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.

With your patch applied, the warning is gone, thanks.

The touchscreen is registered:

[    8.207461] input: mxs-lradc-ts as
/devices/soc0/80000000.apb/80040000.apbx/80050000.lradc/mxs-lradc-ts/input/input0

but when I run  "cat /dev/input/event0" and touch the screen, no irq
event is generated.

Looking at cat /proc/interrupts shows that no mxs-lradc-touchscreen
irq happened:

216:          0         -  10 Edge      mxs-lradc-touchscreen

Thanks

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

[Marc Zyngier]

Hi Fabio,

On 2022-06-21 17:23, Fabio Estevam wrote:
> Hi Marc,
> 
> On Tue, Jun 21, 2022 at 12:41 PM Marc Zyngier <maz@kernel.org> wrote:
> 
>> 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.
> 
> With your patch applied, the warning is gone, thanks.
> 
> The touchscreen is registered:
> 
> [    8.207461] input: mxs-lradc-ts as
> /devices/soc0/80000000.apb/80040000.apbx/80050000.lradc/mxs-lradc-ts/input/input0
> 
> but when I run  "cat /dev/input/event0" and touch the screen, no irq
> event is generated.
> 
> Looking at cat /proc/interrupts shows that no mxs-lradc-touchscreen
> irq happened:
> 
> 216:          0         -  10 Edge      mxs-lradc-touchscreen

I'm not sure I can help you further on that. '10' seems to be
the correct interrupt for the interrupt number (irq index 0 in
the lradc device).

You'll have to debug it further, I'm afraid.

         M.
-- 
Jazz is not dead. It just smells funny...

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

[Fabio Estevam]

Hi Marc,

On Tue, Jun 21, 2022 at 1:39 PM Marc Zyngier <maz@kernel.org> wrote:

> I'm not sure I can help you further on that. '10' seems to be
> the correct interrupt for the interrupt number (irq index 0 in
> the lradc device).

As your proposed changes improve things (no more irq 0 warning is seen and the
touchscreen irq is registered), please submit it formally, if you have a chance.

You can add:

Tested-by: Fabio Estevam <festevam@gmail.com>

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

[Andy Shevchenko]

On Tue, Jun 21, 2022 at 09:36:57PM -0300, Fabio Estevam wrote:
> Hi Marc,
> 
> On Tue, Jun 21, 2022 at 1:39 PM Marc Zyngier <maz@kernel.org> wrote:
> 
> > I'm not sure I can help you further on that. '10' seems to be
> > the correct interrupt for the interrupt number (irq index 0 in
> > the lradc device).
> 
> As your proposed changes improve things (no more irq 0 warning is seen and the
> touchscreen irq is registered), please submit it formally, if you have a chance.

Sorry, I haven't followed here, is there any progress on this? Or in other words
what the state of affairs with this driver?

> You can add:
> 
> Tested-by: Fabio Estevam <festevam@gmail.com>


-- 
With Best Regards,
Andy Shevchenko