Re: [PATCH 0/3] IRQ_DOMAIN: remove all "depends on", use only "select"
From: Marc Zyngier
Date: Tue Feb 14 2023 - 14:56:19 EST
On Tue, 14 Feb 2023 18:30:54 +0000,
Randy Dunlap <rdunlap@xxxxxxxxxxxxx> 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.