On Tue, 05 Jul 2022 14:52:43 +0100, Serge Semin <fancer.lancer@xxxxxxxxx> 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@xxxxxxxxx> > > Signed-off-by: Samuel Holland <samuel@xxxxxxxxxxxx> > > --- > > > > 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.