Re: [PATCH RFC v2 4/4] PCI: hotplug: Add quirk For Dell nvme pcie switches

From: <Alex_Gagniuc@Dellteam.com>
To: <lukas@wunner.de>, <mr.nuke.me@gmail.com>
Cc: <bhelgaas@google.com>, <Austin.Bolen@dell.com>,
	<keith.busch@intel.com>, <Shyam.Iyer@dell.com>,
	<okaya@kernel.org>, <linux-pci@vger.kernel.org>,
	<mika.westerberg@linux.intel.com>, <okaya@codeaurora.org>,
	<poza@codeaurora.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH RFC v2 4/4] PCI: hotplug: Add quirk For Dell nvme pcie switches
Date: Thu, 21 Feb 2019 18:35:12 +0000	[thread overview]
Message-ID: <027101b4652c4e0c98b86649aa3e4732@ausx13mps321.AMER.DELL.COM> (raw)
In-Reply-To: 20190221075634.uih5ysqwkna7cku4@wunner.de

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