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

From: Alex_Gagniuc
Date: Wed Feb 13 2019 - 13:55:54 EST

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

>>> 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?