Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
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>
From:   <Alex_Gagniuc@Dellteam.com>
To:     <lukas@wunner.de>, <mr.nuke.me@gmail.com>
Subject: Re: [PATCH RFC v2 4/4] PCI: hotplug: Add quirk For Dell nvme pcie
 switches
Thread-Topic: [PATCH RFC v2 4/4] PCI: hotplug: Add quirk For Dell nvme pcie
 switches
Thread-Index: AQHUyLqRKxehBZutlk6MputMunrGNg==
Date:   Thu, 21 Feb 2019 18:35:12 +0000
Message-ID: <027101b4652c4e0c98b86649aa3e4732@ausx13mps321.AMER.DELL.COM>
References: <20190220012031.10741-1-mr.nuke.me@gmail.com>
 <20190220012031.10741-5-mr.nuke.me@gmail.com>
 <20190221075634.uih5ysqwkna7cku4@wunner.de>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 2/21/19 1:57 AM, Lukas Wunner wrote:=0A=
> =0A=
> [EXTERNAL EMAIL]=0A=
> =0A=
> On Tue, Feb 19, 2019 at 07:20:30PM -0600, Alexandru Gagniuc wrote:=0A=
>> --- a/drivers/pci/hotplug/pciehp_hpc.c=0A=
>> +++ b/drivers/pci/hotplug/pciehp_hpc.c=0A=
>> @@ -952,3 +952,23 @@ DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_QCOM, 0=
x0400,=0A=
>>   			      PCI_CLASS_BRIDGE_PCI, 8, quirk_cmd_compl);=0A=
>>   DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_QCOM, 0x0401,=0A=
>>   			      PCI_CLASS_BRIDGE_PCI, 8, quirk_cmd_compl);=0A=
>> +=0A=
>> +=0A=
> =0A=
> Duplicate newline.=0A=
> =0A=
> =0A=
>> +static void fixup_dell_nvme_backplane_switches(struct pci_dev *pdev)=0A=
> =0A=
> Can we have a little code comment above the function such as:=0A=
> =0A=
> +/*=0A=
> + * Dell <product name> NVMe storage backplanes disable in-band presence=
=0A=
> + * (PCIe r5.0 sec X.Y.Z) but neglect to set the corresponding flag in th=
e=0A=
> + * Slot Capabilities 2 register.=0A=
> + */=0A=
> =0A=
> =0A=
>> +	if (pdev->subsystem_vendor !=3D PCI_VENDOR_ID_DELL=0A=
>> +		|| pdev->subsystem_device !=3D 0x1fc7)=0A=
> =0A=
> This looks a little unpolished, how about:=0A=
> =0A=
> +	if (pdev->subsystem_vendor !=3D PCI_VENDOR_ID_DELL ||=0A=
> +	    pdev->subsystem_device !=3D 0x1fc7)=0A=
> =0A=
> =0A=
>> +		return;=0A=
>> +=0A=
>> +	pdev->no_in_band_presence =3D 1;=0A=
>> +}=0A=
>> +=0A=
>> +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_PLX, 0x9733,=0A=
> =0A=
> By convention there's no blank line between the closing curly brace=0A=
> and the DECLARE_PCI_FIXUP_CLASS_FINAL().=0A=
=0A=
I'm sorry for all the style issues. I realize it's noise and should just =
=0A=
be done right from the beginning. Is there a way to make checkpatch.pl =0A=
catch these before they go out?=0A=
=0A=
=0A=
> If the quirk is x86-specific, please enclose it in "#ifdef CONFIG_X86"=0A=
> to reduce kernel footprint on other arches.=0A=
=0A=
That's a tricky one. If you look at p. 185 of [1], items 9, 11, and 12 =0A=
are standard x16 cards that would fit in any x16 slot. Those cards have =0A=
the offending switches.=0A=
=0A=
On the one hand, you could take the cards and backplane and put them in =0A=
a non-hax86 system. On the other hand, I don't see why someone would =0A=
want to do this.=0A=
=0A=
Alex=0A=
=0A=
[1] https://topics-cdn.dell.com/pdf/poweredge-r740xd_owners-manual_en-us.pd=
f=0A=
=0A=
=0A=