Re: [PATCH] PCI: pciehp: Do not turn off slot if presence comes up after link

From: Alex_Gagniuc
Date: Tue Feb 12 2019 - 19:07:30 EST

On 2/12/19 2:30 AM, Lukas Wunner wrote:
> should probably consider granting access to specs to open source
> developers to ease adoption of new features in Linux et al.

Don't get me started on just how ridiculous I think PCI-SIG's policy is
with respect to public availability of standards.

>>> Table 4-14 in sec 4.2.6 is also important because it shows the difference
>>> between Link Up and in-band presence.
>> So, we'd expect PD to come up at the same time or before DLL?
> Per table 4-14 and figure 4-23 (immediately below the table) in r4.0,
> PDC ought to be signaled before DLLSC. As said, Linux can handle PDC
> after DLLSC if they're not too far apart (100 ms, see pcie_wait_for_link()).
>> I'd like a generic solution. I suspect supporting 'in-band PD disabled'
>> and quirking for that would be a saner way to do things. If we support
>> it, then we need to handle PDSC after DLLSC scenarios anyway.
> I agree, having support for this new ECN would be great.
> If you look at struct controller in drivers/pci/hotplug/pciehp.h,
> there's a section labelled /* capabilities and quirks */. An idea
> would be to add an unsigned int there to indicate whether in-band
> presence is disabled. An alternative would be to add a flag to
> struct pci_dev.

> Instead of modifying the logic in pciehp_handle_presence_or_link_change(),
> you could amend pcie_wait_for_link() to poll PDS until it's set, in
> addition to DLLLA. The rationale would be that although the link is up,
> the hot-added device cannot really be considered accessible until PDS
> is also set. Unfortunately we cannot do this for all devices because
> (as I've said before), some broken devices hardwire PDS to zero. But
> it should be safe to constrain it to those which can disable in-band
> presence.

The recommendation is to set the "in-band PD disable" bit, and it's
possible that platform firmware may have set it at boot time (*). I'm
not sure that there is a spec-justifiable reason to not access a device
whose DLL is up, but PD isn't.

(*) A bit hypothetical: There is no hardware yet implementing the ECN.

> Be mindful however that pcie_wait_for_link() is also called from the
> DPC driver. Keith should be able to judge whether a change to that
> function breaks DPC.

That's why I went for ammending pciehp_handle_presence_or_link_change().
Smaller bug surface ;). What I'm thinking at this point is, keep the
patch as is, but, also check for in-band PD disable before aborting the
shutdown. Old behavior stays the same.

I'll rework this a little bit for in-band PD disable (what's a good
acronym for that, BTW?), and then quirk those machines that are known to
'disable' this in hardware.