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

[Thomas Gleixner <tglx@linutronix.de>]

On Thu, Sep 12 2024 at 18:34, Jonathan Cameron wrote:
> On Thu, 12 Sep 2024 17:37:20 +0100
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
>> One bit that is challenging me is that the PCI access functions
>> use the pci_dev that is embedded via irq_data->common->msi_desc->dev
>> (and hence doesn't give us a obvious path to any hierarchical structures)
>> E.g. __pci_write_msi_msg() and which checks the pci_dev is
>> powered up.
>>
>> I'd like to be able to do a call to the parent similar to
>> irq_chip_unmask_parent to handle write_msi_msg() for the new device
>> domain.

That's what hierarchy is about.  I see your problem vs. the
msi_desc::dev though.

>> So how should this be avoided or should msi_desc->dev be the
>> PCI device?

See below.

> I thought about this whilst cycling home.  Perhaps my fundamental
> question is more basic  Which device should
> msi_dec->dev be?
> * The ultimate requester of the msi  -  so here the one calling
>   our new pci_msix_subdev_alloc_at(),
> * The one on which the msi_write_msg() should occur. - here
>   that would be the struct pci_dev given the checks needed on
>   whether the device is powered up.  If this is the case, can
>   we instead use the irq_data->dev in __pci_write_msi_msg()?

Only for per device MSI domains and you need to look irq data up, so
leave that alone.

> Also, is it valid to use the irq_domain->dev for callbacks such
> as msi_prepare_desc on basis that (I think) is the device
> that 'hosts' the irq domain, so can we use it to replace the
> use of msi_desc->dev in pci_msix_prepare_desc()
> If we can that enables our subdev to use a prepare_desc callback
> that does something like
>
> 	struct msi_domain *info;
>
> 	domain = domain->parent;
> 	info = domain->host_data;
>
> 	info->ops->prepare_desc(domain, arg, desc);

That should work, but you can simply do:

subdev_prepare_desc(domain, arg, desc)
{
        desc->dev = domain->parent->dev;
        pci_msix_prepare_desc(domain, arg, desc);
}

as this is a strict relationship in the PCI code.

This needs a validation that nothing relies on desc->dev being the same
as the device which allocated the descriptor:

  msi_desc_to_pci_dev(), msi_desc_to_dev() and a pile of open coded
  accesses.

The open coded access should be fixed and prevented by marking it
__private so people are forced to use the accessor functions.

If there is no reliance, we can just do that.

I checked the MSI core code already. Nothing to see there.

Thanks,

        tglx