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

From: Lukas Wunner <lukas@wunner.de>
To: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Cc: bhelgaas@google.com, austin_bolen@dell.com,
	alex_gagniuc@dellteam.com, keith.busch@intel.com,
	Shyam_Iyer@Dell.com,
	"Gustavo A. R. Silva" <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: Sat, 9 Feb 2019 12:58:49 +0100	[thread overview]
Message-ID: <20190209115849.244u67h7wmn3eb7o@wunner.de> (raw)
In-Reply-To: <20190205210701.25387-1-mr.nuke.me@gmail.com>

On Tue, Feb 05, 2019 at 03:06:56PM -0600, Alexandru Gagniuc wrote:
> According to PCIe 3.0, the presence detect state is a logical OR of
> in-band and out-of-band presence.

Since Bjorn asked for a spec reference:

PCIe r4.0 sec 7.8.11: "Presence Detect State - This bit indicates the
presence of an adapter in the slot, reflected by the logical OR of the
Physical Layer in-band presence detect mechanism and, if present, any
out-of-band presence detect mechanism defined for the slot's
corresponding form factor."

Table 4-14 in sec 4.2.6 is also important because it shows the difference
between Link Up and in-band presence.

> With this, we'd expect the presence
> state to always be asserted when the link comes up.
> 
> Not all hardware follows this, and it is possible for the presence to
> come up after the link. In this case, the PCIe device would be
> erroneously disabled and re-probed. It is possible to distinguish
> between a delayed presence and a card swap by looking at the DLL state
> changed bit -- The link has to come down if the card is removed.

So in reality, PDC and DLLSC events rarely come in simultaneously and
we do allow some leeway in pcie_wait_for_link(), it's just not enough
in your particular case due to the way presence is detected by the
platform.

I think in this case instead of changing the behavior for everyone,
it's more appropriate to add a quirk for your hardware.  Two ideas:

* Amend pcie_init() to set slot_cap |= PCI_EXP_SLTCAP_ABP.  Insert
  this as a quirk right below the existing Thunderbolt quirk and
  use PCI vendor/device IDs and/or DMI checks to identify your
  particular hardware.  If the ABP bit is set, PDC events are not
  enabled by pcie_enable_notification(), so you will solely rely on
  DLLSC events.  Problem solved.  (Hopefully.)

* Alternatively, add a DECLARE_PCI_FIXUP_FINAL() quirk which sets
  pdev->link_active_reporting = false.  This causes pcie_wait_for_link()
  to wait 1100 msec before the hot-added device's config space is
  accessed for the first time.

Would this work for you?

Thanks,

Lukas