Re: [PATCH] PCI: Add sysfs attribute for disabling PCIe link to downstream component

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Aug 20, 2019 at 12:58:20PM +0300, Mika Westerberg wrote:
> On Mon, Aug 19, 2019 at 06:52:45PM -0500, Bjorn Helgaas wrote:
> > > Right, it looks like we need some sort of flag there anyway.
> > 
> > Does this mean you're looking at getting rid of "has_secondary_link",
> > you think it's impossible, or you think it's not worth trying?
> 
> I was of thinking that we need some flag anyway for the downstream port
> (such as has_secondary_link) that tells us the which side of the port
> the link is.
> 
> > I'm pretty sure we could get rid of it by looking upstream, but I
> > haven't actually tried it.
> 
> So if we are downstream port, look at the parent and if it is also
> downstream port (or root port) we change the type to upstream port
> accordingly? That might work.

If we see a type of PCI_EXP_TYPE_ROOT_PORT or
PCI_EXP_TYPE_PCIE_BRIDGE, I think we have to assume that's accurate
(which we already do today -- for those types, we assume the device
has a secondary link).

For a device that claims to be PCI_EXP_TYPE_DOWNSTREAM, if a parent
device exists and is a Downstream Port (Root Port, Switch Downstream
Port, and I suppose a PCI-to-PCIe bridge (this is basically
pcie_downstream_port()), this device must actually be acting as a
PCI_EXP_TYPE_UPSTREAM device.

If a device claiming to be PCI_EXP_TYPE_UPSTREAM has a parent that is
PCI_EXP_TYPE_UPSTREAM, this device must actually be a
PCI_EXP_TYPE_DOWNSTREAM port.

For PCI_EXP_TYPE_DOWNSTREAM and PCI_EXP_TYPE_UPSTREAM devices that
don't have parents, we just have to assume they advertise the correct
type (as we do today).  There are sparc and virtualization configs
like this.

> Another option may be to just add a quirk for these ports.

I don't really like the quirk approach because then we have to rely on
user reports of something being broken.

> Only concern for both is that we have functions that rely on the type
> such as pcie_capability_read_word() so if we change the type do we end
> up breaking something? I did not check too closely, though.

I don't think we'll break anything that's not already broken because
the type will reflect exactly what has_secondary_link now tells us.
In fact, we might *fix* some things, e.g., pcie_capability_read_word()
should work better if we fix the type that pcie_downstream_port()
checks.

> I'm willing to cook a patch that fixes this once we have some consensus
> what it should do ;-)