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

[Marc Zyngier]

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);
}

>
> > 
> >> +	}
> >> +	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.

> >> +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).

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.