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

[Marc Zyngier]

On Mon, 21 Nov 2022 14:36:28 +0000,
Thomas Gleixner <tglx@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxx>

	M.

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