On 2/13/19 2:36 AM, Lukas Wunner wrote: >> (*) A bit hypothetical: There is no hardware yet implementing the ECN. > > Hm, this contradicts Austin Bolen's e-mail of Jan 25 that "Yes, this > platform disables in-band presence" (when asked whether your host > controller already adheres to the ECN). Both statements are true. The hardware does indeed disable in-band presence, in a rudimentary way that is not compliant with the ECN -- it doesn't implement the bits required by the ECN. >> I'm >> not sure that there is a spec-justifiable reason to not access a device >> whose DLL is up, but PD isn't. > > Austin asked in an e-mail of Jan 24 whether "the hot-inserted device's > config space [is] accessed immediately after waiting this 20 + 100 ms > delay", which sounded to me like you'd prefer the device not to be > accessed until PDS is 1. "Unless Readiness Notifications mechanisms are used (see Section 6.23), the Root Complex and/or system software must allow at least 1.0 s after a Conventional Reset of a device, before it may determine that a device which fails to return a Successful Completion status for a valid Configuration Request is a broken device. This period is independent of how quickly Link training completes." (Section 6.6) The concern was the one second delay, which is addressed by the use of pci_bus_wait_crs(). >>> 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'm worried that amending pciehp_handle_presence_or_link_change() makes > the event handling logic more difficult to understand. Don't worry. It's already difficult to understand ;) > Polling PDS in > pcie_wait_for_link() or disabling either PDC or DLLSC if in-band presence > is disabled seems simpler to reason about. pcie_wait_for_link() is generic PCIe layer. I don't think mixing hotplug concepts is a good layering violation. Disabling PDC or DLLSC can work. I've sometimes wondered why we even care about PDC. I can imagine situations where platform might want to signal imminent removal by yanking PD. >> in-band PD disable (what's a good acronym for that, BTW?) > > I don't know, maybe inband_presence_disabled? PCI_EXP_SLTCAP2_IBPD ?
