| Message ID | 20190220012031.10741-5-mr.nuke.me@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | PCI: pciehp: Do not turn off slot if presence comes up after link | expand |
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, Alex_Gagniuc@Dellteam.com 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 <Alex_Gagniuc@dellteam.com> 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 <Alex_Gagniuc@dellteam.com> 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
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);
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 <mr.nuke.me@gmail.com> --- drivers/pci/hotplug/pciehp_hpc.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)