Message-ID: <2eb39a8f-cc58-4774-836c-e6293300a4d9@gmail.com>
Date: Tue, 6 Aug 2024 14:51:00 +0300
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Thomas Gleixner <tglx@...utronix.de>,
 Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
Cc: Mark Brown <broonie@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 "Rafael J. Wysocki" <rafael@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] regmap: Allow setting IRQ domain name suffix

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! ~~

Powered by blists - more mailing lists