[V2,08/21] genirq/msi: Add pointers for per device irq domains

  With the upcoming per device MSI interrupt domain support it is necessary
to store the domain pointers per device.

Instead of delegating that storage to device drivers or subsystems create a
storage array in struct msi_device_data which will also take care of
tearing down the irq domains when msi_device_data is cleaned up via devres.

The interfaces into the MSI core will be changed from irqdomain pointer
based interfaces to domain id based interfaces to support multiple MSI
domains on a single device (e.g. PCI/MSI[-X] and PCI/IMS.

Once the per device domain support is complete the irq domain pointer in
struct device::msi.domain will not longer contain a pointer to the "global"
MSI domain. It will contain a pointer to the MSI parent domain instead.

It would be a horrible maze of conditionals to evaluate all over the place
which domain pointer should be used, i.e. the "global" one in
device::msi::domain or one from the internal pointer array.

To avoid this evaluate in msi_setup_device_data() whether the irq domain
which is associated to a device is a "global" or a parent MSI domain. If it
is global then copy the pointer into the first entry in the irqdomain
pointer array.

This allows to convert interfaces and implementation to domain ids while
keeping everything existing working.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/msi.h     |    3 +++
 include/linux/msi_api.h |    8 ++++++++
 kernel/irq/msi.c        |   14 ++++++++++++++
 3 files changed, 25 insertions(+)
  

On Mon, 21 Nov 2022 14:36:28 +0000,
Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> With the upcoming per device MSI interrupt domain support it is necessary
> to store the domain pointers per device.
> 
> Instead of delegating that storage to device drivers or subsystems create a
> storage array in struct msi_device_data which will also take care of
> tearing down the irq domains when msi_device_data is cleaned up via devres.
> 
> The interfaces into the MSI core will be changed from irqdomain pointer
> based interfaces to domain id based interfaces to support multiple MSI
> domains on a single device (e.g. PCI/MSI[-X] and PCI/IMS.
> 
> Once the per device domain support is complete the irq domain pointer in
> struct device::msi.domain will not longer contain a pointer to the "global"
> MSI domain. It will contain a pointer to the MSI parent domain instead.
> 
> It would be a horrible maze of conditionals to evaluate all over the place
> which domain pointer should be used, i.e. the "global" one in
> device::msi::domain or one from the internal pointer array.
> 
> To avoid this evaluate in msi_setup_device_data() whether the irq domain
> which is associated to a device is a "global" or a parent MSI domain. If it
> is global then copy the pointer into the first entry in the irqdomain
> pointer array.
> 
> This allows to convert interfaces and implementation to domain ids while
> keeping everything existing working.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  include/linux/msi.h     |    3 +++
>  include/linux/msi_api.h |    8 ++++++++
>  kernel/irq/msi.c        |   14 ++++++++++++++
>  3 files changed, 25 insertions(+)
> 
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -77,6 +77,7 @@ struct msi_desc;
>  struct pci_dev;
>  struct platform_msi_priv_data;
>  struct device_attribute;
> +struct irq_domain;
>  
>  void __get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg);
>  #ifdef CONFIG_GENERIC_MSI_IRQ
> @@ -180,6 +181,7 @@ enum msi_desc_filter {
>   * @mutex:		Mutex protecting the MSI descriptor store
>   * @__store:		Xarray for storing MSI descriptor pointers
>   * @__iter_idx:		Index to search the next entry for iterators
> + * @__irqdomains:	Per device interrupt domains
>   */
>  struct msi_device_data {
>  	unsigned long			properties;
> @@ -187,6 +189,7 @@ struct msi_device_data {
>  	struct mutex			mutex;
>  	struct xarray			__store;
>  	unsigned long			__iter_idx;
> +	struct irq_domain		*__irqdomains[MSI_MAX_DEVICE_IRQDOMAINS];
>  };
>  
>  int msi_setup_device_data(struct device *dev);
> --- a/include/linux/msi_api.h
> +++ b/include/linux/msi_api.h
> @@ -10,6 +10,14 @@
>  
>  struct device;
>  
> +/*
> + * Per device interrupt domain related constants.
> + */
> +enum msi_domain_ids {
> +	MSI_DEFAULT_DOMAIN,
> +	MSI_MAX_DEVICE_IRQDOMAINS,
> +};
> +
>  unsigned int msi_get_virq(struct device *dev, unsigned int index);
>  
>  #endif
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -21,6 +21,18 @@
>  
>  static inline int msi_sysfs_create_group(struct device *dev);
>  
> +static inline void msi_setup_default_irqdomain(struct device *dev, struct msi_device_data *md)

Do we really need this to be inline? I'm sure the compiler can figure
it out.

> +{
> +	if (!dev->msi.domain)
> +		return;
> +	/*
> +	 * If @dev::msi::domain is a global MSI domain, copy the pointer
> +	 * into the domain array to avoid conditionals all over the place.
> +	 */
> +	if (!irq_domain_is_msi_parent(dev->msi.domain))
> +		md->__irqdomains[MSI_DEFAULT_DOMAIN] = dev->msi.domain;
> +}
> +
>  /**
>   * msi_alloc_desc - Allocate an initialized msi_desc
>   * @dev:	Pointer to the device for which this is allocated
> @@ -213,6 +225,8 @@ int msi_setup_device_data(struct device
>  		return ret;
>  	}
>  
> +	msi_setup_default_irqdomain(dev, md);
> +

nit: if you move the setup below the msi.data assignment, you could
only pass dev as a parameter. Or pass both and move the assignment in
the function?

>  	xa_init(&md->__store);
>  	mutex_init(&md->mutex);
>  	dev->msi.data = md;
> 
> 

Irrespective of the above,

Reviewed-by: Marc Zyngier <maz@kernel.org>

	M.
  

On Thu, Nov 24 2022 at 14:56, Marc Zyngier wrote:
> On Mon, 21 Nov 2022 14:36:28 +0000,
> Thomas Gleixner <tglx@linutronix.de> wrote:
>>  static inline int msi_sysfs_create_group(struct device *dev);
>>  
>> +static inline void msi_setup_default_irqdomain(struct device *dev, struct msi_device_data *md)
>
> Do we really need this to be inline? I'm sure the compiler can figure
> it out.

No. I'll fix that up.

>> +{
>> +	if (!dev->msi.domain)
>> +		return;
>> +	/*
>> +	 * If @dev::msi::domain is a global MSI domain, copy the pointer
>> +	 * into the domain array to avoid conditionals all over the place.
>> +	 */
>> +	if (!irq_domain_is_msi_parent(dev->msi.domain))
>> +		md->__irqdomains[MSI_DEFAULT_DOMAIN] = dev->msi.domain;
>> +}
>> +
>>  /**
>>   * msi_alloc_desc - Allocate an initialized msi_desc
>>   * @dev:	Pointer to the device for which this is allocated
>> @@ -213,6 +225,8 @@ int msi_setup_device_data(struct device
>>  		return ret;
>>  	}
>>  
>> +	msi_setup_default_irqdomain(dev, md);
>> +
>
> nit: if you move the setup below the msi.data assignment, you could
> only pass dev as a parameter. Or pass both and move the assignment in
> the function?

In which order we do that it all looks wrong :)

>>  	xa_init(&md->__store);
>>  	mutex_init(&md->mutex);
>>  	dev->msi.data = md;
>> 
>> 
>
> Irrespective of the above,
>
> Reviewed-by: Marc Zyngier <maz@kernel.org>
>
> 	M.