| Message ID | 20210903034029.306816-1-nathan@nathanrossi.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: Add ACS errata for Pericom PI7C9X2G404 switch | expand |
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
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
[+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
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
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
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);