drivers/extcon/Kconfig | 3 ++- drivers/of/Kconfig | 3 ++- drivers/rtc/Kconfig | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-)
IRQ_DOMAIN is a hidden (not user visible) symbol. Users cannot set it directly thru "make *config", so drivers should select it instead of depending on it if they need it. Relying on it being set for a dependency is risky. Consistently using "select" or "depends on" can also help reduce Kconfig circular dependency issues. IRQ_DOMAIN is selected 109 times and is depended on 3 times in current linux-next. Eliminate the uses of "depends on" by converting them to "select". [PATCH 1/3] extcon: max8997: select IRQ_DOMAIN instead of depending on it [PATCH 2/3] of: OF_IRQ: select IRQ_DOMAIN instead of depending on it [PATCH 3/3] rtc: mt6397: select IRQ_DOMAIN instead of depending on it diffstat: drivers/extcon/Kconfig | 3 ++- drivers/of/Kconfig | 3 ++- drivers/rtc/Kconfig | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-) Cc: Arnd Bergmann <arnd@arndb.de> Cc: MyungJoo Ham <myungjoo.ham@samsung.com> Cc: Chanwoo Choi <cw00.choi@samsung.com> Cc: Donggeun Kim <dg77.kim@samsung.com> Cc: Marc Zyngier <maz@kernel.org> Cc: Philipp Zabel <p.zabel@pengutronix.de> Cc: Peter Rosin <peda@axentia.se> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Geert Uytterhoeven <geert@linux-m68k.org> Cc: Rob Herring <robh@kernel.org> Cc: Eddie Huang <eddie.huang@mediatek.com> Cc: Sean Wang <sean.wang@mediatek.com> Cc: Matthias Brugger <matthias.bgg@gmail.com> Cc: Alessandro Zummo <a.zummo@towertech.it> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com> Cc: linux-rtc@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-mediatek@lists.infradead.org
On Mon, Feb 13, 2023, at 05:15, Randy Dunlap wrote:
> IRQ_DOMAIN is a hidden (not user visible) symbol. Users cannot set
> it directly thru "make *config", so drivers should select it instead
> of depending on it if they need it.
> Relying on it being set for a dependency is risky.
>
> Consistently using "select" or "depends on" can also help reduce
> Kconfig circular dependency issues.
>
> IRQ_DOMAIN is selected 109 times and is depended on 3 times in
> current linux-next. Eliminate the uses of "depends on" by
> converting them to "select".
>
> [PATCH 1/3] extcon: max8997: select IRQ_DOMAIN instead of depending on it
> [PATCH 2/3] of: OF_IRQ: select IRQ_DOMAIN instead of depending on it
> [PATCH 3/3] rtc: mt6397: select IRQ_DOMAIN instead of depending on it
From a Kconfig perspective, your reasoning makes a lot of sense.
Looking at the bigger picture, I wonder if we should just remove the
option and make it unconditional. It is enabled in ever architecture
defconfig other than alpha and sparc, and it's selected by a lot of
very common options such as I2C, GENERIC_MSI_IRQ, GENERIC_IRQ_CHIP,
and PCI_HOST_GENERIC. Enabling the option on Alpha grows the kernel
image from 9010KB to 9023KB, or on m68k Coldfire from 3346KB to
3351KB.
Arnd
On 2/13/23 00:05, Arnd Bergmann wrote: > On Mon, Feb 13, 2023, at 05:15, Randy Dunlap wrote: >> IRQ_DOMAIN is a hidden (not user visible) symbol. Users cannot set >> it directly thru "make *config", so drivers should select it instead >> of depending on it if they need it. >> Relying on it being set for a dependency is risky. >> >> Consistently using "select" or "depends on" can also help reduce >> Kconfig circular dependency issues. >> >> IRQ_DOMAIN is selected 109 times and is depended on 3 times in >> current linux-next. Eliminate the uses of "depends on" by >> converting them to "select". >> >> [PATCH 1/3] extcon: max8997: select IRQ_DOMAIN instead of depending on it >> [PATCH 2/3] of: OF_IRQ: select IRQ_DOMAIN instead of depending on it >> [PATCH 3/3] rtc: mt6397: select IRQ_DOMAIN instead of depending on it > > From a Kconfig perspective, your reasoning makes a lot of sense. > > Looking at the bigger picture, I wonder if we should just remove the > option and make it unconditional. It is enabled in ever architecture > defconfig other than alpha and sparc, and it's selected by a lot of > very common options such as I2C, GENERIC_MSI_IRQ, GENERIC_IRQ_CHIP, > and PCI_HOST_GENERIC. Enabling the option on Alpha grows the kernel > image from 9010KB to 9023KB, or on m68k Coldfire from 3346KB to > 3351KB. Marc, what do you think about this suggestion? Thanks. -- ~Randy
On Tue, 14 Feb 2023 18:30:54 +0000,
Randy Dunlap <rdunlap@infradead.org> wrote:
>
>
>
> On 2/13/23 00:05, Arnd Bergmann wrote:
> > On Mon, Feb 13, 2023, at 05:15, Randy Dunlap wrote:
> >> IRQ_DOMAIN is a hidden (not user visible) symbol. Users cannot set
> >> it directly thru "make *config", so drivers should select it instead
> >> of depending on it if they need it.
> >> Relying on it being set for a dependency is risky.
> >>
> >> Consistently using "select" or "depends on" can also help reduce
> >> Kconfig circular dependency issues.
> >>
> >> IRQ_DOMAIN is selected 109 times and is depended on 3 times in
> >> current linux-next. Eliminate the uses of "depends on" by
> >> converting them to "select".
> >>
> >> [PATCH 1/3] extcon: max8997: select IRQ_DOMAIN instead of depending on it
> >> [PATCH 2/3] of: OF_IRQ: select IRQ_DOMAIN instead of depending on it
> >> [PATCH 3/3] rtc: mt6397: select IRQ_DOMAIN instead of depending on it
> >
> > From a Kconfig perspective, your reasoning makes a lot of sense.
> >
> > Looking at the bigger picture, I wonder if we should just remove the
> > option and make it unconditional. It is enabled in ever architecture
> > defconfig other than alpha and sparc, and it's selected by a lot of
> > very common options such as I2C, GENERIC_MSI_IRQ, GENERIC_IRQ_CHIP,
> > and PCI_HOST_GENERIC. Enabling the option on Alpha grows the kernel
> > image from 9010KB to 9023KB, or on m68k Coldfire from 3346KB to
> > 3351KB.
>
> Marc, what do you think about this suggestion?
Seems sensible enough to me.
I'd also get rid of the IRQ_DOMAIN_HIERARCHY option, which is used by
a ton of things. Architectures that are not using it are either dead,
or at least terminally comatose.
I'm half-tempted to put the following patch into -next. Maybe after
-rc1 though. And then the option can go as well.
M.
diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index a372086750ca..b701569a6237 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -95,7 +95,6 @@ struct irq_domain_ops {
int (*xlate)(struct irq_domain *d, struct device_node *node,
const u32 *intspec, unsigned int intsize,
unsigned long *out_hwirq, unsigned int *out_type);
-#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
/* extended V2 interfaces to support hierarchy irq_domains */
int (*alloc)(struct irq_domain *d, unsigned int virq,
unsigned int nr_irqs, void *arg);
@@ -105,7 +104,6 @@ struct irq_domain_ops {
void (*deactivate)(struct irq_domain *d, struct irq_data *irq_data);
int (*translate)(struct irq_domain *d, struct irq_fwspec *fwspec,
unsigned long *out_hwirq, unsigned int *out_type);
-#endif
#ifdef CONFIG_GENERIC_IRQ_DEBUGFS
void (*debug_show)(struct seq_file *m, struct irq_domain *d,
struct irq_data *irqd, int ind);
@@ -160,9 +158,7 @@ struct irq_domain {
struct irq_domain_chip_generic *gc;
struct device *dev;
struct device *pm_dev;
-#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
struct irq_domain *parent;
-#endif
#ifdef CONFIG_GENERIC_MSI_IRQ
const struct msi_parent_ops *msi_parent_ops;
#endif
@@ -472,7 +468,6 @@ extern void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
void *chip_data, irq_flow_handler_t handler,
void *handler_data, const char *handler_name);
extern void irq_domain_reset_irq_data(struct irq_data *irq_data);
-#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
extern struct irq_domain *irq_domain_create_hierarchy(struct irq_domain *parent,
unsigned int flags, unsigned int size,
struct fwnode_handle *fwnode,
@@ -576,64 +571,6 @@ static inline bool irq_domain_is_msi_device(struct irq_domain *domain)
return domain->flags & IRQ_DOMAIN_FLAG_MSI_DEVICE;
}
-#else /* CONFIG_IRQ_DOMAIN_HIERARCHY */
-static inline int irq_domain_alloc_irqs(struct irq_domain *domain,
- unsigned int nr_irqs, int node, void *arg)
-{
- return -1;
-}
-
-static inline void irq_domain_free_irqs(unsigned int virq,
- unsigned int nr_irqs) { }
-
-static inline bool irq_domain_is_hierarchy(struct irq_domain *domain)
-{
- return false;
-}
-
-static inline bool irq_domain_is_ipi(struct irq_domain *domain)
-{
- return false;
-}
-
-static inline bool irq_domain_is_ipi_per_cpu(struct irq_domain *domain)
-{
- return false;
-}
-
-static inline bool irq_domain_is_ipi_single(struct irq_domain *domain)
-{
- return false;
-}
-
-static inline bool irq_domain_is_msi(struct irq_domain *domain)
-{
- return false;
-}
-
-static inline bool irq_domain_is_msi_remap(struct irq_domain *domain)
-{
- return false;
-}
-
-static inline bool
-irq_domain_hierarchical_is_msi_remap(struct irq_domain *domain)
-{
- return false;
-}
-
-static inline bool irq_domain_is_msi_parent(struct irq_domain *domain)
-{
- return false;
-}
-
-static inline bool irq_domain_is_msi_device(struct irq_domain *domain)
-{
- return false;
-}
-
-#endif /* CONFIG_IRQ_DOMAIN_HIERARCHY */
-
#else /* CONFIG_IRQ_DOMAIN */
static inline void irq_dispose_mapping(unsigned int virq) { }
static inline struct irq_domain *irq_find_matching_fwnode(
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 798a9042421f..57fe065ecd5a 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -730,10 +730,8 @@ static int irq_domain_translate(struct irq_domain *d,
struct irq_fwspec *fwspec,
irq_hw_number_t *hwirq, unsigned int *type)
{
-#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
if (d->ops->translate)
return d->ops->translate(d, fwspec, hwirq, type);
-#endif
if (d->ops->xlate)
return d->ops->xlate(d, to_of_node(fwspec->fwnode),
fwspec->param, fwspec->param_count,
@@ -1076,7 +1074,6 @@ void irq_domain_reset_irq_data(struct irq_data *irq_data)
}
EXPORT_SYMBOL_GPL(irq_domain_reset_irq_data);
-#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
/**
* irq_domain_create_hierarchy - Add a irqdomain into the hierarchy
* @parent: Parent irq domain to associate with the new domain
@@ -1829,46 +1826,6 @@ bool irq_domain_hierarchical_is_msi_remap(struct irq_domain *domain)
}
return false;
}
-#else /* CONFIG_IRQ_DOMAIN_HIERARCHY */
-/**
- * irq_domain_get_irq_data - Get irq_data associated with @virq and @domain
- * @domain: domain to match
- * @virq: IRQ number to get irq_data
- */
-struct irq_data *irq_domain_get_irq_data(struct irq_domain *domain,
- unsigned int virq)
-{
- struct irq_data *irq_data = irq_get_irq_data(virq);
-
- return (irq_data && irq_data->domain == domain) ? irq_data : NULL;
-}
-EXPORT_SYMBOL_GPL(irq_domain_get_irq_data);
-
-/**
- * irq_domain_set_info - Set the complete data for a @virq in @domain
- * @domain: Interrupt domain to match
- * @virq: IRQ number
- * @hwirq: The hardware interrupt number
- * @chip: The associated interrupt chip
- * @chip_data: The associated interrupt chip data
- * @handler: The interrupt flow handler
- * @handler_data: The interrupt flow handler data
- * @handler_name: The interrupt handler name
- */
-void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
- irq_hw_number_t hwirq, const struct irq_chip *chip,
- void *chip_data, irq_flow_handler_t handler,
- void *handler_data, const char *handler_name)
-{
- irq_set_chip_and_handler_name(virq, chip, handler, handler_name);
- irq_set_chip_data(virq, chip_data);
- irq_set_handler_data(virq, handler_data);
-}
-
-static void irq_domain_check_hierarchy(struct irq_domain *domain)
-{
-}
-#endif /* CONFIG_IRQ_DOMAIN_HIERARCHY */
#ifdef CONFIG_GENERIC_IRQ_DEBUGFS
static struct dentry *domain_dir;
@@ -1882,12 +1839,10 @@ irq_domain_debug_show_one(struct seq_file *m, struct irq_domain *d, int ind)
seq_printf(m, "%*sflags: 0x%08x\n", ind +1 , "", d->flags);
if (d->ops && d->ops->debug_show)
d->ops->debug_show(m, d, NULL, ind + 1);
-#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
if (!d->parent)
return;
seq_printf(m, "%*sparent: %s\n", ind + 1, "", d->parent->name);
irq_domain_debug_show_one(m, d->parent, ind + 4);
-#endif
}
static int irq_domain_debug_show(struct seq_file *m, void *p)
--
Without deviation from the norm, progress is not possible.
Hi Marc, On 2/14/23 11:56, Marc Zyngier wrote: > On Tue, 14 Feb 2023 18:30:54 +0000, > Randy Dunlap <rdunlap@infradead.org> wrote: >> >> >> >> On 2/13/23 00:05, Arnd Bergmann wrote: >>> On Mon, Feb 13, 2023, at 05:15, Randy Dunlap wrote: >>>> IRQ_DOMAIN is a hidden (not user visible) symbol. Users cannot set >>>> it directly thru "make *config", so drivers should select it instead >>>> of depending on it if they need it. >>>> Relying on it being set for a dependency is risky. >>>> >>>> Consistently using "select" or "depends on" can also help reduce >>>> Kconfig circular dependency issues. >>>> >>>> IRQ_DOMAIN is selected 109 times and is depended on 3 times in >>>> current linux-next. Eliminate the uses of "depends on" by >>>> converting them to "select". >>>> >>>> [PATCH 1/3] extcon: max8997: select IRQ_DOMAIN instead of depending on it >>>> [PATCH 2/3] of: OF_IRQ: select IRQ_DOMAIN instead of depending on it >>>> [PATCH 3/3] rtc: mt6397: select IRQ_DOMAIN instead of depending on it >>> >>> From a Kconfig perspective, your reasoning makes a lot of sense. >>> >>> Looking at the bigger picture, I wonder if we should just remove the >>> option and make it unconditional. It is enabled in ever architecture >>> defconfig other than alpha and sparc, and it's selected by a lot of >>> very common options such as I2C, GENERIC_MSI_IRQ, GENERIC_IRQ_CHIP, >>> and PCI_HOST_GENERIC. Enabling the option on Alpha grows the kernel >>> image from 9010KB to 9023KB, or on m68k Coldfire from 3346KB to >>> 3351KB. >> >> Marc, what do you think about this suggestion? > > Seems sensible enough to me. > > I'd also get rid of the IRQ_DOMAIN_HIERARCHY option, which is used by > a ton of things. Architectures that are not using it are either dead, > or at least terminally comatose. > > I'm half-tempted to put the following patch into -next. Maybe after > -rc1 though. And then the option can go as well. > > M. What is this patch based on? It doesn't apply cleanly to current linux-next. I made a similar patch (to linux-next) that drops the IRQ_DOMAIN_HIERARCHY option and converts its dependent code to always on. It has been built (multiple randconfigs) on all ARCHes (except hexagon), both 32-bit and 64-bit where applicable (not that it should matter here). But yes, let's plan to get one of these patches in soon (after -rc1). Thanks. -- ~Randy
On Thu, 23 Feb 2023 18:37:01 +0000, Randy Dunlap <rdunlap@infradead.org> wrote: > > Hi Marc, > > On 2/14/23 11:56, Marc Zyngier wrote: > > On Tue, 14 Feb 2023 18:30:54 +0000, > > Randy Dunlap <rdunlap@infradead.org> wrote: > >> > >> > >> > >> On 2/13/23 00:05, Arnd Bergmann wrote: > >>> On Mon, Feb 13, 2023, at 05:15, Randy Dunlap wrote: > >>>> IRQ_DOMAIN is a hidden (not user visible) symbol. Users cannot set > >>>> it directly thru "make *config", so drivers should select it instead > >>>> of depending on it if they need it. > >>>> Relying on it being set for a dependency is risky. > >>>> > >>>> Consistently using "select" or "depends on" can also help reduce > >>>> Kconfig circular dependency issues. > >>>> > >>>> IRQ_DOMAIN is selected 109 times and is depended on 3 times in > >>>> current linux-next. Eliminate the uses of "depends on" by > >>>> converting them to "select". > >>>> > >>>> [PATCH 1/3] extcon: max8997: select IRQ_DOMAIN instead of depending on it > >>>> [PATCH 2/3] of: OF_IRQ: select IRQ_DOMAIN instead of depending on it > >>>> [PATCH 3/3] rtc: mt6397: select IRQ_DOMAIN instead of depending on it > >>> > >>> From a Kconfig perspective, your reasoning makes a lot of sense. > >>> > >>> Looking at the bigger picture, I wonder if we should just remove the > >>> option and make it unconditional. It is enabled in ever architecture > >>> defconfig other than alpha and sparc, and it's selected by a lot of > >>> very common options such as I2C, GENERIC_MSI_IRQ, GENERIC_IRQ_CHIP, > >>> and PCI_HOST_GENERIC. Enabling the option on Alpha grows the kernel > >>> image from 9010KB to 9023KB, or on m68k Coldfire from 3346KB to > >>> 3351KB. > >> > >> Marc, what do you think about this suggestion? > > > > Seems sensible enough to me. > > > > I'd also get rid of the IRQ_DOMAIN_HIERARCHY option, which is used by > > a ton of things. Architectures that are not using it are either dead, > > or at least terminally comatose. > > > > I'm half-tempted to put the following patch into -next. Maybe after > > -rc1 though. And then the option can go as well. > > > > M. > > What is this patch based on? It doesn't apply cleanly to current linux-next. Not very surprising, I usually base my stuff on a stable rc tag. But in this instance, it may have been based on whatever was in my sandbox at that point in time, and subsequently discarded. > I made a similar patch (to linux-next) that drops the IRQ_DOMAIN_HIERARCHY > option and converts its dependent code to always on. > It has been built (multiple randconfigs) on all ARCHes (except hexagon), > both 32-bit and 64-bit where applicable (not that it should matter here). > > But yes, let's plan to get one of these patches in soon (after -rc1). Please send it based on -rc1 once it is out, and I'll be happy to stick that in -next for further simmering. Thanks, M. -- Without deviation from the norm, progress is not possible.
On 2/28/23 11:29, Marc Zyngier wrote: > On Thu, 23 Feb 2023 18:37:01 +0000, > Randy Dunlap <rdunlap@infradead.org> wrote: >> >> Hi Marc, >> >> On 2/14/23 11:56, Marc Zyngier wrote: >>> On Tue, 14 Feb 2023 18:30:54 +0000, >>> Randy Dunlap <rdunlap@infradead.org> wrote: >>>> >>>> >>>> >>>> On 2/13/23 00:05, Arnd Bergmann wrote: >>>>> On Mon, Feb 13, 2023, at 05:15, Randy Dunlap wrote: >>>>>> IRQ_DOMAIN is a hidden (not user visible) symbol. Users cannot set >>>>>> it directly thru "make *config", so drivers should select it instead >>>>>> of depending on it if they need it. >>>>>> Relying on it being set for a dependency is risky. >>>>>> >>>>>> Consistently using "select" or "depends on" can also help reduce >>>>>> Kconfig circular dependency issues. >>>>>> >>>>>> IRQ_DOMAIN is selected 109 times and is depended on 3 times in >>>>>> current linux-next. Eliminate the uses of "depends on" by >>>>>> converting them to "select". >>>>>> >>>>>> [PATCH 1/3] extcon: max8997: select IRQ_DOMAIN instead of depending on it >>>>>> [PATCH 2/3] of: OF_IRQ: select IRQ_DOMAIN instead of depending on it >>>>>> [PATCH 3/3] rtc: mt6397: select IRQ_DOMAIN instead of depending on it >>>>> >>>>> From a Kconfig perspective, your reasoning makes a lot of sense. >>>>> >>>>> Looking at the bigger picture, I wonder if we should just remove the >>>>> option and make it unconditional. It is enabled in ever architecture >>>>> defconfig other than alpha and sparc, and it's selected by a lot of >>>>> very common options such as I2C, GENERIC_MSI_IRQ, GENERIC_IRQ_CHIP, >>>>> and PCI_HOST_GENERIC. Enabling the option on Alpha grows the kernel >>>>> image from 9010KB to 9023KB, or on m68k Coldfire from 3346KB to >>>>> 3351KB. >>>> >>>> Marc, what do you think about this suggestion? >>> >>> Seems sensible enough to me. >>> >>> I'd also get rid of the IRQ_DOMAIN_HIERARCHY option, which is used by >>> a ton of things. Architectures that are not using it are either dead, >>> or at least terminally comatose. >>> >>> I'm half-tempted to put the following patch into -next. Maybe after >>> -rc1 though. And then the option can go as well. >>> >>> M. >> >> What is this patch based on? It doesn't apply cleanly to current linux-next. > > Not very surprising, I usually base my stuff on a stable rc tag. But > in this instance, it may have been based on whatever was in my sandbox > at that point in time, and subsequently discarded. > >> I made a similar patch (to linux-next) that drops the IRQ_DOMAIN_HIERARCHY >> option and converts its dependent code to always on. >> It has been built (multiple randconfigs) on all ARCHes (except hexagon), >> both 32-bit and 64-bit where applicable (not that it should matter here). >> >> But yes, let's plan to get one of these patches in soon (after -rc1). > > Please send it based on -rc1 once it is out, and I'll be happy to > stick that in -next for further simmering. Alrighty, will do. Thanks. -- ~Randy
© 2016 - 2024 Red Hat, Inc.