On 2/12/19 2:30 AM, Lukas Wunner wrote: > The PCI SIG > 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. Alex
