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

  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.
  

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.