These switches are used to fornicate the motherboard's x16 PCIe ports
into four x4 ports for NVMe drives. In conjunction with the storage
backplane, the PDS bit reports only the out-of-band presence. The fact
that inband presence is disabled is not reported in the slot
capabilities 2 (SLTCAP2) register.
Because this does not conform to the PCIe spec, add a quirk to let
hotplug code know to expect and handle this.
Signed-off-by: Alexandru Gagniuc <[email protected]>
---
drivers/pci/hotplug/pciehp_hpc.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 9bcadb5d3561..853fb4ab53de 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -952,3 +952,23 @@ DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_QCOM, 0x0400,
PCI_CLASS_BRIDGE_PCI, 8, quirk_cmd_compl);
DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_QCOM, 0x0401,
PCI_CLASS_BRIDGE_PCI, 8, quirk_cmd_compl);
+
+
+static void fixup_dell_nvme_backplane_switches(struct pci_dev *pdev)
+{
+ if (!pci_is_pcie(pdev))
+ return;
+
+ if (pci_pcie_type(pdev) != PCI_EXP_TYPE_DOWNSTREAM)
+ return;
+
+ if (pdev->subsystem_vendor != PCI_VENDOR_ID_DELL
+ || pdev->subsystem_device != 0x1fc7)
+ return;
+
+ pdev->no_in_band_presence = 1;
+}
+
+DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_PLX, 0x9733,
+ PCI_CLASS_BRIDGE_PCI, 8,
+ fixup_dell_nvme_backplane_switches);
--
2.19.2
On Tue, Feb 19, 2019 at 07:20:30PM -0600, Alexandru Gagniuc wrote:
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -952,3 +952,23 @@ DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_QCOM, 0x0400,
> PCI_CLASS_BRIDGE_PCI, 8, quirk_cmd_compl);
> DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_QCOM, 0x0401,
> PCI_CLASS_BRIDGE_PCI, 8, quirk_cmd_compl);
> +
> +
Duplicate newline.
> +static void fixup_dell_nvme_backplane_switches(struct pci_dev *pdev)
Can we have a little code comment above the function such as:
+/*
+ * Dell <product name> NVMe storage backplanes disable in-band presence
+ * (PCIe r5.0 sec X.Y.Z) but neglect to set the corresponding flag in the
+ * Slot Capabilities 2 register.
+ */
> + if (pdev->subsystem_vendor != PCI_VENDOR_ID_DELL
> + || pdev->subsystem_device != 0x1fc7)
This looks a little unpolished, how about:
+ if (pdev->subsystem_vendor != PCI_VENDOR_ID_DELL ||
+ pdev->subsystem_device != 0x1fc7)
> + return;
> +
> + pdev->no_in_band_presence = 1;
> +}
> +
> +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_PLX, 0x9733,
By convention there's no blank line between the closing curly brace
and the DECLARE_PCI_FIXUP_CLASS_FINAL().
If the quirk is x86-specific, please enclose it in "#ifdef CONFIG_X86"
to reduce kernel footprint on other arches.
Thanks,
Lukas
On 2/21/19 1:57 AM, Lukas Wunner wrote:
>
> [EXTERNAL EMAIL]
>
> On Tue, Feb 19, 2019 at 07:20:30PM -0600, Alexandru Gagniuc wrote:
>> --- a/drivers/pci/hotplug/pciehp_hpc.c
>> +++ b/drivers/pci/hotplug/pciehp_hpc.c
>> @@ -952,3 +952,23 @@ DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_QCOM, 0x0400,
>> PCI_CLASS_BRIDGE_PCI, 8, quirk_cmd_compl);
>> DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_QCOM, 0x0401,
>> PCI_CLASS_BRIDGE_PCI, 8, quirk_cmd_compl);
>> +
>> +
>
> Duplicate newline.
>
>
>> +static void fixup_dell_nvme_backplane_switches(struct pci_dev *pdev)
>
> Can we have a little code comment above the function such as:
>
> +/*
> + * Dell <product name> NVMe storage backplanes disable in-band presence
> + * (PCIe r5.0 sec X.Y.Z) but neglect to set the corresponding flag in the
> + * Slot Capabilities 2 register.
> + */
>
>
>> + if (pdev->subsystem_vendor != PCI_VENDOR_ID_DELL
>> + || pdev->subsystem_device != 0x1fc7)
>
> This looks a little unpolished, how about:
>
> + if (pdev->subsystem_vendor != PCI_VENDOR_ID_DELL ||
> + pdev->subsystem_device != 0x1fc7)
>
>
>> + return;
>> +
>> + pdev->no_in_band_presence = 1;
>> +}
>> +
>> +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_PLX, 0x9733,
>
> By convention there's no blank line between the closing curly brace
> and the DECLARE_PCI_FIXUP_CLASS_FINAL().
I'm sorry for all the style issues. I realize it's noise and should just
be done right from the beginning. Is there a way to make checkpatch.pl
catch these before they go out?
> If the quirk is x86-specific, please enclose it in "#ifdef CONFIG_X86"
> to reduce kernel footprint on other arches.
That's a tricky one. If you look at p. 185 of [1], items 9, 11, and 12
are standard x16 cards that would fit in any x16 slot. Those cards have
the offending switches.
On the one hand, you could take the cards and backplane and put them in
a non-hax86 system. On the other hand, I don't see why someone would
want to do this.
Alex
[1] https://topics-cdn.dell.com/pdf/poweredge-r740xd_owners-manual_en-us.pdf
On Thu, 2019-02-21 at 18:35 +0000, [email protected] wrote:
> On 2/21/19 1:57 AM, Lukas Wunner wrote:
> > On Tue, Feb 19, 2019 at 07:20:30PM -0600, Alexandru Gagniuc wrote:
> > > --- a/drivers/pci/hotplug/pciehp_hpc.c
> > >
> > > +}
> > > +
> > > +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_PLX, 0x9733,
> >
> > By convention there's no blank line between the closing curly brace
> > and the DECLARE_PCI_FIXUP_CLASS_FINAL().
Not completely true.
See: arch/mips/pci/fixup-loongson3.c
> I'm sorry for all the style issues. I realize it's noise and should just
> be done right from the beginning. Is there a way to make checkpatch.pl
> catch these before they go out?
You could write a new rule, but it's non trivial as
there are multiple consecutive uses and multi-line
uses too.
I think it's a rule not worth writing.
On Fri, Feb 22, 2019 at 5:38 AM <[email protected]> wrote:
>
> On 2/21/19 1:57 AM, Lukas Wunner wrote:
> >
> > [EXTERNAL EMAIL]
> >
> > On Tue, Feb 19, 2019 at 07:20:30PM -0600, Alexandru Gagniuc wrote:
> >> --- a/drivers/pci/hotplug/pciehp_hpc.c
> >> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> >> @@ -952,3 +952,23 @@ DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_QCOM, 0x0400,
> >> PCI_CLASS_BRIDGE_PCI, 8, quirk_cmd_compl);
> >> DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_QCOM, 0x0401,
> >> PCI_CLASS_BRIDGE_PCI, 8, quirk_cmd_compl);
> >> +
> >> +
> >
> > Duplicate newline.
> >
> >
> >> +static void fixup_dell_nvme_backplane_switches(struct pci_dev *pdev)
> >
> > Can we have a little code comment above the function such as:
> >
> > +/*
> > + * Dell <product name> NVMe storage backplanes disable in-band presence
> > + * (PCIe r5.0 sec X.Y.Z) but neglect to set the corresponding flag in the
> > + * Slot Capabilities 2 register.
> > + */
> >
> >
> >> + if (pdev->subsystem_vendor != PCI_VENDOR_ID_DELL
> >> + || pdev->subsystem_device != 0x1fc7)
> >
> > This looks a little unpolished, how about:
> >
> > + if (pdev->subsystem_vendor != PCI_VENDOR_ID_DELL ||
> > + pdev->subsystem_device != 0x1fc7)
> >
> >
> >> + return;
> >> +
> >> + pdev->no_in_band_presence = 1;
> >> +}
> >> +
> >> +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_PLX, 0x9733,
> >
> > By convention there's no blank line between the closing curly brace
> > and the DECLARE_PCI_FIXUP_CLASS_FINAL().
>
> I'm sorry for all the style issues. I realize it's noise and should just
> be done right from the beginning. Is there a way to make checkpatch.pl
> catch these before they go out?
>
>
> > If the quirk is x86-specific, please enclose it in "#ifdef CONFIG_X86"
> > to reduce kernel footprint on other arches.
>
> That's a tricky one. If you look at p. 185 of [1], items 9, 11, and 12
> are standard x16 cards that would fit in any x16 slot. Those cards have
> the offending switches.
>
> On the one hand, you could take the cards and backplane and put them in
> a non-hax86 system. On the other hand, I don't see why someone would
> want to do this.
I have a couple of POWER boxes with Dell branded switch cards in them.
I have no idea why either, but it does happen.
>
> Alex
>
> [1] https://topics-cdn.dell.com/pdf/poweredge-r740xd_owners-manual_en-us.pdf
>
>
On 2/21/19 8:05 PM, Oliver wrote:
> On Fri, Feb 22, 2019 at 5:38 AM <[email protected]> wrote:
>> On 2/21/19 1:57 AM, Lukas Wunner wrote:
[snip]
>>> If the quirk is x86-specific, please enclose it in "#ifdef CONFIG_X86"
>>> to reduce kernel footprint on other arches.
>>
>> That's a tricky one. If you look at p. 185 of [1], items 9, 11, and 12
>> are standard x16 cards that would fit in any x16 slot. Those cards have
>> the offending switches.
>>
>> On the one hand, you could take the cards and backplane and put them in
>> a non-hax86 system. On the other hand, I don't see why someone would
>> want to do this.
>
> I have a couple of POWER boxes with Dell branded switch cards in them.
> I have no idea why either, but it does happen.
The hardware debouncer, I think, is on the backplane, so you'd really
need both the switch and backplane combo. I've seen other marketing
departments call the switches "NVMe HBA".
Alex