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

From: Lukas Wunner <lukas@wunner.de>
To: Alex_Gagniuc@Dellteam.com
Cc: mr.nuke.me@gmail.com, bhelgaas@google.com, Austin.Bolen@dell.com,
	keith.busch@intel.com, Shyam.Iyer@dell.com,
	gustavo@embeddedor.com, linux-pci@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] PCI: pciehp: Do not turn off slot if presence comes up after link
Date: Wed, 13 Feb 2019 09:36:32 +0100	[thread overview]
Message-ID: <20190213083632.GA3387@wunner.de> (raw)
In-Reply-To: <b2fffa5beb7941ebaceb8b8ce6e5b133@ausx13mps321.AMER.DELL.COM>

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