Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 24 Nov 2022 14:56:25 +0000
Message-ID: <86sfi8mmee.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     LKML <linux-kernel@vger.kernel.org>, x86@kernel.org,
        Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
        linux-pci@vger.kernel.org, Bjorn Helgaas <bhelgaas@google.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jason Gunthorpe <jgg@mellanox.com>,
        Dave Jiang <dave.jiang@intel.com>,
        Alex Williamson <alex.williamson@redhat.com>,
        Kevin Tian <kevin.tian@intel.com>,
        Dan Williams <dan.j.williams@intel.com>,
        Logan Gunthorpe <logang@deltatee.com>,
        Ashok Raj <ashok.raj@intel.com>, Jon Mason <jdmason@kudzu.us>,
        Allen Hubbe <allenbh@gmail.com>
Subject: Re: [patch V2 08/21] genirq/msi: Add pointers for per device irq domains
In-Reply-To: <20221121083325.950255253@linutronix.de>
References: <20221121083210.309161925@linutronix.de>
        <20221121083325.950255253@linutronix.de>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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.

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