Re: [Patch V2 13/13] genirq/msi: Provide helpers to return Linux IRQ/dev_msi hw IRQ number

[Dey, Megha]

Hi Marc,

On 3/26/2021 6:28 PM, Marc Zyngier wrote:
> On Fri, 26 Mar 2021 01:02:43 +0000,
> "Dey, Megha" <megha.dey@xxxxxxxxx> wrote:
> > Hi Marc,
> >
> > On 3/25/2021 10:53 AM, Marc Zyngier wrote:
> > > On Fri, 26 Feb 2021 20:11:17 +0000,
> > > Megha Dey <megha.dey@xxxxxxxxx> wrote:
> > > > From: Dave Jiang <dave.jiang@xxxxxxxxx>
> > > >
> > > > Add new helpers to get the Linux IRQ number and device specific index
> > > > for given device-relative vector so that the drivers don't need to
> > > > allocate their own arrays to keep track of the vectors and hwirq for
> > > > the multi vector device MSI case.
> > > >
> > > > Reviewed-by: Tony Luck <tony.luck@xxxxxxxxx>
> > > > Signed-off-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> > > > Signed-off-by: Megha Dey <megha.dey@xxxxxxxxx>
> > > > ---
> > > >    include/linux/msi.h |  2 ++
> > > >    kernel/irq/msi.c    | 44 ++++++++++++++++++++++++++++++++++++++++++++
> > > >    2 files changed, 46 insertions(+)
> > > >
> > > > diff --git a/include/linux/msi.h b/include/linux/msi.h
> > > > index 24abec0..d60a6ba 100644
> > > > --- a/include/linux/msi.h
> > > > +++ b/include/linux/msi.h
> > > > @@ -451,6 +451,8 @@ struct irq_domain *platform_msi_create_irq_domain(struct fwnode_handle *fwnode,
> > > >    int platform_msi_domain_alloc_irqs(struct device *dev, unsigned int nvec,
> > > >    				   irq_write_msi_msg_t write_msi_msg);
> > > >    void platform_msi_domain_free_irqs(struct device *dev);
> > > > +int msi_irq_vector(struct device *dev, unsigned int nr);
> > > > +int dev_msi_hwirq(struct device *dev, unsigned int nr);
> > > >      /* When an MSI domain is used as an intermediate domain */
> > > >    int msi_domain_prepare_irqs(struct irq_domain *domain, struct device *dev,
> > > > diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> > > > index 047b59d..f2a8f55 100644
> > > > --- a/kernel/irq/msi.c
> > > > +++ b/kernel/irq/msi.c
> > > > @@ -581,4 +581,48 @@ struct msi_domain_info *msi_get_domain_info(struct irq_domain *domain)
> > > >    	return (struct msi_domain_info *)domain->host_data;
> > > >    }
> > > >    +/**
> > > > + * msi_irq_vector - Get the Linux IRQ number of a device vector
> > > > + * @dev: device to operate on
> > > > + * @nr: device-relative interrupt vector index (0-based).
> > > > + *
> > > > + * Returns the Linux IRQ number of a device vector.
> > > > + */
> > > > +int msi_irq_vector(struct device *dev, unsigned int nr)
> > > > +{
> > > > +	struct msi_desc *entry;
> > > > +	int i = 0;
> > > > +
> > > > +	for_each_msi_entry(entry, dev) {
> > > > +		if (i == nr)
> > > > +			return entry->irq;
> > > > +		i++;
> > > This obviously doesn't work with Multi-MSI, does it?
> > This API is only for devices that support device MSI interrupts. They
> > follow MSI-x format and don't support multi MSI (part of MSI).
> >
> > Not sure if I am missing something here, can you please let me know?
> Nothing in the prototype of the function indicates this limitation,
> nor does the documentation. And I'm not sure why you should exclude
> part of the MSI functionality here. It can't be for performance
> reason, so you might as well make sure this works for all the MSI
> variants:
>
> int msi_irq_vector(struct device *dev, unsigned int nr)
> {
> 	struct msi_desc *entry;
> 	int irq, index = 0;
>
> 	for_each_msi_vector(entry, irq, dev) {
> 		if (index == nr}
> 			return irq;
> 		index++;
> 	}
>
> 	return WARN_ON_ONCE(-EINVAL);
> }
Ok, got it!
> > > > +	}
> > > > +	WARN_ON_ONCE(1);
> > > > +	return -EINVAL;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(msi_irq_vector);
> > > > +
> > > > +/**
> > > > + * dev_msi_hwirq - Get the device MSI hw IRQ number of a device vector
> > > > + * @dev: device to operate on
> > > > + * @nr: device-relative interrupt vector index (0-based).
> > > > + *
> > > > + * Return the dev_msi hw IRQ number of a device vector.
> > > > + */
> > > > +int dev_msi_hwirq(struct device *dev, unsigned int nr)
> > > > +{
> > > > +	struct msi_desc *entry;
> > > > +	int i = 0;
> > > > +
> > > > +	for_each_msi_entry(entry, dev) {
> > > > +		if (i == nr)
> > > > +			return entry->device_msi.hwirq;
> > > > +		i++;
> > > > +	}
> > > > +	WARN_ON_ONCE(1);
> > > > +	return -EINVAL;
> > > > +}
> And this helper would be more generally useful if it returned the n-th
> msi_desc entry rather than some obscure field in a substructure.
>
> struct msi_desc *msi_get_nth_desc(struct device *dev, unsigned int nth)
> {
> 	struct msi_desc *entry = NULL;
> 	unsigned int i = 0;
>
> 	for_each_msi_entry(entry, dev) {
> 		if (i == nth)
> 			return entry;
>
> 		i++;
> 	}
>
> 	WARN_ON_ONCE(!entry);
> 	return entry;
> }
>
> You can always wrap it for your particular use case.
Yeah, makes sense.
> > > > +EXPORT_SYMBOL_GPL(dev_msi_hwirq);
> > > > +
> > > >    #endif /* CONFIG_GENERIC_MSI_IRQ_DOMAIN */
> > > And what uses these helpers?]
> > These helpers are to be used by a driver series(Intel's IDXD driver)
> > which is currently stuck due to VFIO refactoring.
> Then I's suggest you keep the helpers together with the actual user,
> unless this can generally be useful to existing users (exported
> symbols without in-tree users is always a bit odd).
Yeah in the next submission, we will submit this patch series along with 
the user driver patch series.
> Thanks,
>
> 	M.