Re: [PATCH 0/3] IRQ_DOMAIN: remove all "depends on", use only "select"

[Marc Zyngier <maz@kernel.org>]

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.