| Message ID | 20220701200056.46555-2-samuel@sholland.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | genirq: Provide real IRQ affinity masks in non-SMP configs | expand |
Hi Samuel On Fri, Jul 01, 2022 at 03:00:49PM -0500, Samuel Holland wrote: > The MIPS GIC irqchip driver may be selected in a uniprocessor > configuration, but it unconditionally registers an IPI domain. > > Limit the part of the driver dealing with IPIs to only be compiled when > GENERIC_IRQ_IPI is enabled, which corresponds to an SMP configuration. Thanks for the patch. Some comment is below. > > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Samuel Holland <samuel@sholland.org> > --- > > Changes in v3: > - New patch to fix build errors in uniprocessor MIPS configs > > drivers/irqchip/Kconfig | 3 +- > drivers/irqchip/irq-mips-gic.c | 80 +++++++++++++++++++++++----------- > 2 files changed, 56 insertions(+), 27 deletions(-) > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > index 1f23a6be7d88..d26a4ff7c99f 100644 > --- a/drivers/irqchip/Kconfig > +++ b/drivers/irqchip/Kconfig > @@ -322,7 +322,8 @@ config KEYSTONE_IRQ > > config MIPS_GIC > bool > - select GENERIC_IRQ_IPI > + select GENERIC_IRQ_IPI if SMP > + select IRQ_DOMAIN_HIERARCHY It seems to me that the IRQ domains hierarchy is supposed to be created only if IPI is required. At least that's what the MIPS GIC driver implies. Thus we can go further and CONFIG_IRQ_DOMAIN_HIERARCHY ifdef-out the gic_irq_domain_alloc() and gic_irq_domain_free() methods definition together with the initialization: static const struct irq_domain_ops gic_irq_domain_ops = { .xlate = gic_irq_domain_xlate, +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY .alloc = gic_irq_domain_alloc, .free = gic_irq_domain_free, +#endif .map = gic_irq_domain_map, }; If the GENERIC_IRQ_IPI config is enabled, CONFIG_IRQ_DOMAIN_HIERARCHY will be automatically selected (see the config definition in kernel/irq/Kconfig). If the IRQs hierarchy is needed for some another functionality like GENERIC_MSI_IRQ_DOMAIN or GPIOs then they will explicitly enable the IRQ_DOMAIN_HIERARCHY config thus activating the denoted .alloc and .free methods definitions. To sum up you can get rid of the IRQ_DOMAIN_HIERARCHY config force-select from this patch and make the MIPS GIC driver code a bit more coherent. @Marc, please correct me if were wrong. -Serget > select MIPS_CM > > config INGENIC_IRQ > diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c > index ff89b36267dd..8a9efb6ae587 100644 > --- a/drivers/irqchip/irq-mips-gic.c > +++ b/drivers/irqchip/irq-mips-gic.c > @@ -52,13 +52,15 @@ static DEFINE_PER_CPU_READ_MOSTLY(unsigned long[GIC_MAX_LONGS], pcpu_masks); > > static DEFINE_SPINLOCK(gic_lock); > static struct irq_domain *gic_irq_domain; > -static struct irq_domain *gic_ipi_domain; > static int gic_shared_intrs; > static unsigned int gic_cpu_pin; > static unsigned int timer_cpu_pin; > static struct irq_chip gic_level_irq_controller, gic_edge_irq_controller; > + > +#ifdef CONFIG_GENERIC_IRQ_IPI > static DECLARE_BITMAP(ipi_resrv, GIC_MAX_INTRS); > static DECLARE_BITMAP(ipi_available, GIC_MAX_INTRS); > +#endif /* CONFIG_GENERIC_IRQ_IPI */ > > static struct gic_all_vpes_chip_data { > u32 map; > @@ -472,9 +474,11 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int virq, > u32 map; > > if (hwirq >= GIC_SHARED_HWIRQ_BASE) { > +#ifdef CONFIG_GENERIC_IRQ_IPI > /* verify that shared irqs don't conflict with an IPI irq */ > if (test_bit(GIC_HWIRQ_TO_SHARED(hwirq), ipi_resrv)) > return -EBUSY; > +#endif /* CONFIG_GENERIC_IRQ_IPI */ > > err = irq_domain_set_hwirq_and_chip(d, virq, hwirq, > &gic_level_irq_controller, > @@ -567,6 +571,8 @@ static const struct irq_domain_ops gic_irq_domain_ops = { > .map = gic_irq_domain_map, > }; > > +#ifdef CONFIG_GENERIC_IRQ_IPI > + > static int gic_ipi_domain_xlate(struct irq_domain *d, struct device_node *ctrlr, > const u32 *intspec, unsigned int intsize, > irq_hw_number_t *out_hwirq, > @@ -670,6 +676,48 @@ static const struct irq_domain_ops gic_ipi_domain_ops = { > .match = gic_ipi_domain_match, > }; > > +static int gic_register_ipi_domain(struct device_node *node) > +{ > + struct irq_domain *gic_ipi_domain; > + unsigned int v[2], num_ipis; > + > + gic_ipi_domain = irq_domain_add_hierarchy(gic_irq_domain, > + IRQ_DOMAIN_FLAG_IPI_PER_CPU, > + GIC_NUM_LOCAL_INTRS + gic_shared_intrs, > + node, &gic_ipi_domain_ops, NULL); > + if (!gic_ipi_domain) { > + pr_err("Failed to add IPI domain"); > + return -ENXIO; > + } > + > + irq_domain_update_bus_token(gic_ipi_domain, DOMAIN_BUS_IPI); > + > + if (node && > + !of_property_read_u32_array(node, "mti,reserved-ipi-vectors", v, 2)) { > + bitmap_set(ipi_resrv, v[0], v[1]); > + } else { > + /* > + * Reserve 2 interrupts per possible CPU/VP for use as IPIs, > + * meeting the requirements of arch/mips SMP. > + */ > + num_ipis = 2 * num_possible_cpus(); > + bitmap_set(ipi_resrv, gic_shared_intrs - num_ipis, num_ipis); > + } > + > + bitmap_copy(ipi_available, ipi_resrv, GIC_MAX_INTRS); > + > + return 0; > +} > + > +#else /* !CONFIG_GENERIC_IRQ_IPI */ > + > +static inline int gic_register_ipi_domain(struct device_node *node) > +{ > + return 0; > +} > + > +#endif /* !CONFIG_GENERIC_IRQ_IPI */ > + > static int gic_cpu_startup(unsigned int cpu) > { > /* Enable or disable EIC */ > @@ -688,11 +736,12 @@ static int gic_cpu_startup(unsigned int cpu) > static int __init gic_of_init(struct device_node *node, > struct device_node *parent) > { > - unsigned int cpu_vec, i, gicconfig, v[2], num_ipis; > + unsigned int cpu_vec, i, gicconfig; > unsigned long reserved; > phys_addr_t gic_base; > struct resource res; > size_t gic_len; > + int ret; > > /* Find the first available CPU vector. */ > i = 0; > @@ -780,30 +829,9 @@ static int __init gic_of_init(struct device_node *node, > return -ENXIO; > } > > - gic_ipi_domain = irq_domain_add_hierarchy(gic_irq_domain, > - IRQ_DOMAIN_FLAG_IPI_PER_CPU, > - GIC_NUM_LOCAL_INTRS + gic_shared_intrs, > - node, &gic_ipi_domain_ops, NULL); > - if (!gic_ipi_domain) { > - pr_err("Failed to add IPI domain"); > - return -ENXIO; > - } > - > - irq_domain_update_bus_token(gic_ipi_domain, DOMAIN_BUS_IPI); > - > - if (node && > - !of_property_read_u32_array(node, "mti,reserved-ipi-vectors", v, 2)) { > - bitmap_set(ipi_resrv, v[0], v[1]); > - } else { > - /* > - * Reserve 2 interrupts per possible CPU/VP for use as IPIs, > - * meeting the requirements of arch/mips SMP. > - */ > - num_ipis = 2 * num_possible_cpus(); > - bitmap_set(ipi_resrv, gic_shared_intrs - num_ipis, num_ipis); > - } > - > - bitmap_copy(ipi_available, ipi_resrv, GIC_MAX_INTRS); > + ret = gic_register_ipi_domain(node); > + if (ret) > + return ret; > > board_bind_eic_interrupt = &gic_bind_eic_interrupt; > > -- > 2.35.1 >
On Tue, 05 Jul 2022 14:52:43 +0100, Serge Semin <fancer.lancer@gmail.com> wrote: > > Hi Samuel > > On Fri, Jul 01, 2022 at 03:00:49PM -0500, Samuel Holland wrote: > > The MIPS GIC irqchip driver may be selected in a uniprocessor > > configuration, but it unconditionally registers an IPI domain. > > > > Limit the part of the driver dealing with IPIs to only be compiled when > > GENERIC_IRQ_IPI is enabled, which corresponds to an SMP configuration. > > Thanks for the patch. Some comment is below. > > > > > Reported-by: kernel test robot <lkp@intel.com> > > Signed-off-by: Samuel Holland <samuel@sholland.org> > > --- > > > > Changes in v3: > > - New patch to fix build errors in uniprocessor MIPS configs > > > > drivers/irqchip/Kconfig | 3 +- > > drivers/irqchip/irq-mips-gic.c | 80 +++++++++++++++++++++++----------- > > 2 files changed, 56 insertions(+), 27 deletions(-) > > > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > > index 1f23a6be7d88..d26a4ff7c99f 100644 > > --- a/drivers/irqchip/Kconfig > > +++ b/drivers/irqchip/Kconfig > > @@ -322,7 +322,8 @@ config KEYSTONE_IRQ > > > > config MIPS_GIC > > bool > > - select GENERIC_IRQ_IPI > > + select GENERIC_IRQ_IPI if SMP > > > + select IRQ_DOMAIN_HIERARCHY > > It seems to me that the IRQ domains hierarchy is supposed to be > created only if IPI is required. At least that's what the MIPS GIC > driver implies. Thus we can go further and CONFIG_IRQ_DOMAIN_HIERARCHY > ifdef-out the gic_irq_domain_alloc() and gic_irq_domain_free() > methods definition together with the initialization: > > static const struct irq_domain_ops gic_irq_domain_ops = { > .xlate = gic_irq_domain_xlate, > +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY > .alloc = gic_irq_domain_alloc, > .free = gic_irq_domain_free, > +#endif > .map = gic_irq_domain_map, > }; > > If the GENERIC_IRQ_IPI config is enabled, CONFIG_IRQ_DOMAIN_HIERARCHY > will be automatically selected (see the config definition in > kernel/irq/Kconfig). If the IRQs hierarchy is needed for some another > functionality like GENERIC_MSI_IRQ_DOMAIN or GPIOs then they will > explicitly enable the IRQ_DOMAIN_HIERARCHY config thus activating the > denoted .alloc and .free methods definitions. > > To sum up you can get rid of the IRQ_DOMAIN_HIERARCHY config > force-select from this patch and make the MIPS GIC driver code a bit > more coherent. > > @Marc, please correct me if were wrong. Either way probably works correctly, but Samuel's approach is more readable IMO. It is far easier to reason about a high-level feature (GENERIC_IRQ_IPI) than an implementation detail (IRQ_DOMAIN_HIERARCHY). If you really want to save a handful of bytes, you can make the callbacks conditional on GENERIC_IRQ_IPI, and be done with it. But this can come as an additional patch. Thanks, M.
On Thu, Jul 07, 2022 at 09:22:26AM +0100, Marc Zyngier wrote: > On Tue, 05 Jul 2022 14:52:43 +0100, > Serge Semin <fancer.lancer@gmail.com> wrote: > > > > Hi Samuel > > > > On Fri, Jul 01, 2022 at 03:00:49PM -0500, Samuel Holland wrote: > > > The MIPS GIC irqchip driver may be selected in a uniprocessor > > > configuration, but it unconditionally registers an IPI domain. > > > > > > Limit the part of the driver dealing with IPIs to only be compiled when > > > GENERIC_IRQ_IPI is enabled, which corresponds to an SMP configuration. > > > > Thanks for the patch. Some comment is below. > > > > > > > > Reported-by: kernel test robot <lkp@intel.com> > > > Signed-off-by: Samuel Holland <samuel@sholland.org> > > > --- > > > > > > Changes in v3: > > > - New patch to fix build errors in uniprocessor MIPS configs > > > > > > drivers/irqchip/Kconfig | 3 +- > > > drivers/irqchip/irq-mips-gic.c | 80 +++++++++++++++++++++++----------- > > > 2 files changed, 56 insertions(+), 27 deletions(-) > > > > > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > > > index 1f23a6be7d88..d26a4ff7c99f 100644 > > > --- a/drivers/irqchip/Kconfig > > > +++ b/drivers/irqchip/Kconfig > > > @@ -322,7 +322,8 @@ config KEYSTONE_IRQ > > > > > > config MIPS_GIC > > > bool > > > - select GENERIC_IRQ_IPI > > > + select GENERIC_IRQ_IPI if SMP > > > > > + select IRQ_DOMAIN_HIERARCHY > > > > It seems to me that the IRQ domains hierarchy is supposed to be > > created only if IPI is required. At least that's what the MIPS GIC > > driver implies. Thus we can go further and CONFIG_IRQ_DOMAIN_HIERARCHY > > ifdef-out the gic_irq_domain_alloc() and gic_irq_domain_free() > > methods definition together with the initialization: > > > > static const struct irq_domain_ops gic_irq_domain_ops = { > > .xlate = gic_irq_domain_xlate, > > +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY > > .alloc = gic_irq_domain_alloc, > > .free = gic_irq_domain_free, > > +#endif > > .map = gic_irq_domain_map, > > }; > > > > If the GENERIC_IRQ_IPI config is enabled, CONFIG_IRQ_DOMAIN_HIERARCHY > > will be automatically selected (see the config definition in > > kernel/irq/Kconfig). If the IRQs hierarchy is needed for some another > > functionality like GENERIC_MSI_IRQ_DOMAIN or GPIOs then they will > > explicitly enable the IRQ_DOMAIN_HIERARCHY config thus activating the > > denoted .alloc and .free methods definitions. > > > > To sum up you can get rid of the IRQ_DOMAIN_HIERARCHY config > > force-select from this patch and make the MIPS GIC driver code a bit > > more coherent. > > > > @Marc, please correct me if were wrong. > > Either way probably works correctly, but Samuel's approach is more > readable IMO. It is far easier to reason about a high-level feature > (GENERIC_IRQ_IPI) than an implementation detail (IRQ_DOMAIN_HIERARCHY). > The main idea of my comment was to get rid of the forcible IRQ_DOMAIN_HIERARCHY config selection, because the basic part of the driver doesn't depends on the hierarchical IRQ-domains functionality. It's needed only for IPIs and implicitly for the lower level IRQ device drivers like GPIO or PCIe-controllers, which explicitly enable the IRQ_DOMAIN_HIERARCHY config anyway. That's why instead of forcible IRQ_DOMAIN_HIERARCHY config selection (see Samuel patch) I suggested to make the corresponding functionality defined under the IRQ_DOMAIN_HIERARCHY config ifdefs, thus having the driver capable of creating the hierarchical IRQs domains only if it's required. > If you really want to save a handful of bytes, you can make the > callbacks conditional on GENERIC_IRQ_IPI, and be done with it. AFAIU I can't in this case. It must be either IRQ_DOMAIN_HIERARCHY ifdefs or explicit IRQ_DOMAIN_HIERARCHY select. There can be non-SMP (UP) systems with no need in IPIs but for instance having a GPIO or PCIe controller which require the hierarchical IRQ-domains support of the parental IRQ controller. So making the callbacks definition depended on the GENERIC_IRQ_IPI config state will break the driver for these systems. That's why I suggested to use CONFIG_IRQ_DOMAIN_HIERARCHY which activates the hierarchical IRQ domains support in the IRQ-chip system (see the irq_domain_ops structure conditional fields definition) and shall we have the suggested approach implemented in the MIPS GIC driver. -Sergey > But this can come as an additional patch. > > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible.
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index 1f23a6be7d88..d26a4ff7c99f 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -322,7 +322,8 @@ config KEYSTONE_IRQ config MIPS_GIC bool - select GENERIC_IRQ_IPI + select GENERIC_IRQ_IPI if SMP + select IRQ_DOMAIN_HIERARCHY select MIPS_CM config INGENIC_IRQ diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c index ff89b36267dd..8a9efb6ae587 100644 --- a/drivers/irqchip/irq-mips-gic.c +++ b/drivers/irqchip/irq-mips-gic.c @@ -52,13 +52,15 @@ static DEFINE_PER_CPU_READ_MOSTLY(unsigned long[GIC_MAX_LONGS], pcpu_masks); static DEFINE_SPINLOCK(gic_lock); static struct irq_domain *gic_irq_domain; -static struct irq_domain *gic_ipi_domain; static int gic_shared_intrs; static unsigned int gic_cpu_pin; static unsigned int timer_cpu_pin; static struct irq_chip gic_level_irq_controller, gic_edge_irq_controller; + +#ifdef CONFIG_GENERIC_IRQ_IPI static DECLARE_BITMAP(ipi_resrv, GIC_MAX_INTRS); static DECLARE_BITMAP(ipi_available, GIC_MAX_INTRS); +#endif /* CONFIG_GENERIC_IRQ_IPI */ static struct gic_all_vpes_chip_data { u32 map; @@ -472,9 +474,11 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int virq, u32 map; if (hwirq >= GIC_SHARED_HWIRQ_BASE) { +#ifdef CONFIG_GENERIC_IRQ_IPI /* verify that shared irqs don't conflict with an IPI irq */ if (test_bit(GIC_HWIRQ_TO_SHARED(hwirq), ipi_resrv)) return -EBUSY; +#endif /* CONFIG_GENERIC_IRQ_IPI */ err = irq_domain_set_hwirq_and_chip(d, virq, hwirq, &gic_level_irq_controller, @@ -567,6 +571,8 @@ static const struct irq_domain_ops gic_irq_domain_ops = { .map = gic_irq_domain_map, }; +#ifdef CONFIG_GENERIC_IRQ_IPI + static int gic_ipi_domain_xlate(struct irq_domain *d, struct device_node *ctrlr, const u32 *intspec, unsigned int intsize, irq_hw_number_t *out_hwirq, @@ -670,6 +676,48 @@ static const struct irq_domain_ops gic_ipi_domain_ops = { .match = gic_ipi_domain_match, }; +static int gic_register_ipi_domain(struct device_node *node) +{ + struct irq_domain *gic_ipi_domain; + unsigned int v[2], num_ipis; + + gic_ipi_domain = irq_domain_add_hierarchy(gic_irq_domain, + IRQ_DOMAIN_FLAG_IPI_PER_CPU, + GIC_NUM_LOCAL_INTRS + gic_shared_intrs, + node, &gic_ipi_domain_ops, NULL); + if (!gic_ipi_domain) { + pr_err("Failed to add IPI domain"); + return -ENXIO; + } + + irq_domain_update_bus_token(gic_ipi_domain, DOMAIN_BUS_IPI); + + if (node && + !of_property_read_u32_array(node, "mti,reserved-ipi-vectors", v, 2)) { + bitmap_set(ipi_resrv, v[0], v[1]); + } else { + /* + * Reserve 2 interrupts per possible CPU/VP for use as IPIs, + * meeting the requirements of arch/mips SMP. + */ + num_ipis = 2 * num_possible_cpus(); + bitmap_set(ipi_resrv, gic_shared_intrs - num_ipis, num_ipis); + } + + bitmap_copy(ipi_available, ipi_resrv, GIC_MAX_INTRS); + + return 0; +} + +#else /* !CONFIG_GENERIC_IRQ_IPI */ + +static inline int gic_register_ipi_domain(struct device_node *node) +{ + return 0; +} + +#endif /* !CONFIG_GENERIC_IRQ_IPI */ + static int gic_cpu_startup(unsigned int cpu) { /* Enable or disable EIC */ @@ -688,11 +736,12 @@ static int gic_cpu_startup(unsigned int cpu) static int __init gic_of_init(struct device_node *node, struct device_node *parent) { - unsigned int cpu_vec, i, gicconfig, v[2], num_ipis; + unsigned int cpu_vec, i, gicconfig; unsigned long reserved; phys_addr_t gic_base; struct resource res; size_t gic_len; + int ret; /* Find the first available CPU vector. */ i = 0; @@ -780,30 +829,9 @@ static int __init gic_of_init(struct device_node *node, return -ENXIO; } - gic_ipi_domain = irq_domain_add_hierarchy(gic_irq_domain, - IRQ_DOMAIN_FLAG_IPI_PER_CPU, - GIC_NUM_LOCAL_INTRS + gic_shared_intrs, - node, &gic_ipi_domain_ops, NULL); - if (!gic_ipi_domain) { - pr_err("Failed to add IPI domain"); - return -ENXIO; - } - - irq_domain_update_bus_token(gic_ipi_domain, DOMAIN_BUS_IPI); - - if (node && - !of_property_read_u32_array(node, "mti,reserved-ipi-vectors", v, 2)) { - bitmap_set(ipi_resrv, v[0], v[1]); - } else { - /* - * Reserve 2 interrupts per possible CPU/VP for use as IPIs, - * meeting the requirements of arch/mips SMP. - */ - num_ipis = 2 * num_possible_cpus(); - bitmap_set(ipi_resrv, gic_shared_intrs - num_ipis, num_ipis); - } - - bitmap_copy(ipi_available, ipi_resrv, GIC_MAX_INTRS); + ret = gic_register_ipi_domain(node); + if (ret) + return ret; board_bind_eic_interrupt = &gic_bind_eic_interrupt;
The MIPS GIC irqchip driver may be selected in a uniprocessor configuration, but it unconditionally registers an IPI domain. Limit the part of the driver dealing with IPIs to only be compiled when GENERIC_IRQ_IPI is enabled, which corresponds to an SMP configuration. Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: Samuel Holland <samuel@sholland.org> --- Changes in v3: - New patch to fix build errors in uniprocessor MIPS configs drivers/irqchip/Kconfig | 3 +- drivers/irqchip/irq-mips-gic.c | 80 +++++++++++++++++++++++----------- 2 files changed, 56 insertions(+), 27 deletions(-)