Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices

[Mika Westerberg]

On Tue, Nov 13, 2018 at 10:56:36AM +0000, Lorenzo Pieralisi wrote:
> On Mon, Nov 12, 2018 at 07:02:03PM +0100, Lukas Wunner wrote:
> > On Mon, Nov 12, 2018 at 07:06:25PM +0300, Mika Westerberg wrote:
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -1378,6 +1378,27 @@ static void set_pcie_thunderbolt(struct pci_dev *dev)
> > >  	}
> > >  }
> > >  
> > > +static void set_pcie_external(struct pci_dev *dev)
> > > +{
> > > +	struct pci_dev *parent;
> > > +
> > > +	/*
> > > +	 * Walk up the device hierarchy and check for any upstream
> > > +	 * bridge that has is_external_facing set to true. This means
> > > +	 * the hierarchy is below PCIe port that is exposed externally
> > > +	 * (such as Thunderbolt).
> > > +	 */
> > > +	parent = pci_upstream_bridge(dev);
> > > +	while (parent) {
> > > +		if (parent->is_external) {
> > > +			dev->is_external = true;
> > > +			break;
> > > +		}
> > > +
> > > +		parent = pci_upstream_bridge(parent);
> > > +	}
> > > +}
> > > +
> > 
> > This looks pretty much like a duplication of the is_thunderbolt bit
> > in struct pci_dev and the pci_is_thunderbolt_attached() helper.
> > 
> > Why constrain the functionality to ports with the _DSD property
> > instead of making it available for *any* Thunderbolt port?
> 
> I assume it is because this is just not needed for Thuderbolt ports but
> rather for all PCIe devices that are "external" (whatever that is
> supposed to mean), ie it is valid also for PCIe slots.

Yes, that was the idea. We could have other "external" devices that are
not using Thunderbolt as the interconnect.

We could do so that we automatically set "is_external" for devices with
"is_thunderbolt" so it should cover those as well.

> To be frank the concept (and Microsoft _DSD bindings) seems a bit vague
> and not thoroughly defined and I would question its detection at
> PCI/ACPI core level, I would hope this can be clarified at ACPI
> specification level, at least.

I guess that is the way they envision to use _DSD. Instead of having
single UUID that covers all properties (like what we have with device
properties) they have one UUID per property "class". I certainly hope we
don't need to keep extending prp_guids[] array each time they invent
another "class" of properties.