Re: [PATCH 1/2] genirq: Extract irq_set_affinity_masks() from devm_platform_get_irqs_affinity()

[Marc Zyngier]

Hi John,

On 2022-02-17 17:07, John Garry wrote:
> On 16/02/2022 09:08, Marc Zyngier wrote:
>
> Hi Marc,
>
> > In order to better support drivers that deal with interrupts in a more
> > "hands-on" way, extract the core of devm_platform_get_irqs_affinity()
> > and expose it as irq_set_affinity_masks().
> >
> > This helper allows a driver to provide a set of wired interrupts that
> > are to be configured as managed interrupts. As with the original 
> > helper,
> > this is exported as EXPORT_SYMBOL_GPL.
>
> I know you mentioned it in 2/2, but it would be interesting to see how
> network controller drivers can handle the problem of missing in-flight
> IO completions for managed irq shutdown. For storage controllers this
> is all now safely handled in the block layer.

Do you have a pointer to this? It'd be interesting to see if there is
a common pattern.

> > Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
>
> Just a small comment, below.
>
> Tested-by: John Garry <john.garry@xxxxxxxxxx> #D05
>
> Thanks,
> John
>
> > ---
> >   drivers/base/platform.c   | 20 +++-----------------
> >   include/linux/interrupt.h |  8 ++++++++
> >   kernel/irq/affinity.c     | 27 +++++++++++++++++++++++++++
> >   3 files changed, 38 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 6cb04ac48bf0..b363cf6ce5be 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -335,7 +335,6 @@ int devm_platform_get_irqs_affinity(struct 
> > platform_device *dev,
> >   				    int **irqs)
> >   {
> >   	struct irq_affinity_devres *ptr;
> > -	struct irq_affinity_desc *desc;
> >   	size_t size;
> >   	int i, ret, nvec;
> >   @@ -376,31 +375,18 @@ int devm_platform_get_irqs_affinity(struct 
> > platform_device *dev,
> >   		ptr->irq[i] = irq;
> >   	}
> >   -	desc = irq_create_affinity_masks(nvec, affd);
> > -	if (!desc) {
> > -		ret = -ENOMEM;
> > +	ret = irq_set_affinity_masks(affd, ptr->irq, nvec);
> > +	if (ret) {
> > +		dev_err(&dev->dev, "failed to update affinity descriptors (%d)\n", 
> > ret);
> >   		goto err_free_devres;
> >   	}
> >   -	for (i = 0; i < nvec; i++) {
> > -		ret = irq_update_affinity_desc(ptr->irq[i], &desc[i]);
> > -		if (ret) {
> > -			dev_err(&dev->dev, "failed to update irq%d affinity descriptor 
> > (%d)\n",
> > -				ptr->irq[i], ret);
> > -			goto err_free_desc;
> > -		}
> > -	}
> > -
> >   	devres_add(&dev->dev, ptr);
> >   -	kfree(desc);
> > -
> >   	*irqs = ptr->irq;
> >     	return nvec;
> >   -err_free_desc:
> > -	kfree(desc);
> >   err_free_devres:
> >   	devres_free(ptr);
> >   	return ret;
> > diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> > index 9367f1cb2e3c..6bfce96206f8 100644
> > --- a/include/linux/interrupt.h
> > +++ b/include/linux/interrupt.h
> > @@ -381,6 +381,8 @@ irq_create_affinity_masks(unsigned int nvec, 
> > struct irq_affinity *affd);
> >   unsigned int irq_calc_affinity_vectors(unsigned int minvec, unsigned 
> > int maxvec,
> >   				       const struct irq_affinity *affd);
> >   +int irq_set_affinity_masks(struct irq_affinity *affd, int *irqs, 
> > int nvec);
> > +
> >   #else /* CONFIG_SMP */
> >     static inline int irq_set_affinity(unsigned int irq, const struct 
> > cpumask *m)
> > @@ -443,6 +445,12 @@ irq_calc_affinity_vectors(unsigned int minvec, 
> > unsigned int maxvec,
> >   	return maxvec;
> >   }
> >   +static inline int
> > +irq_set_affinity_masks(struct irq_affinity *affd, int *irqs, int 
> > nvec)
> > +{
> > +	return -EINVAL;
> > +}
> > +
> >   #endif /* CONFIG_SMP */
> >     /*
> > diff --git a/kernel/irq/affinity.c b/kernel/irq/affinity.c
> > index f7ff8919dc9b..c0f868cd5b87 100644
> > --- a/kernel/irq/affinity.c
> > +++ b/kernel/irq/affinity.c
> > @@ -512,3 +512,30 @@ unsigned int irq_calc_affinity_vectors(unsigned 
> > int minvec, unsigned int maxvec,
> >     	return resv + min(set_vecs, maxvec - resv);
> >   }
> > +
> > +/*
> > + * irq_set_affinity_masks - Set the affinity masks of a number of 
> > interrupts
> > + *                          for multiqueue spreading
> > + * @affd:	Description of the affinity requirements
> > + * @irqs:	An array of interrupt numbers
> > + * @nvec:	The total number of interrupts
> > + */
> > +int irq_set_affinity_masks(struct irq_affinity *affd, int *irqs, int 
> > nvec)
> > +{
> > +	struct irq_affinity_desc *desc;
> > +	int i, err = 0;
>
> nit: it might be worth doing something similar to how
> pci_alloc_irq_vectors_affinity() handles sets with no pre- and
> post-vectors with msi_default_affd

Yes, good point. This would probably simplify most callers of this code.

Thanks,

          M.
--
Jazz is not dead. It just smells funny...