[PATCH] PCI: Add ACS errata for Pericom PI7C9X2G404 switch

[PATCH] PCI: Add ACS errata for Pericom PI7C9X2G404 switch

[Nathan Rossi]

From: Nathan Rossi <nathan.rossi@digi.com>

The Pericom PI7C9X2G404 PCIe switch has an errata for ACS P2P Request
Redirect behaviour when used in the cut-through forwarding mode. The
recommended work around for this issue is to use the switch in store and
forward mode.

This change adds a fixup specific to this switch that when enabling the
downstream port it checks if it has enabled ACS P2P Request Redirect,
and if so changes the device (via the upstream port) to use the store
and forward operating mode.

Signed-off-by: Nathan Rossi <nathan.rossi@digi.com>
---
 drivers/pci/quirks.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index ab3de1551b..1ea6661d01 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -5704,3 +5704,45 @@ static void apex_pci_fixup_class(struct pci_dev *pdev)
 }
 DECLARE_PCI_FIXUP_CLASS_HEADER(0x1ac1, 0x089a,
 			       PCI_CLASS_NOT_DEFINED, 8, apex_pci_fixup_class);
+
+/*
+ * Pericom PI7C9X2G404 switch errata E5 - ACS P2P Request Redirect is not
+ * functional
+ *
+ * When ACS P2P Request Redirect is enabled and bandwidth is not balanced
+ * between upstream and downstream ports, packets are queued in an internal
+ * buffer until CPLD packet. The workaround is to use the switch in store and
+ * forward mode.
+ */
+#define PI7C9X2G404_MODE_REG		0x74
+#define PI7C9X2G404_STORE_FORWARD_MODE	BIT(0)
+static void pci_fixup_pericom_acs_store_forward(struct pci_dev *pdev)
+{
+	struct pci_dev *upstream;
+	u16 val;
+
+	/* Downstream ports only */
+	if (pci_pcie_type(pdev) != PCI_EXP_TYPE_DOWNSTREAM)
+		return;
+
+	/* Check for ACS P2P Request Redirect use */
+	if (!pdev->acs_cap)
+		return;
+	pci_read_config_word(pdev, pdev->acs_cap + PCI_ACS_CTRL, &val);
+	if (!(val & PCI_ACS_RR))
+		return;
+
+	upstream = pci_upstream_bridge(pdev);
+	if (!upstream)
+		return;
+
+	pci_read_config_word(upstream, PI7C9X2G404_MODE_REG, &val);
+	if (!(val & PI7C9X2G404_STORE_FORWARD_MODE)) {
+		pci_info(upstream, "Setting PI7C9X2G404 store-forward mode\n");
+		/* Enable store-foward mode */
+		pci_write_config_word(upstream, PI7C9X2G404_MODE_REG, val |
+				      PI7C9X2G404_STORE_FORWARD_MODE);
+	}
+}
+DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_PERICOM, 0x2404,
+			 pci_fixup_pericom_acs_store_forward);
---
2.33.0

Re: [PATCH] PCI: Add ACS errata for Pericom PI7C9X2G404 switch

[Lukas Wunner]

On Fri, Sep 03, 2021 at 03:40:29AM +0000, Nathan Rossi wrote:
> The Pericom PI7C9X2G404 PCIe switch has an errata for ACS P2P Request
> Redirect behaviour when used in the cut-through forwarding mode. The
> recommended work around for this issue is to use the switch in store and
> forward mode.
> 
> This change adds a fixup specific to this switch that when enabling the
> downstream port it checks if it has enabled ACS P2P Request Redirect,
> and if so changes the device (via the upstream port) to use the store
> and forward operating mode.

From a quick look at the datasheet, this switch seems to support
hot-plug on its Downstream Ports:

https://www.diodes.com/assets/Datasheets/PI7C9X2G404SL.pdf

I think your quirk isn't executed if a device is hotplugged to an
initially-empty Downstream Port.

Also, if a device which triggered the quirk is hot-removed and none
of its siblings uses ACS P2P Request Redirect, cut-through forwarding
isn't reinstated.

Perhaps we need additional pci_fixup ELF sections which are used on
hot-add and hot-remove?

Thanks,

Lukas

Re: [PATCH] PCI: Add ACS errata for Pericom PI7C9X2G404 switch

[Nathan Rossi]

On Fri, 3 Sept 2021 at 16:18, Lukas Wunner <lukas@wunner.de> wrote:
>
> On Fri, Sep 03, 2021 at 03:40:29AM +0000, Nathan Rossi wrote:
> > The Pericom PI7C9X2G404 PCIe switch has an errata for ACS P2P Request
> > Redirect behaviour when used in the cut-through forwarding mode. The
> > recommended work around for this issue is to use the switch in store and
> > forward mode.
> >
> > This change adds a fixup specific to this switch that when enabling the
> > downstream port it checks if it has enabled ACS P2P Request Redirect,
> > and if so changes the device (via the upstream port) to use the store
> > and forward operating mode.
>
> From a quick look at the datasheet, this switch seems to support
> hot-plug on its Downstream Ports:
>
> https://www.diodes.com/assets/Datasheets/PI7C9X2G404SL.pdf
>
> I think your quirk isn't executed if a device is hotplugged to an
> initially-empty Downstream Port.

The device I am testing against has the ports wired directly to
devices (though can be disconnected) without hotplug so I will see if
I can find a development board with this switch to test the hotplug
behaviour. However it should be noted that the downstream ports are
probed with the switch, and are enabled with the ACS P2P Request
Redirect configured regardless of the presence of a device connected
to the downstream port.

>
> Also, if a device which triggered the quirk is hot-removed and none
> of its siblings uses ACS P2P Request Redirect, cut-through forwarding
> isn't reinstated.

The quirk is enabled on the downstream port of the switch, using the
state of the downstream port and not the device attached to it. My
understanding is that the only path that enables/disables the ACS P2P
Request Redirect on the downstream port is the initial pci_enable_acs.
This means that devices attached to the downstream port either
initially or with hotplugging should not change the ACS configuration
of the switches downstream port.

Which means nothing can cause the switch to need to be reinstated with
cut-through forwarding except the switch itself being hotplugged,
which would cause reset of the switch and the enable fixup to be
called again.

Thanks,
Nathan

>
> Perhaps we need additional pci_fixup ELF sections which are used on
> hot-add and hot-remove?
>
> Thanks,
>
> Lukas

Re: [PATCH] PCI: Add ACS errata for Pericom PI7C9X2G404 switch

[Bjorn Helgaas]

[+cc Alex, beginning of thread:
https://lore.kernel.org/r/20210903034029.306816-1-nathan@nathanrossi.com]

On Mon, Sep 06, 2021 at 04:01:20PM +1000, Nathan Rossi wrote:
> On Fri, 3 Sept 2021 at 16:18, Lukas Wunner <lukas@wunner.de> wrote:
> >
> > On Fri, Sep 03, 2021 at 03:40:29AM +0000, Nathan Rossi wrote:
> > > The Pericom PI7C9X2G404 PCIe switch has an errata for ACS P2P Request
> > > Redirect behaviour when used in the cut-through forwarding mode. The
> > > recommended work around for this issue is to use the switch in store and
> > > forward mode.

Is there a URL for this erratum?  What is the issue?  Does the switch
fail to redirect P2P requests upstream?  How would someone recognize
that they are affected by it?

> > > This change adds a fixup specific to this switch that when enabling the
> > > downstream port it checks if it has enabled ACS P2P Request Redirect,
> > > and if so changes the device (via the upstream port) to use the store
> > > and forward operating mode.
> >
> > From a quick look at the datasheet, this switch seems to support
> > hot-plug on its Downstream Ports:
> >
> > https://www.diodes.com/assets/Datasheets/PI7C9X2G404SL.pdf
> >
> > I think your quirk isn't executed if a device is hotplugged to an
> > initially-empty Downstream Port.
> 
> The device I am testing against has the ports wired directly to
> devices (though can be disconnected) without hotplug so I will see if
> I can find a development board with this switch to test the hotplug
> behaviour. However it should be noted that the downstream ports are
> probed with the switch, and are enabled with the ACS P2P Request
> Redirect configured regardless of the presence of a device connected
> to the downstream port.
> 
> > Also, if a device which triggered the quirk is hot-removed and none
> > of its siblings uses ACS P2P Request Redirect, cut-through forwarding
> > isn't reinstated.
> 
> The quirk is enabled on the downstream port of the switch, using the
> state of the downstream port and not the device attached to it. My
> understanding is that the only path that enables/disables the ACS P2P
> Request Redirect on the downstream port is the initial pci_enable_acs.

pci_enable_acs() is called during enumeration of each device
(including the switch, of course):

  pci_init_capabilities
    pci_acs_init
      pci_enable_acs

and your quirk is DECLARE_PCI_FIXUP_ENABLE(), so it runs later, during
pci_enable_device().  I don't think we ever turn on ACS P2P Request
Redirect after enumeration.

But we do also call pci_enable_acs() from pci_restore_state(), so this
happens during resume.  I assume your quirk would also need to run
then?  Is there a pci_enable_device() somewhere in the resume path
that will do this?

> This means that devices attached to the downstream port either
> initially or with hotplugging should not change the ACS configuration
> of the switches downstream port.
> 
> Which means nothing can cause the switch to need to be reinstated with
> cut-through forwarding except the switch itself being hotplugged,
> which would cause reset of the switch and the enable fixup to be
> called again.

Seems right to me, since we enable ACS P2P Request Redirect regardless
of whether any downstream device exists.

> > Perhaps we need additional pci_fixup ELF sections which are used on
> > hot-add and hot-remove?
> >
> > Thanks,
> >
> > Lukas

Re: [PATCH] PCI: Add ACS errata for Pericom PI7C9X2G404 switch

[Nathan Rossi]

On Thu, 9 Sept 2021 at 08:24, Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Alex, beginning of thread:
> https://lore.kernel.org/r/20210903034029.306816-1-nathan@nathanrossi.com]
>
> On Mon, Sep 06, 2021 at 04:01:20PM +1000, Nathan Rossi wrote:
> > On Fri, 3 Sept 2021 at 16:18, Lukas Wunner <lukas@wunner.de> wrote:
> > >
> > > On Fri, Sep 03, 2021 at 03:40:29AM +0000, Nathan Rossi wrote:
> > > > The Pericom PI7C9X2G404 PCIe switch has an errata for ACS P2P Request
> > > > Redirect behaviour when used in the cut-through forwarding mode. The
> > > > recommended work around for this issue is to use the switch in store and
> > > > forward mode.
>
> Is there a URL for this erratum?  What is the issue?  Does the switch

Unfortunately the document is not public, it was provided under a NDA.
However with that said, there is very little additional information in
the document itself compared to the information provided in the commit
message/code comments here. The only other information in the document
that may be applicable is that the whole document is for a number of
Pericom switch models, however I do not have access to the other two
switch models and thus cannot validate if this fixup would also apply
to them.

For reference the models with PCI IDs:
- PI7C9X2G404 - 12d8:2404
- PI7C9X2G304 - 12d8:2304
- PI7C9X2G303 - 12d8:2303

> fail to redirect P2P requests upstream?  How would someone recognize
> that they are affected by it?

Firstly it only affects users if ACS P2P Request Redirect is enabled.
It is quite obvious when the issue is present as devices attached to
the switch will behave poorly. With network devices the observed
effects are drastically inconsistent ping (1ms - 1500ms) and very poor
TCP bandwidth (<1Mbit). With a serial port device, device generated
interrupts are not received causing no data to be received.

>
> > > > This change adds a fixup specific to this switch that when enabling the
> > > > downstream port it checks if it has enabled ACS P2P Request Redirect,
> > > > and if so changes the device (via the upstream port) to use the store
> > > > and forward operating mode.
> > >
> > > From a quick look at the datasheet, this switch seems to support
> > > hot-plug on its Downstream Ports:
> > >
> > > https://www.diodes.com/assets/Datasheets/PI7C9X2G404SL.pdf
> > >
> > > I think your quirk isn't executed if a device is hotplugged to an
> > > initially-empty Downstream Port.
> >
> > The device I am testing against has the ports wired directly to
> > devices (though can be disconnected) without hotplug so I will see if
> > I can find a development board with this switch to test the hotplug
> > behaviour. However it should be noted that the downstream ports are
> > probed with the switch, and are enabled with the ACS P2P Request
> > Redirect configured regardless of the presence of a device connected
> > to the downstream port.
> >
> > > Also, if a device which triggered the quirk is hot-removed and none
> > > of its siblings uses ACS P2P Request Redirect, cut-through forwarding
> > > isn't reinstated.
> >
> > The quirk is enabled on the downstream port of the switch, using the
> > state of the downstream port and not the device attached to it. My
> > understanding is that the only path that enables/disables the ACS P2P
> > Request Redirect on the downstream port is the initial pci_enable_acs.
>
> pci_enable_acs() is called during enumeration of each device
> (including the switch, of course):
>
>   pci_init_capabilities
>     pci_acs_init
>       pci_enable_acs
>
> and your quirk is DECLARE_PCI_FIXUP_ENABLE(), so it runs later, during
> pci_enable_device().  I don't think we ever turn on ACS P2P Request
> Redirect after enumeration.
>
> But we do also call pci_enable_acs() from pci_restore_state(), so this
> happens during resume.  I assume your quirk would also need to run
> then?  Is there a pci_enable_device() somewhere in the resume path
> that will do this?

There is a pci_enable_device path in pci_pm_resume, I initially
thought this would cover this device however that is not called for
pcieport devices as the pcieport driver provides its own pm ops.

I have tested and confirmed that this change will also need
DECLARE_PCI_FIXUP_RESUME in order to apply the fixup on resume. I will
send an updated v2 patch to include that.

Thanks,
Nathan

>
> > This means that devices attached to the downstream port either
> > initially or with hotplugging should not change the ACS configuration
> > of the switches downstream port.
> >
> > Which means nothing can cause the switch to need to be reinstated with
> > cut-through forwarding except the switch itself being hotplugged,
> > which would cause reset of the switch and the enable fixup to be
> > called again.
>
> Seems right to me, since we enable ACS P2P Request Redirect regardless
> of whether any downstream device exists.
>
> > > Perhaps we need additional pci_fixup ELF sections which are used on
> > > hot-add and hot-remove?
> > >
> > > Thanks,
> > >
> > > Lukas

Re: [PATCH] PCI: Add ACS errata for Pericom PI7C9X2G404 switch

[Bjorn Helgaas]

On Thu, Sep 09, 2021 at 06:08:33PM +1000, Nathan Rossi wrote:
> On Thu, 9 Sept 2021 at 08:24, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > [+cc Alex, beginning of thread:
> > https://lore.kernel.org/r/20210903034029.306816-1-nathan@nathanrossi.com]
> >
> > On Mon, Sep 06, 2021 at 04:01:20PM +1000, Nathan Rossi wrote:
> > > On Fri, 3 Sept 2021 at 16:18, Lukas Wunner <lukas@wunner.de> wrote:
> > > >
> > > > On Fri, Sep 03, 2021 at 03:40:29AM +0000, Nathan Rossi wrote:
> > > > > The Pericom PI7C9X2G404 PCIe switch has an errata for ACS P2P Request
> > > > > Redirect behaviour when used in the cut-through forwarding mode. The
> > > > > recommended work around for this issue is to use the switch in store and
> > > > > forward mode.
> >
> > Is there a URL for this erratum?  What is the issue?  Does the switch
> 
> Unfortunately the document is not public, it was provided under a NDA.
> However with that said, there is very little additional information in
> the document itself compared to the information provided in the commit
> message/code comments here. The only other information in the document
> that may be applicable is that the whole document is for a number of
> Pericom switch models, however I do not have access to the other two
> switch models and thus cannot validate if this fixup would also apply
> to them.
> 
> For reference the models with PCI IDs:
> - PI7C9X2G404 - 12d8:2404
> - PI7C9X2G304 - 12d8:2304
> - PI7C9X2G303 - 12d8:2303

I assume that running all these models in store and forward mode is
safe, even if it's not the highest-performance config.  If so, I'd
prefer to include all the documented Device IDs in the quirk so people
don't have to stumble over them before we can fix them.

If somebody complains about the performance and can verify that a
device *doesn't* need the quirk, we can remove it then.

Bjorn