Re: [PATCH v4 3/3] PCI: pciehp: Add dmi table for in-band presence disabled

[Stuart Hayes <stuart.w.hayes@gmail.com>]

On 2/20/20 11:21 PM, Bjorn Helgaas wrote:
> On Fri, Oct 25, 2019 at 03:00:47PM -0400, Stuart Hayes wrote:
>> Some systems have in-band presence detection disabled for hot-plug PCI
>> slots, but do not report this in the slot capabilities 2 (SLTCAP2) register.
> 
> This doesn't seem quite accurate to me.  PCI_EXP_SLTCAP2_IBPD does not
> actually tell us whether in-band presence detection is disabled.  It
> only tells us whether it *can* be disabled.
> 
> I think I know what you mean, but this text suggests that
> PCI_EXP_SLTCAP2_IBPD not being set is the defect, and I don't think it
> is.  IIUC, even if PCI_EXP_SLTCAP2_IBPD were set,
> PCI_EXP_SLTCTL_IBPD_DISABLE would have no effect because in-band
> presence detect just isn't supported at all regardless of how we set
> PCI_EXP_SLTCTL_IBPD_DISABLE.
> 

I agree, the patch notes aren't quite correct.  Many Dell systems use
out-of-band presence detect, and there is no way to enable in-band presence
detection.

So I guess the notes would be more accurate if they said something like this:

Some systems use out-of-band presence detection exclusively, and do not
support the SLTCAP2_IBPD and SLTCTL_IBPD_DISABLE bits, but they still need
to have "inband_presence_disabled" set.

Add a dmi table to flag these systems as having in-band presence disabled.

>> On these systems, presence detect can become active well after the link is
>> reported to be active, which can cause the slots to be disabled after a
>> device is connected.
>>
>> Add a dmi table to flag these systems as having in-band presence disabled.
>>
>> Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
>> ---
>> v4
>>   add comment to dmi table
>>
>>  drivers/pci/hotplug/pciehp_hpc.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
>> index 02d95ab27a12..9541735bd0aa 100644
>> --- a/drivers/pci/hotplug/pciehp_hpc.c
>> +++ b/drivers/pci/hotplug/pciehp_hpc.c
>> @@ -14,6 +14,7 @@
>>  
>>  #define dev_fmt(fmt) "pciehp: " fmt
>>  
>> +#include <linux/dmi.h>
>>  #include <linux/kernel.h>
>>  #include <linux/types.h>
>>  #include <linux/jiffies.h>
>> @@ -26,6 +27,24 @@
>>  #include "../pci.h"
>>  #include "pciehp.h"
>>  
>> +static const struct dmi_system_id inband_presence_disabled_dmi_table[] = {
>> +	/*
>> +	 * Match all Dell systems, as some Dell systems have inband
>> +	 * presence disabled on NVMe slots (but don't support the bit to
> 
> Is there something that restricts these slots to being used only for
> NVMe?  If not, I'd rather simply say that some Root Ports don't
> support in-band presence detect.  You say it's "disabled", which
> suggests that it could be *enabled*.  But I have the impression that
> it's actually just not supported at all (or maybe it's disabled by the
> BIOS via some non-architected mechanism).
> 

These slots are in the backplane that is meant for hard drives, so the
I don't know that there would be any other devices that would fit in them.
But, if some other device did fit physically and had the right connector,
I believe it would work.

I'm not sure if in-band presence detection is disabled by firmware, or
hardwired that way in the circuitry somehow, but you are correct, in-band
presence detect can't be enabled on these systems.

>> +	 * report it). Setting inband presence disabled should have no
>> +	 * negative effect, except on broken hotplug slots that never
>> +	 * assert presence detect--and those will still work, they will
>> +	 * just have a bit of extra delay before being probed.
>> +	 */
>> +	{
>> +		.ident = "Dell System",
>> +		.matches = {
>> +			DMI_MATCH(DMI_OEM_STRING, "Dell System"),
>> +		},
>> +	},
>> +	{}
>> +};
>> +
>>  static inline struct pci_dev *ctrl_dev(struct controller *ctrl)
>>  {
>>  	return ctrl->pcie->port;
>> @@ -895,6 +914,9 @@ struct controller *pcie_init(struct pcie_device *dev)
>>  		ctrl->inband_presence_disabled = 1;
>>  	}
>>  
>> +	if (dmi_first_match(inband_presence_disabled_dmi_table))
>> +		ctrl->inband_presence_disabled = 1;
> 
> This doesn't seem quite right: the DMI table should only apply to
> built-in ports, not to ports on plugin cards.
> 
> If we plug in a switch with hotplug-capable downstream ports, and
> those ports do not advertise PCI_EXP_SLTCAP2_IBPD, I think this code
> sets "inband_presence_disabled" for those ports even though it is not
> disabled.
> 
> IIUC, that will make this plugin card behave differently in a Dell
> system than it will in other systems, and that doesn't seem right to
> me.
> 

Setting "inband_presence_disabled" will only make a hotplug-capable slot
behave differently in two situations:

(1) The slot is using out-of-band presence detection instead of in-band,
in which case the hotplug code will correctly wait for the presence detect
bit to be active (after the link goes active) before proceeding to add the
device.

(2) The slot is broken and the presence detect bit never goes high in spite
of the PCI spec saying "This bit must be implemented on all Downstream Ports
that implement slots" (pcie base spec 5.0r1.0, section 7.5.3.11, slot status
register).  In this case, the hotplug slot will still work when
"inband_presence_disabled" is set, but it will take it a second or so longer
for the device to be added in linux because it will timeout waiting for the
presence detect bit to go active, instead of adding the device as soon as
the link goes active.

Since Dell doesn't support any plugin cards that provide hot plug slots that
are broken in that way, setting "inband_presence_disabled" shouldn't make any
difference.  But even if someone did use a card that was broken in this way,
it would still function.

If you think this is important, I could try to find a way to set
"inband_presence_disabled" just for built-in ports.  I'm not sure it would
be of any practical benefit, though.


>>  	/*
>>  	 * If empty slot's power status is on, turn power off.  The IRQ isn't
>>  	 * requested yet, so avoid triggering a notification with this command.
>> -- 
>> 2.18.1
>>