[PATCH v10 4/6] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05

[arnd@arndb.de (Arnd Bergmann)]

On Tuesday 13 October 2015 14:49:07 Gabriele Paoloni wrote:
> > On Monday 12 October 2015 16:35:45 Bjorn Helgaas wrote:
> > >
> > > > +{
> > > > +     u64 addr;
> > > > +     struct device_node *msi_node;
> > > > +     struct resource res;
> > > > +     struct device_node *np = pp->dev->of_node;
> > > > +     struct hisi_pcie *pcie = to_hisi_pcie(pp);
> > > > +
> > > > +     msi_node = of_parse_phandle(np, "msi-parent", 0);
> > > > +     if (!msi_node) {
> > > > +             dev_err(pp->dev, "failed to find msi-parent\n");
> > > > +             return -EINVAL;
> > > > +     }
> > > > +     of_address_to_resource(msi_node, 0, &res);
> > >
> > > Does this use the "msi-parent" node in the same way as other drivers
> > > do?  I'm sure there must be other places where we extract struct
> > > resource information from an "msi-parent" node, but I don't see them.
> > >
> > > I'm trying to verify that this isn't some kind of incompatible
> > > extension of the "msi-parent" property.  I cc'd Arnd and Rob (DT
> > > experts).
> > 
> > This is not ok, what this does is that it relies on a particular
> > implementation of the MSI controller and directly accesses its
> > registers.
> 
> Hi Arnd, thanks for reviewing.
> 
> What we do is to retrieve the msi-parent physical address and we store it
> in our internal PCIe register locations...

Ah, thanks for the clarification, I missed that part.

> So we do not operate directly on the msi controller registers...
> 
> So I wonder if the current implementation is Ok to retrieve the
> msi-parent address....

Not sure. Why do you do this? Normally the PCI host does not know
or care about the address of the MSI controller, because the messages
are just passed up to the parent bus as DMA transfers.

I don't think what you do here is safe because the 'reg' property
of the MSI controller might point to the address that is used for
the message directly.

	Arnd