| Message ID | 20230213041535.12083-1-rdunlap@infradead.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | IRQ_DOMAIN: remove all "depends on", use only "select" | expand |
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.
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)
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.
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.
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.