Re: [RFC PATCH 0/9] pci: portdrv: Add auxiliary bus and register CXL PMUs (and aer)

[Thomas Gleixner <tglx@linutronix.de>]

On Tue, Sep 10 2024 at 17:47, Jonathan Cameron wrote:
> There are quite a few places that make the assumption that the
> preirq_data->parent->chip is the right chip to for example call
> irq_set_affinity on.
>
> So the simple way to make it all work is to just have
> a msi_domain_template->msi_domain_ops->prepare_desc
> that sets the desc->dev = to the parent device (here the
> pci_dev->dev)

Creative :)

> At that point everything more or less just works and all the
> rest of the callbacks can use generic forms.
>
> Alternative might be to make calls like the one in
> arch/x86/kernel/apic/msi.c msi_set_affinity search
> for the first ancestor device that has an irq_set_affinity().
> That unfortunately may affect quite a few places.

What's the problem? None of the CXL drivers should care about that at
all. Delegating other callbacks to the parent domain is what hierarchical
domains are about. If there is some helper missing, so we add it.

> Anyhow, I'll probably send out the prepare_desc hack set with driver
> usage etc after I've written up a proper description of problems
> encountered etc so you can see what it all looks like and will be more
> palatable in general but in the meantime this is the guts of it of the
> variant where the subdev related desc has the dev set to the parent
> device.

Let me look at this pile then :)

> Note for the avoidance of doubt, I don't much like this
> solution but maybe it will grow on me ;)

Maybe, but I doubt it will survive my abstraction taste check.

> +static const struct msi_parent_ops pci_msix_parent_ops = {
> +	.supported_flags = MSI_FLAG_PCI_MSIX | MSI_FLAG_PCI_MSIX_ALLOC_DYN,
> +	.prefix = "PCI-MSIX-PROXY-",
> +	.init_dev_msi_info = msi_parent_init_dev_msi_info,

The obligatory link to:

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers

> +};
> +
> +int pci_msi_enable_parent_domain(struct pci_dev *pdev)
> +{
> +	struct irq_domain *msix_dom;
> +	/* TGLX: Validate has v2 parent domain */
> +	/* TGLX: validate msix enabled */
> +	/* TGLX: Validate msix domain supports dynamics msi-x */
> +
> +	/* Enable PCI device parent domain */
> +	msix_dom = dev_msi_get_msix_device_domain(&pdev->dev);
> +	msix_dom->flags |= IRQ_DOMAIN_FLAG_MSI_PARENT;
> +	msix_dom->msi_parent_ops = &pci_msix_parent_ops;

That want's to be a call into the MSI core code, Something like:

     msi_enable_sub_parent_domain(&pdev->dev, DOMAIN_BUS_PCI_DEVICE_MSIX)

or something like that. I really don't want to expose the internals of
MSI. I spent a lot of effort to encapsulate that...

> +void pci_msi_init_subdevice(struct pci_dev *pdev, struct device *dev)
> +{
> +	dev_set_msi_domain(dev, dev_msi_get_msix_device_domain(&pdev->dev));

     msi_set_parent_domain(.....);

> +static void pci_subdev_msix_prepare_desc(struct irq_domain *domain, msi_alloc_info_t *arg,
> +					struct msi_desc *desc)
> +{
> +	struct device *parent = desc->dev->parent;
> +
> +	if (!desc->pci.mask_base) {
> +		/* Not elegant - but needed for irq affinity to work? */

Which makes me ask the question why desc->pci.mask_base is NULL in the
first place. That needs some thought at the place which initializes the
descriptor.

> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -1720,3 +1720,8 @@ bool msi_device_has_isolated_msi(struct device *dev)
>  	return arch_is_isolated_msi();
>  }
>  EXPORT_SYMBOL_GPL(msi_device_has_isolated_msi);
> +struct irq_domain *dev_msi_get_msix_device_domain(struct device *dev)

You clearly run out of new lines here :)

> +{
> +	return dev->msi.data->__domains[0].domain;
> +}

But aside of that. No. See above.

Thanks,

        tglx