[PATCH RFC v2 1/4] PCI: hotplug: Add support for disabling in-band presence

[PATCH RFC v2 1/4] PCI: hotplug: Add support for disabling in-band presence

[Alexandru Gagniuc]

The presence detect state (PDS) is normally a logical or of in-band
and out-of-band presence. In PCIe 4.0, there is the option to disable
in-band presence so that the PDS bit always reflects the state of the
out-of-band presence.

The recommendation of the PCIe spec is to disable in-band presence
whenever supported.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 drivers/pci/hotplug/pciehp.h     |  1 +
 drivers/pci/hotplug/pciehp_hpc.c | 12 +++++++++++-
 include/linux/pci.h              |  1 +
 include/uapi/linux/pci_regs.h    |  2 ++
 4 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 506e1d923a1f..6f729ce4a7b9 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -98,6 +98,7 @@ struct controller {
 	struct pcie_device *pcie;
 
 	u32 slot_cap;				/* capabilities and quirks */
+	unsigned int inband_presence_disabled:1;
 
 	u16 slot_ctrl;				/* control register access */
 	struct mutex ctrl_lock;
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 7dd443aea5a5..f77dc7c38f9a 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -824,7 +824,7 @@ static inline void dbg_ctrl(struct controller *ctrl)
 struct controller *pcie_init(struct pcie_device *dev)
 {
 	struct controller *ctrl;
-	u32 slot_cap, link_cap;
+	u32 slot_cap, slot_cap2, link_cap;
 	u8 poweron;
 	struct pci_dev *pdev = dev->port;
 	struct pci_bus *subordinate = pdev->subordinate;
@@ -846,6 +846,9 @@ struct controller *pcie_init(struct pcie_device *dev)
 	if (pdev->is_thunderbolt)
 		slot_cap |= PCI_EXP_SLTCAP_NCCS;
 
+	if (pdev->no_in_band_presence)
+		ctrl->inband_presence_disabled = 1;
+
 	ctrl->slot_cap = slot_cap;
 	mutex_init(&ctrl->ctrl_lock);
 	mutex_init(&ctrl->state_lock);
@@ -882,6 +885,13 @@ struct controller *pcie_init(struct pcie_device *dev)
 		FLAG(link_cap, PCI_EXP_LNKCAP_DLLLARC),
 		pdev->broken_cmd_compl ? " (with Cmd Compl erratum)" : "");
 
+	pcie_capability_read_dword(pdev, PCI_EXP_SLTCAP2, &slot_cap2);
+	if (slot_cap2 & PCI_EXP_SLTCAP2_IBPD) {
+		pcie_write_cmd_nowait(ctrl, PCI_EXP_SLTCTL_IBPD_DISABLE,
+				      PCI_EXP_SLTCTL_IBPD_DISABLE);
+		ctrl->inband_presence_disabled = 1;
+	}
+
 	/*
 	 * If empty slot's power status is on, turn power off.  The IRQ isn't
 	 * requested yet, so avoid triggering a notification with this command.
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 65f1d8c2f082..9d08cdbca459 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -413,6 +413,7 @@ struct pci_dev {
 	unsigned int	non_compliant_bars:1;	/* Broken BARs; ignore them */
 	unsigned int	is_probed:1;		/* Device probing in progress */
 	unsigned int	link_active_reporting:1;/* Device capable of reporting link active */
+	unsigned int	no_in_band_presence:1;	/* Device does not report in-band presence */
 	unsigned int	no_vf_scan:1;		/* Don't scan for VFs after IOV enablement */
 	pci_dev_flags_t dev_flags;
 	atomic_t	enable_cnt;	/* pci_enable_device has been called */
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index e1e9888c85e6..5423dc476c77 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -597,6 +597,7 @@
 #define  PCI_EXP_SLTCTL_PWR_OFF        0x0400 /* Power Off */
 #define  PCI_EXP_SLTCTL_EIC	0x0800	/* Electromechanical Interlock Control */
 #define  PCI_EXP_SLTCTL_DLLSCE	0x1000	/* Data Link Layer State Changed Enable */
+#define  PCI_EXP_SLTCTL_IBPD_DISABLE	0x4000 /* In-band PD disable */
 #define PCI_EXP_SLTSTA		26	/* Slot Status */
 #define  PCI_EXP_SLTSTA_ABP	0x0001	/* Attention Button Pressed */
 #define  PCI_EXP_SLTSTA_PFD	0x0002	/* Power Fault Detected */
@@ -667,6 +668,7 @@
 #define PCI_EXP_LNKSTA2		50	/* Link Status 2 */
 #define PCI_CAP_EXP_ENDPOINT_SIZEOF_V2	52	/* v2 endpoints with link end here */
 #define PCI_EXP_SLTCAP2		52	/* Slot Capabilities 2 */
+#define  PCI_EXP_SLTCAP2_IBPD	0x0001	/* In-band PD Disable Supported */
 #define PCI_EXP_SLTCTL2		56	/* Slot Control 2 */
 #define PCI_EXP_SLTSTA2		58	/* Slot Status 2 */
 
-- 
2.19.2

Re: [PATCH RFC v2 1/4] PCI: hotplug: Add support for disabling in-band presence

[Lukas Wunner]

On Tue, Feb 19, 2019 at 07:20:27PM -0600, Alexandru Gagniuc wrote:
> @@ -846,6 +846,9 @@ struct controller *pcie_init(struct pcie_device *dev)
>  	if (pdev->is_thunderbolt)
>  		slot_cap |= PCI_EXP_SLTCAP_NCCS;
>  
> +	if (pdev->no_in_band_presence)
> +		ctrl->inband_presence_disabled = 1;
> +
>  	ctrl->slot_cap = slot_cap;
>  	mutex_init(&ctrl->ctrl_lock);
>  	mutex_init(&ctrl->state_lock);

The above hunk belongs in patch 4.


> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -413,6 +413,7 @@ struct pci_dev {
>  	unsigned int	non_compliant_bars:1;	/* Broken BARs; ignore them */
>  	unsigned int	is_probed:1;		/* Device probing in progress */
>  	unsigned int	link_active_reporting:1;/* Device capable of reporting link active */
> +	unsigned int	no_in_band_presence:1;	/* Device does not report in-band presence */
>  	unsigned int	no_vf_scan:1;		/* Don't scan for VFs after IOV enablement */
>  	pci_dev_flags_t dev_flags;
>  	atomic_t	enable_cnt;	/* pci_enable_device has been called */

Same here.

Thanks,

Lukas

Re: [PATCH RFC v2 1/4] PCI: hotplug: Add support for disabling in-band presence

[Alex_Gagniuc]

On 2/21/19 1:20 AM, Lukas Wunner wrote:
> 
> [EXTERNAL EMAIL]
> 
> On Tue, Feb 19, 2019 at 07:20:27PM -0600, Alexandru Gagniuc wrote:
>> @@ -846,6 +846,9 @@ struct controller *pcie_init(struct pcie_device *dev)
>>   	if (pdev->is_thunderbolt)
>>   		slot_cap |= PCI_EXP_SLTCAP_NCCS;
>>   
>> +	if (pdev->no_in_band_presence)
>> +		ctrl->inband_presence_disabled = 1;
>> +
>>   	ctrl->slot_cap = slot_cap;
>>   	mutex_init(&ctrl->ctrl_lock);
>>   	mutex_init(&ctrl->state_lock);
> 
> The above hunk belongs in patch 4.
> 
> 
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -413,6 +413,7 @@ struct pci_dev {
>>   	unsigned int	non_compliant_bars:1;	/* Broken BARs; ignore them */
>>   	unsigned int	is_probed:1;		/* Device probing in progress */
>>   	unsigned int	link_active_reporting:1;/* Device capable of reporting link active */
>> +	unsigned int	no_in_band_presence:1;	/* Device does not report in-band presence */
>>   	unsigned int	no_vf_scan:1;		/* Don't scan for VFs after IOV enablement */
>>   	pci_dev_flags_t dev_flags;
>>   	atomic_t	enable_cnt;	/* pci_enable_device has been called */
> 
> Same here.

:)

> Thanks,
> 
> Lukas
> 

[PATCH RFC v2 2/4] PCI: pciehp: Do not turn off slot if presence comes up after link

[Alexandru Gagniuc]

According to PCIe 3.0, the presence detect state is a logical OR of
in-band and out-of-band presence. With this, we'd expect the presence
state to always be asserted when the link comes up.

Not all hardware follows this, and it is possible for the presence to
come up after the link. In this case, the PCIe device would be
erroneously disabled and re-probed. It is possible to distinguish
between a delayed presence and a card swap by looking at the DLL state
changed bit -- The link has to come down if the card is removed.

Thus, for a device that is probed, present and has its link active, a
lack of a link state change event guarantees we have the same device,
and shutdown is not needed.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 drivers/pci/hotplug/pciehp_ctrl.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 3f3df4c29f6e..28965995ebb9 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -213,6 +213,21 @@ void pciehp_handle_disable_request(struct controller *ctrl)
 	ctrl->request_result = pciehp_disable_slot(ctrl, SAFE_REMOVAL);
 }
 
+static bool is_delayed_presence_up_event(struct controller *ctrl, u32 events)
+{
+	bool present, link_active;
+
+	if (!ctrl->inband_presence_disabled)
+		return false;
+
+	present = pciehp_card_present(ctrl);
+	link_active = pciehp_check_link_active(ctrl);
+
+	if (!present || !link_active || events & PCI_EXP_SLTSTA_DLLSC)
+		return false;
+
+	return true;
+}
 void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
 {
 	bool present, link_active;
@@ -220,13 +235,22 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
 	/*
 	 * If the slot is on and presence or link has changed, turn it off.
 	 * Even if it's occupied again, we cannot assume the card is the same.
+	 * When the card is swapped, we also expect a change in link state,
+	 * without which, it's likely presence became high after link-active.
 	 */
 	mutex_lock(&ctrl->state_lock);
+	present = pciehp_card_present(ctrl);
+	link_active = pciehp_check_link_active(ctrl);
 	switch (ctrl->state) {
 	case BLINKINGOFF_STATE:
 		cancel_delayed_work(&ctrl->button_work);
 		/* fall through */
 	case ON_STATE:
+		if (is_delayed_presence_up_event(ctrl, events)) {
+			mutex_unlock(&ctrl->state_lock);
+			ctrl_dbg(ctrl, "Presence state came up after link");
+			return;
+		}
 		ctrl->state = POWEROFF_STATE;
 		mutex_unlock(&ctrl->state_lock);
 		if (events & PCI_EXP_SLTSTA_DLLSC)
-- 
2.19.2

Re: [PATCH RFC v2 2/4] PCI: pciehp: Do not turn off slot if presence comes up after link

[Lukas Wunner]

On Tue, Feb 19, 2019 at 07:20:28PM -0600, Alexandru Gagniuc wrote:
> @@ -213,6 +213,21 @@ void pciehp_handle_disable_request(struct controller *ctrl)
>  	ctrl->request_result = pciehp_disable_slot(ctrl, SAFE_REMOVAL);
>  }
>  
> +static bool is_delayed_presence_up_event(struct controller *ctrl, u32 events)
> +{
> +	bool present, link_active;
> +
> +	if (!ctrl->inband_presence_disabled)
> +		return false;
> +
> +	present = pciehp_card_present(ctrl);
> +	link_active = pciehp_check_link_active(ctrl);
> +
> +	if (!present || !link_active || events & PCI_EXP_SLTSTA_DLLSC)
> +		return false;
> +
> +	return true;
> +}
>  void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)

Newline please after the closing curly brace.


> @@ -220,13 +235,22 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
>  	/*
>  	 * If the slot is on and presence or link has changed, turn it off.
>  	 * Even if it's occupied again, we cannot assume the card is the same.
> +	 * When the card is swapped, we also expect a change in link state,
> +	 * without which, it's likely presence became high after link-active.
>  	 */

Maybe it's just me but I find the code comment difficult to understand.
How about something along the lines of:

 	/*
 	 * If the slot is on and presence or link has changed, turn it off.
 	 * Even if it's occupied again, we cannot assume the card is the same.
+	 *
+	 * An exception is a delayed "Card present" after a "Link Up".
+	 * This can happen on controllers with in-band presence disabled,
+	 * PCIe r5.0 sec X.Y.Z.
 	 */


>  	mutex_lock(&ctrl->state_lock);
> +	present = pciehp_card_present(ctrl);
> +	link_active = pciehp_check_link_active(ctrl);
>  	switch (ctrl->state) {

These two assignments appear to be superfluous as you're also performing
them in pciehp_check_link_active().

Thanks,

Lukas

Re: [PATCH RFC v2 2/4] PCI: pciehp: Do not turn off slot if presence comes up after link

[Alex_Gagniuc]

On 2/21/19 1:36 AM, Lukas Wunner wrote:
> On Tue, Feb 19, 2019 at 07:20:28PM -0600, Alexandru Gagniuc wrote:
>>   	mutex_lock(&ctrl->state_lock);
>> +	present = pciehp_card_present(ctrl);
>> +	link_active = pciehp_check_link_active(ctrl);
>>   	switch (ctrl->state) {
> 
> These two assignments appear to be superfluous as you're also performing
> them in pciehp_check_link_active().

Not sure. Between the first check, and this check, you can have several 
seconds elapse depending on whether the driver's .probe()/remove() is 
invoked. Whatever you got at the beginning would be stale. If you had a 
picture dictionary and looked up 'bad idea', it would have a picture of 
the above code with the second check removed.

I've got all the other review comments addressed in my local branch. I'm 
waiting on Lord Helgass' decision on which solution is better.

Alex

Re: [PATCH RFC v2 2/4] PCI: pciehp: Do not turn off slot if presence comes up after link

[Lukas Wunner]

On Fri, Feb 22, 2019 at 07:56:28PM +0000, Alex_Gagniuc@Dellteam.com wrote:
> On 2/21/19 1:36 AM, Lukas Wunner wrote:
> > On Tue, Feb 19, 2019 at 07:20:28PM -0600, Alexandru Gagniuc wrote:
> >>   	mutex_lock(&ctrl->state_lock);
> >> +	present = pciehp_card_present(ctrl);
> >> +	link_active = pciehp_check_link_active(ctrl);
> >>   	switch (ctrl->state) {
> > 
> > These two assignments appear to be superfluous as you're also performing
> > them in pciehp_check_link_active().
> 
> Not sure. Between the first check, and this check, you can have several 
> seconds elapse depending on whether the driver's .probe()/remove() is 
> invoked. Whatever you got at the beginning would be stale. If you had a 
> picture dictionary and looked up 'bad idea', it would have a picture of 
> the above code with the second check removed.

I don't quite follow.  You're no longer using the "present" and
"link_active" variables in pciehp_handle_presence_or_link_change(),
the variables are set again further down in the function and you're
*also* reading PDS and DLLLA in is_delayed_presence_up_event().
So the above-quoted assignments are superfluous.  Am I missing something?

(Sorry, I had copy-pasted the wrong function name, I meant
is_delayed_presence_up_event() above, not pciehp_check_link_active().


> I've got all the other review comments addressed in my local branch. I'm 
> waiting on Lord Helgass' decision on which solution is better.
             ^^^^^^^^^^^^

Can we keep this discussion in a neutral tone please?

Thanks,

Lukas

Re: [PATCH RFC v2 2/4] PCI: pciehp: Do not turn off slot if presence comes up after link

[Alex_Gagniuc]

On 2/23/19 12:50 AM, Lukas Wunner wrote:
> 
> [EXTERNAL EMAIL]
> 
> On Fri, Feb 22, 2019 at 07:56:28PM +0000, Alex_Gagniuc@Dellteam.com wrote:
>> On 2/21/19 1:36 AM, Lukas Wunner wrote:
>>> On Tue, Feb 19, 2019 at 07:20:28PM -0600, Alexandru Gagniuc wrote:
>>>>    	mutex_lock(&ctrl->state_lock);
>>>> +	present = pciehp_card_present(ctrl);
>>>> +	link_active = pciehp_check_link_active(ctrl);
>>>>    	switch (ctrl->state) {
>>>
>>> These two assignments appear to be superfluous as you're also performing
>>> them in pciehp_check_link_active().
>>
>> Not sure. Between the first check, and this check, you can have several
>> seconds elapse depending on whether the driver's .probe()/remove() is
>> invoked. Whatever you got at the beginning would be stale. If you had a
>> picture dictionary and looked up 'bad idea', it would have a picture of
>> the above code with the second check removed.
> 
> I don't quite follow.  You're no longer using the "present" and
> "link_active" variables in pciehp_handle_presence_or_link_change(),
> the variables are set again further down in the function and you're
> *also* reading PDS and DLLLA in is_delayed_presence_up_event().
> So the above-quoted assignments are superfluous.  Am I missing something?
> 
> (Sorry, I had copy-pasted the wrong function name, I meant
> is_delayed_presence_up_event() above, not pciehp_check_link_active().


I see what I did. You're right. I should remove the following lines from 
the patch. I'll have that fixed when I re-submit this.

+       present = pciehp_card_present(ctrl);
+       link_active = pciehp_check_link_active(ctrl);

> 
>> I've got all the other review comments addressed in my local branch. I'm
>> waiting on Lord Helgass' decision on which solution is better.
>               ^^^^^^^^^^^^
> 
> Can we keep this discussion in a neutral tone please?

I'm sorry. I thought comparing linux to feudalism would be hillarious, 
but I now see not everyone agrees. Sorry, Bjorn.

Alex

[PATCH RFC v2 3/4] PCI: hotplug: Wait for PDS when in-band presence is disabled

[Alexandru Gagniuc]

When inband presence is disabled, PDS may come up at any time, or not
at all. PDS being low may indicate that the card is still mating, and
we could expect contact bounce to bring down the link as well.

It is reasonable to assume that most cards will mate in a hotplug slot
in less than a second. Thus, when we know PDS only reflects
out-of-band presence, it's worthwhile to wait the extra second and
make sure the card is properly mated before loading the driver.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 drivers/pci/hotplug/pciehp_hpc.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index f77dc7c38f9a..9bcadb5d3561 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -240,6 +240,25 @@ static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
 	return found;
 }
 
+static void pcie_wait_for_presence(struct pci_dev *pdev)
+{
+	int timeout = 1000;
+	bool pds;
+	u16 slot_status;
+
+	while (true) {
+		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
+		pds = !!(slot_status & PCI_EXP_SLTSTA_PDS);
+		if (pds || timeout <= 0)
+			break;
+		msleep(20);
+		timeout -= 20;
+	}
+
+	if (!pds)
+		pci_info(pdev, "Presence Detect state not set in 1000 msec\n");
+}
+
 int pciehp_check_link_status(struct controller *ctrl)
 {
 	struct pci_dev *pdev = ctrl_dev(ctrl);
@@ -249,6 +268,9 @@ int pciehp_check_link_status(struct controller *ctrl)
 	if (!pcie_wait_for_link(pdev, true))
 		return -1;
 
+	if (ctrl->inband_presence_disabled)
+		pcie_wait_for_presence(pdev);
+
 	found = pci_bus_check_dev(ctrl->pcie->port->subordinate,
 					PCI_DEVFN(0, 0));
 
-- 
2.19.2

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

[Alexandru Gagniuc]

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(+)

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

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

[Lukas Wunner]

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

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

[Alex_Gagniuc]

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

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

[Joe Perches]

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.

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

[Oliver]

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
>
>

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

[Alex_Gagniuc]

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