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

[Alex_Gagniuc]

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 ?