Re: [PATCH 2/2] regmap: Allow setting IRQ domain name suffix

Matti Vaittinen
2024-08-06T11:51:04Z

Hello again!

On 7/13/24 15:22, Thomas Gleixner wrote:
> Matti!
>
> On Mon, Jul 08 2024 at 15:40, Matti Vaittinen wrote:
>> On 7/7/24 21:13, Thomas Gleixner wrote:
>>>
>>> I wonder whether this can be handled at the core. Let me stare at it.
>>
>> Thanks Thomas! I'll wait for your ideas before re-spinning this series :)
>
> Something like the untested below should work. That would make your
> info:
>
> struct irq_domain_info info = {
> .fwnode = fwnode,
> .size = chip->num_irqs,

Based on my code reading, the .size is used for allocating the "revmap".
Looking at the info struct for existing implementation of the
irq_domain_create_legacy(), the .size is set as:

.size = first_hwirq + size,

> .hwirq_max = chip->num_irqs,

Also, the irq_domain_create_legacy() sets hwirq_max as:

.hwirq_max = first_hwirq + size.

see:

> @@ -476,18 +486,14 @@ struct irq_domain *irq_domain_create_leg
> .fwnode = fwnode,
> .size = first_hwirq + size,
> .hwirq_max = first_hwirq + size,
> + .hwirq_base = first_hwirq,
> + .virq_base = first_irq,
> .ops = ops,
> .host_data = host_data,
> };
> - struct irq_domain *domain;
> + struct irq_domain *domain = irq_domain_instantiate(&info);
>
> - domain = irq_domain_instantiate(&info);
> - if (IS_ERR(domain))
> - return NULL;
> -
> - irq_domain_associate_many(domain, first_irq, first_hwirq, size);

Lookin at this, the existing code calls irq_domain_associate_many() with
the given size parameter (without the + first_hwirq which is assigned to
.size).

I think this is not aligned with what the patch below results (and yes,
I know Thomas told it's untested).

I'd better admit I am not 100% sure how the legacy domains work and that
I don't (any more) fully trust on my ability to flawlessly interpret the
code ;) Hence I'd rather learn from a small explanation (what is the
expected .size) than by fixing this after I see regression reports from
real users of the irq_domain_create_legacy() :)

So, any guidance as to what the revmap allocation size should be (the
info->size), and what should be the size for the
irq_domain_associate_many()?

Thanks...

Yours,
-- Matti

> .virq_base = irq_base,
> .ops = &regmap_domain_ops,
> .host_data = d,
> .name_suffix = chip->domain_suffix,
> };
>
> Thanks,
>
> tglx
> ---
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -291,6 +291,9 @@ struct irq_domain_chip_generic_info;
> * @hwirq_max: Maximum number of interrupts supported by controller
> * @direct_max: Maximum value of direct maps;
> * Use ~0 for no limit; 0 for no direct mapping
> + * @hwirq_base: The first hardware interrupt number (legacy domains only)
> + * @virq_base: The first Linux interrupt number for legacy domains to
> + * immediately associate the interrupts after domain creation
> * @bus_token: Domain bus token
> * @ops: Domain operation callbacks
> * @host_data: Controller private data pointer
> @@ -307,6 +310,8 @@ struct irq_domain_info {
> unsigned int size;
> irq_hw_number_t hwirq_max;
> int direct_max;
> + unsigned int hwirq_base;
> + unsigned int virq_base;
> enum irq_domain_bus_token bus_token;
> const struct irq_domain_ops *ops;
> void *host_data;
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -267,13 +267,20 @@ static void irq_domain_free(struct irq_d
> kfree(domain);
> }
>
> -/**
> - * irq_domain_instantiate() - Instantiate a new irq domain data structure
> - * @info: Domain information pointer pointing to the information for this domain
> - *
> - * Return: A pointer to the instantiated irq domain or an ERR_PTR value.
> - */
> -struct irq_domain *irq_domain_instantiate(const struct irq_domain_info *info)
> +static void irq_domain_instantiate_descs(const struct irq_domain_info *info)
> +{
> + if (!IS_ENABLED(CONFIG_SPARSE_IRQ))
> + return;
> +
> + if (irq_alloc_descs(info->virq_base, info->virq_base, info->size,
> + of_node_to_nid(to_of_node(info->fwnode))) < 0) {
> + pr_info("Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
> + info->virq_base);
> + }
> +}
> +
> +static struct irq_domain *__irq_domain_instantiate(const struct irq_domain_info *info,
> + bool cond_alloc_descs)
> {
> struct irq_domain *domain;
> int err;
> @@ -306,6 +313,13 @@ struct irq_domain *irq_domain_instantiat
>
> __irq_domain_publish(domain);
>
> + if (cond_alloc_descs && info->virq_base > 0)
> + irq_domain_instantiate_descs(info);
> +
> + /* Legacy interrupt domains have a fixed Linux interrupt number */
> + if (info->virq_base > 0)
> + irq_domain_associate_many(domain, info->virq_base, info->hwirq_base, info->size);
> +
> return domain;
>
> err_domain_gc_remove:
> @@ -315,6 +329,17 @@ struct irq_domain *irq_domain_instantiat
> irq_domain_free(domain);
> return ERR_PTR(err);
> }
> +
> +/**
> + * irq_domain_instantiate() - Instantiate a new irq domain data structure
> + * @info: Domain information pointer pointing to the information for this domain
> + *
> + * Return: A pointer to the instantiated irq domain or an ERR_PTR value.
> + */
> +struct irq_domain *irq_domain_instantiate(const struct irq_domain_info *info)
> +{
> + return __irq_domain_instantiate(info, false);
> +}
> EXPORT_SYMBOL_GPL(irq_domain_instantiate);
>
> /**
> @@ -413,28 +438,13 @@ struct irq_domain *irq_domain_create_sim
> .fwnode = fwnode,
> .size = size,
> .hwirq_max = size,
> + .virq_base = first_irq,
> .ops = ops,
> .host_data = host_data,
> };
> - struct irq_domain *domain;
> -
> - domain = irq_domain_instantiate(&info);
> - if (IS_ERR(domain))
> - return NULL;
> -
> - if (first_irq > 0) {
> - if (IS_ENABLED(CONFIG_SPARSE_IRQ)) {
> - /* attempt to allocated irq_descs */
> - int rc = irq_alloc_descs(first_irq, first_irq, size,
> - of_node_to_nid(to_of_node(fwnode)));
> - if (rc < 0)
> - pr_info("Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
> - first_irq);
> - }
> - irq_domain_associate_many(domain, first_irq, 0, size);
> - }
> + struct irq_domain *domain = __irq_domain_instantiate(&info, true);
>
> - return domain;
> + return IS_ERR(domain) ? NULL : domain;
> }
> EXPORT_SYMBOL_GPL(irq_domain_create_simple);
>
> @@ -476,18 +486,14 @@ struct irq_domain *irq_domain_create_leg
> .fwnode = fwnode,
> .size = first_hwirq + size,
> .hwirq_max = first_hwirq + size,
> + .hwirq_base = first_hwirq,
> + .virq_base = first_irq,
> .ops = ops,
> .host_data = host_data,
> };
> - struct irq_domain *domain;
> + struct irq_domain *domain = irq_domain_instantiate(&info);
>
> - domain = irq_domain_instantiate(&info);
> - if (IS_ERR(domain))
> - return NULL;
> -
> - irq_domain_associate_many(domain, first_irq, first_hwirq, size);
> -
> - return domain;
> + return IS_ERR(domain) ? NULL : domain;
> }
> EXPORT_SYMBOL_GPL(irq_domain_create_legacy);
>

--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~