Re: [PATCH v3 1/8] irqchip/mips-gic: Only register IPI domain when SMP is enabled
On Thu, Jul 07, 2022 at 09:22:26AM +0100, Marc Zyngier wrote:
> On Tue, 05 Jul 2022 14:52:43 +0100,
> Serge Semin 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
> > > Signed-off-by: Samuel Holland
> > > ---
> > >
> > > 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.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/8] irqchip/mips-gic: Only register IPI domain when SMP is enabled
On Tue, 05 Jul 2022 14:52:43 +0100,
Serge Semin 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
> > Signed-off-by: Samuel Holland
> > ---
> >
> > 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.
--
Without deviation from the norm, progress is not possible.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/8] irqchip/mips-gic: Only register IPI domain when SMP is enabled
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
> Signed-off-by: Samuel Holland
> ---
>
> 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,
