Re: [PATCH RFC v2 4/4] PCI: hotplug: Add quirk For Dell nvme pcie switches
On 2/21/19 8:05 PM, Oliver wrote: > On Fri, Feb 22, 2019 at 5:38 AM 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
Re: [PATCH RFC v2 4/4] PCI: hotplug: Add quirk For Dell nvme pcie switches
On Fri, Feb 22, 2019 at 5:38 AM 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 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 > >
Re: [PATCH RFC v2 4/4] PCI: hotplug: Add quirk For Dell nvme pcie switches
On Thu, 2019-02-21 at 18:35 +, alex_gagn...@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.
Re: [PATCH RFC v2 4/4] PCI: hotplug: Add quirk For Dell nvme pcie switches
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 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
Re: [PATCH RFC v2 4/4] PCI: hotplug: Add quirk For Dell nvme pcie switches
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 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
