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

[Lukas Wunner]

On Tue, Feb 12, 2019 at 11:57:55PM +0000, Alex_Gagniuc@xxxxxxxxxxxx wrote:
> 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

Ok, then I'd suggest to check in pcie_init() whether the feature is
supported and enable it if so.  And store that fact in a flag, either
in struct pci_dev or struct controller.


> (*) 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).


> 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.  This can be achieved by polling PDS in
pcie_wait_for_link() if in-band presence is disabled, which is why
I suggested it.


> > 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.  Polling PDS in
pcie_wait_for_link() or disabling either PDC or DLLSC if in-band presence
is disabled seems simpler to reason about.


> in-band PD disable (what's a good acronym for that, BTW?)

I don't know, maybe inband_presence_disabled?

Thanks,

Lukas