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
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.
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...
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
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.
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
dri-devel@lists.freedesktop.org
-
Arnd Bergmann -
Russell King - ARM Linux