Re: [patch v6 3/7] genirq/affinity: Add new callback for (re)calculating interrupt sets

[Thomas Gleixner <tglx@linutronix.de>]

On Wed, Jun 16 2021 at 08:40, Ming Lei wrote:
> On Tue, Jun 15, 2021 at 02:57:07PM -0500, Bjorn Helgaas wrote:

> +static inline void irq_affinity_calc_sets_legacy(struct irq_affinity *affd)

This function name sucks because the function is really a wrapper around
irq_affinity_calc_sets(). What's so legacy about this? The fact that
it's called from the legacy PCI single interrupt code path?

> @@ -405,6 +405,30 @@ static void default_calc_sets(struct irq_affinity *affd, unsigned int affvecs)
>  	affd->set_size[0] = affvecs;
>  }
>  
> +static void irq_affinity_calc_sets(unsigned int affvecs,
> +		struct irq_affinity *affd)

Please align the arguments when you need a line break.

> +{
> +	/*
> +	 * Simple invocations do not provide a calc_sets() callback. Install
> +	 * the generic one.
> +	 */
> +	if (!affd->calc_sets)
> +		affd->calc_sets = default_calc_sets;
> +
> +	/* Recalculate the sets */
> +	affd->calc_sets(affd, affvecs);
> +
> +	WARN_ON_ONCE(affd->nr_sets > IRQ_AFFINITY_MAX_SETS);

Hrm. That function really should return an error code to tell the caller
that something went wrong.

> +}
> +
> +/* Provide a chance to call ->calc_sets for legacy */

What does this comment tell? Close to zero. 

> +void irq_affinity_calc_sets_legacy(struct irq_affinity *affd)
> +{
> +	if (!affd)
> +		return;
> +	irq_affinity_calc_sets(0, affd);
> +}

What's wrong with just exposing irq_affinity_calc_sets() have that
NULL pointer check in the function and add proper function documentation
which explains what this is about?

Thanks,

        tglx