On Thu, Sep 12 2024 at 18:34, Jonathan Cameron wrote:
> On Thu, 12 Sep 2024 17:37:20 +0100
> Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> 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
