Re: [PATCH] PCI: pciehp: Don't enable slot unless link or presence up

[Lukas Wunner <lukas@wunner.de>]

On Wed, Jul 08, 2020 at 05:39:49PM -0500, Stuart Hayes wrote:
> It's more than just the "Link Up" message.  When I "power down" an NVMe
> drive via sysfs (on a system that doesn't actually shut off the power to
> the slot), and then I physically remove the drive, I get this ugly string
> of messages:
> 
> pcieport 0000:3c:06.0: pciehp: Slot(180): Link Up
> pcieport 0000:3c:06.0: pciehp: Timeout waiting for Presence Detect
> pcieport 0000:3c:06.0: pciehp: link training error: status 0x0001
> pcieport 0000:3c:06.0: pciehp: Failed to check link status

I was just wondering if those messages could be avoided by setting
the Link Disable bit in the Link Control register if the slot
is brought down via sysfs.  The question is, if a new NVMe
drive is subsequently inserted into the slot, does the hotplug
controller signal a DLLSC event even though Link Disable is set?
Or at least a PDC event, whereupon we could clear Link Disable?

Or is there some other way to force the link into a state such that
it is off but DLLSC events continue to be received?

There's a function pciehp_link_enable() which can set or clear the
Link Disable flag.  But it's only ever invoked to clear it.  Hm, digging
in the git history shows that between 2012 and 2014 it was used to set
Link Disable when bringing down the slot, but b1811d2455f3 dropped that
because DLLSC was no longer received.

I guess we might be able to set Link Disable, then clear it once the
PDC event is received upon removal of the NVMe drive from the slot.
But I imagine that would complicate the state machine quite a bit.

Another idea:  If the hotplug controller is suspended to D3hot,
the link should be down but presence or link events should still
be received.  Sadly, runtime D3hot is disabled by default for
hotplug ports because it is known to not work on certain SkyLake
Xeon-SP systems and possibly others (see eb3b5bf1a88d).  You can
force it on by passing pcie_port_pm=force on the command line.


> Would you consider the patch if I rework it to separate the
> BLINKINGON_STATE so that its behavior isn't changed, and change the
> fake PDC events to PDC | DLLSC?

TBH I'd suggest to simply drop some of the messages and tone down the
remaining ones, in particular:

* Drop the "Timeout waiting for Presence Detect" message in
  pcie_wait_for_presence().  The sole caller of that function,
  pciehp_check_link_status(), doesn't bail out if it fails
  but continues and will emit an error message of its own.
  I don't think the message provides much additional value.

* Tone down the "link training error" message in
  pciehp_check_link_status() to ctrl_info().  That message at
  least prints the contents of the Link Status register, which
  may have some value.

* Add a message with ctrl_info() severity in the "if (!found)"
  clause in pciehp_check_link_status().  That way the function
  emits a message in all error cases.

* This in turn allows dropping the "Failed to check link status"
  message in board_added().

As a result you should just get two messages with KERN_INFO severity
which simply serve as an indication that DLLLA and PDS did not change
in unison.

With Thunderbolt, unplugging a daisy chain of devices always causes
a bunch of harmless "no response from device" messages with KERN_INFO
severity.  We never know if there's a real problem or not, so we try
to alert the user that there *may* be a problem, but at the same time
we should try to keep noisiness at a minimum.

Thanks,

Lukas