diff mbox

drm: imx: use GENERIC_IRQ_CHIP

Message ID 7367559.qEUUbIia3C@wuerfel (mailing list archive)
State New, archived
Headers show

Commit Message

Arnd Bergmann June 12, 2014, 2:05 p.m. UTC 
This driver defines its own irqchip using the generic chip
infrastructure, and hence needs the GENERIC_IRQ_CHIP Kconfig
symbol enabled, or get this build error:

drivers/built-in.o: In function `ipu_probe':
:(.text+0x49ea4c): undefined reference to `irq_generic_chip_ops'
:(.text+0x49ea5c): undefined reference to `irq_alloc_domain_generic_chips'
:(.text+0x49ea60): undefined reference to `irq_get_domain_generic_chip'
:(.text+0x49ea64): undefined reference to `irq_gc_ack_set_bit'
:(.text+0x49ea6c): undefined reference to `irq_gc_mask_clr_bit'
:(.text+0x49ea70): undefined reference to `irq_gc_mask_set_bit'

Cc: Dave Airlie <airlied@redhat.com>
Cc: dri-devel@lists.freedesktop.org
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Comments

Russell King - ARM Linux June 12, 2014, 2:23 p.m. UTC | #1 
On Thu, Jun 12, 2014 at 04:05:32PM +0200, Arnd Bergmann wrote:
> This driver defines its own irqchip using the generic chip
> infrastructure, and hence needs the GENERIC_IRQ_CHIP Kconfig
> symbol enabled, or get this build error:
> 
> drivers/built-in.o: In function `ipu_probe':
> :(.text+0x49ea4c): undefined reference to `irq_generic_chip_ops'
> :(.text+0x49ea5c): undefined reference to `irq_alloc_domain_generic_chips'
> :(.text+0x49ea60): undefined reference to `irq_get_domain_generic_chip'
> :(.text+0x49ea64): undefined reference to `irq_gc_ack_set_bit'
> :(.text+0x49ea6c): undefined reference to `irq_gc_mask_clr_bit'
> :(.text+0x49ea70): undefined reference to `irq_gc_mask_set_bit'

Let's take a step back, and ask the obvious question: is it reasonable
to make use if GENERIC_IRQ_CHIP in this driver?

Bear in mind that this is a platform driver (and so can be unbound), and
the IRQ domain stuff does not support tear-down.  This code contains this...

static void ipu_irq_exit(struct ipu_soc *ipu)
{
        int i, irq;

        irq_set_chained_handler(ipu->irq_err, NULL);
        irq_set_handler_data(ipu->irq_err, NULL);
        irq_set_chained_handler(ipu->irq_sync, NULL);
        irq_set_handler_data(ipu->irq_sync, NULL);

        /* TODO: remove irq_domain_generic_chips */

        for (i = 0; i < IPU_NUM_IRQS; i++) {
                irq = irq_linear_revmap(ipu->domain, i);
                if (irq)
                        irq_dispose_mapping(irq);
        }

        irq_domain_remove(ipu->domain);
}

which rather hints at it being more broken than just the above.

So, I think you're just papering over the symptom of a broken
implementation with your patch...
Arnd Bergmann June 12, 2014, 2:51 p.m. UTC | #2 
On Thursday 12 June 2014 15:23:54 Russell King - ARM Linux wrote:
> On Thu, Jun 12, 2014 at 04:05:32PM +0200, Arnd Bergmann wrote:
> > This driver defines its own irqchip using the generic chip
> > infrastructure, and hence needs the GENERIC_IRQ_CHIP Kconfig
> > symbol enabled, or get this build error:
> > 
> > drivers/built-in.o: In function `ipu_probe':
> > :(.text+0x49ea4c): undefined reference to `irq_generic_chip_ops'
> > :(.text+0x49ea5c): undefined reference to `irq_alloc_domain_generic_chips'
> > :(.text+0x49ea60): undefined reference to `irq_get_domain_generic_chip'
> > :(.text+0x49ea64): undefined reference to `irq_gc_ack_set_bit'
> > :(.text+0x49ea6c): undefined reference to `irq_gc_mask_clr_bit'
> > :(.text+0x49ea70): undefined reference to `irq_gc_mask_set_bit'
> 
> Let's take a step back, and ask the obvious question: is it reasonable
> to make use if GENERIC_IRQ_CHIP in this driver?

While I haven't looked at this driver in particular, I know that
Thomas Gleixner has been rather upset in the past when new irqchip
drivers got introduced that were reimplementing the generic irqchip
rather than using it.

You can argue over whether it should be an irqchip at all or not,
I don't know, and I simply had to assume that this part of the
code is ok.

> Bear in mind that this is a platform driver (and so can be unbound), and
> the IRQ domain stuff does not support tear-down.  This code contains this...
> 
> static void ipu_irq_exit(struct ipu_soc *ipu)
> {
>         int i, irq;
> 
>         irq_set_chained_handler(ipu->irq_err, NULL);
>         irq_set_handler_data(ipu->irq_err, NULL);
>         irq_set_chained_handler(ipu->irq_sync, NULL);
>         irq_set_handler_data(ipu->irq_sync, NULL);
> 
>         /* TODO: remove irq_domain_generic_chips */
> 
>         for (i = 0; i < IPU_NUM_IRQS; i++) {
>                 irq = irq_linear_revmap(ipu->domain, i);
>                 if (irq)
>                         irq_dispose_mapping(irq);
>         }
> 
>         irq_domain_remove(ipu->domain);
> }
> 
> which rather hints at it being more broken than just the above.
> 
> So, I think you're just papering over the symptom of a broken
> implementation with your patch...

This seems more like a second bug in a related part of the code
to me. Looking at other generic irqchip users, it seems like
the same bug exists in gpio-dwapb.c, gpio-ml-ioh.c, gpio-pch.c
and possibly others, which are all loadable modules using a
generic irqchip that can't be freed.

	Arnd
Russell King - ARM Linux June 12, 2014, 3:04 p.m. UTC | #3 
On Thu, Jun 12, 2014 at 04:51:26PM +0200, Arnd Bergmann wrote:
> On Thursday 12 June 2014 15:23:54 Russell King - ARM Linux wrote:
> > On Thu, Jun 12, 2014 at 04:05:32PM +0200, Arnd Bergmann wrote:
> > > This driver defines its own irqchip using the generic chip
> > > infrastructure, and hence needs the GENERIC_IRQ_CHIP Kconfig
> > > symbol enabled, or get this build error:
> > > 
> > > drivers/built-in.o: In function `ipu_probe':
> > > :(.text+0x49ea4c): undefined reference to `irq_generic_chip_ops'
> > > :(.text+0x49ea5c): undefined reference to `irq_alloc_domain_generic_chips'
> > > :(.text+0x49ea60): undefined reference to `irq_get_domain_generic_chip'
> > > :(.text+0x49ea64): undefined reference to `irq_gc_ack_set_bit'
> > > :(.text+0x49ea6c): undefined reference to `irq_gc_mask_clr_bit'
> > > :(.text+0x49ea70): undefined reference to `irq_gc_mask_set_bit'
> > 
> > Let's take a step back, and ask the obvious question: is it reasonable
> > to make use if GENERIC_IRQ_CHIP in this driver?
> 
> While I haven't looked at this driver in particular, I know that
> Thomas Gleixner has been rather upset in the past when new irqchip
> drivers got introduced that were reimplementing the generic irqchip
> rather than using it.
> 
> You can argue over whether it should be an irqchip at all or not,
> I don't know, and I simply had to assume that this part of the
> code is ok.

The question was more whether "peripheral" drivers should register their
own irqchips to split a single IRQ into multiple separate Linux IRQs.
We don't have PCI devices behaving like that... and I don't think we
should allow it as a general rule.

> This seems more like a second bug in a related part of the code
> to me. Looking at other generic irqchip users, it seems like
> the same bug exists in gpio-dwapb.c, gpio-ml-ioh.c, gpio-pch.c
> and possibly others, which are all loadable modules using a
> generic irqchip that can't be freed.

Generally, that means either (a) the subsystem being used does not
support the approach, or (b) the subsystem is being inappropriately
used.

In the case of (a), it means a discussion whether support for it
should be added.  If the answer to that is no, then we need these
drivers to become modules which can only be loaded _and_ drivers
which can never be unbound.

In the case of (b) it means that the real bug is that the driver is
making use of the subsystem (irqchip in this case) that it should not
be using.
Arnd Bergmann June 12, 2014, 3:30 p.m. UTC | #4 
On Thursday 12 June 2014 16:04:19 Russell King - ARM Linux wrote:
> On Thu, Jun 12, 2014 at 04:51:26PM +0200, Arnd Bergmann wrote:
> > On Thursday 12 June 2014 15:23:54 Russell King - ARM Linux wrote:
> > > On Thu, Jun 12, 2014 at 04:05:32PM +0200, Arnd Bergmann wrote:
> > > > This driver defines its own irqchip using the generic chip
> > > > infrastructure, and hence needs the GENERIC_IRQ_CHIP Kconfig
> > > > symbol enabled, or get this build error:
> > > > 
> > > > drivers/built-in.o: In function `ipu_probe':
> > > > :(.text+0x49ea4c): undefined reference to `irq_generic_chip_ops'
> > > > :(.text+0x49ea5c): undefined reference to `irq_alloc_domain_generic_chips'
> > > > :(.text+0x49ea60): undefined reference to `irq_get_domain_generic_chip'
> > > > :(.text+0x49ea64): undefined reference to `irq_gc_ack_set_bit'
> > > > :(.text+0x49ea6c): undefined reference to `irq_gc_mask_clr_bit'
> > > > :(.text+0x49ea70): undefined reference to `irq_gc_mask_set_bit'
> > > 
> > > Let's take a step back, and ask the obvious question: is it reasonable
> > > to make use if GENERIC_IRQ_CHIP in this driver?
> > 
> > While I haven't looked at this driver in particular, I know that
> > Thomas Gleixner has been rather upset in the past when new irqchip
> > drivers got introduced that were reimplementing the generic irqchip
> > rather than using it.
> > 
> > You can argue over whether it should be an irqchip at all or not,
> > I don't know, and I simply had to assume that this part of the
> > code is ok.
> 
> The question was more whether "peripheral" drivers should register their
> own irqchips to split a single IRQ into multiple separate Linux IRQs.
> We don't have PCI devices behaving like that... and I don't think we
> should allow it as a general rule.

There are two cases I can think of where it makes sense for a driver
to register an irqchip: gpio extenders and multi-function-device (mfd).
It's quite common for both to do this.

For the IPU, it can be seen as a form of MFD, since there are multiple
real drivers in other subsystems that are part of the IPU. It doesn't
have to be done this way, but it seems like a reasonable way to me.

For architecture independent drivers (i.e. most PCI drivers), we
can't do it like this because not all architectures support IRQ
domains.

> > This seems more like a second bug in a related part of the code
> > to me. Looking at other generic irqchip users, it seems like
> > the same bug exists in gpio-dwapb.c, gpio-ml-ioh.c, gpio-pch.c
> > and possibly others, which are all loadable modules using a
> > generic irqchip that can't be freed.
> 
> Generally, that means either (a) the subsystem being used does not
> support the approach, or (b) the subsystem is being inappropriately
> used.
> 
> In the case of (a), it means a discussion whether support for it
> should be added.  If the answer to that is no, then we need these
> drivers to become modules which can only be loaded _and_ drivers
> which can never be unbound.
> 
> In the case of (b) it means that the real bug is that the driver is
> making use of the subsystem (irqchip in this case) that it should not
> be using.

Yes, makes sense. I believe this is just a missing feature in
the generic irqchip code. We keep extending this drive when needed,


	Arnd
diff mbox

Patch

diff --git a/drivers/gpu/ipu-v3/Kconfig b/drivers/gpu/ipu-v3/Kconfig
index 2f228a2..01864a5 100644
--- a/drivers/gpu/ipu-v3/Kconfig
+++ b/drivers/gpu/ipu-v3/Kconfig
@@ -2,6 +2,7 @@  config IMX_IPUV3_CORE
 	tristate "IPUv3 core support"
 	depends on SOC_IMX5 || SOC_IMX6Q || SOC_IMX6SL || ARCH_MULTIPLATFORM
 	depends on RESET_CONTROLLER
+	select GENERIC_IRQ_CHIP
 	help
 	  Choose this if you have a i.MX5/6 system and want to use the Image
 	  Processing Unit. This option only enables IPU base support.