diff mbox series

[RFC,net-next,05/12] PCI: Add device-specific reset for NVIDIA Spectrum devices

Message ID 20231017074257.3389177-6-idosch@nvidia.com (mailing list archive)
State Handled Elsewhere
Delegated to: Bjorn Helgaas
Headers show
Series mlxsw: Add support for new reset flow | expand

Commit Message

Ido Schimmel Oct. 17, 2023, 7:42 a.m. UTC
The PCIe specification defines two methods to trigger a hot reset across
a link: Bus reset and link disablement (r6.0.1, sec 7.1, sec 6.6.1). In
the first method, the Secondary Bus Reset (SBR) bit in the Bridge
Control Register of the Downstream Port is asserted for at least 1ms
(r6.0.1, sec 7.5.1.3.13). In the second method, the Link Disable bit in
the Link Control Register of the Downstream Port is asserted and then
cleared to disable and enable the link (r6.0.1, sec 7.5.3.7).

While the two methods are identical from the perspective of the
Downstream device, they are different as far as the host is concerned.
In the first method, the Link Training and Status State Machine (LTSSM)
of the Downstream Port is expected to be in the Hot Reset state as long
as the SBR bit is asserted. In the second method, the LTSSM of the
Downstream Port is expected to be in the Disabled state as long as the
Link Disable bit is asserted.

This above difference is of importance because the specification
requires the LTTSM to exit from the Hot Reset state to the Detect state
within a 2ms timeout (r6.0.1, sec 4.2.7.11). NVIDIA Spectrum devices
cannot guarantee it and a host enforcing such a behavior might fail to
communicate with the device after issuing a Secondary Bus Reset. With
the link disablement method, the host can leave the link disabled for
enough time to allow the device to undergo a hot reset and reach the
Detect state. After enabling the link, the host will exit from the
Disabled state to Detect state (r6.0.1, sec 4.2.7.9) and observe that
the device is already in the Detect state.

The PCI core only implements the first method, which might not work with
NVIDIA Spectrum devices on certain hosts, as explained above. Therefore,
implement the link disablement method as a device-specific method for
NVIDIA Spectrum devices. Specifically, disable the link, wait for 500ms,
enable the link and then wait for the device to become accessible.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 drivers/pci/quirks.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

Comments

Lukas Wunner Oct. 17, 2023, 10 a.m. UTC | #1
On Tue, Oct 17, 2023 at 10:42:50AM +0300, Ido Schimmel wrote:
> The PCIe specification defines two methods to trigger a hot reset across
> a link: Bus reset and link disablement (r6.0.1, sec 7.1, sec 6.6.1). In
> the first method, the Secondary Bus Reset (SBR) bit in the Bridge
> Control Register of the Downstream Port is asserted for at least 1ms
> (r6.0.1, sec 7.5.1.3.13). In the second method, the Link Disable bit in
> the Link Control Register of the Downstream Port is asserted and then
> cleared to disable and enable the link (r6.0.1, sec 7.5.3.7).
> 
> While the two methods are identical from the perspective of the
> Downstream device, they are different as far as the host is concerned.
> In the first method, the Link Training and Status State Machine (LTSSM)
> of the Downstream Port is expected to be in the Hot Reset state as long
> as the SBR bit is asserted. In the second method, the LTSSM of the
> Downstream Port is expected to be in the Disabled state as long as the
> Link Disable bit is asserted.
> 
> This above difference is of importance because the specification
> requires the LTTSM to exit from the Hot Reset state to the Detect state
> within a 2ms timeout (r6.0.1, sec 4.2.7.11). NVIDIA Spectrum devices
> cannot guarantee it and a host enforcing such a behavior might fail to
> communicate with the device after issuing a Secondary Bus Reset.

How does that failure manifest itself exactly?  Is the problem that
the Vendor ID register in config space is read too early and the
device doesn't like that?

It is possible to increase the d3cold_delay in struct pci_dev to
lengthen the delay until the Vendor ID is read.  Have you considered
that instead of using the Link Disable method?

The following commit queued for v6.7 introduces a quirk for a 1 second
d3cold_delay, perhaps you can take advantage of it?

https://git.kernel.org/pci/pci/c/c9260693aa0c

Thanks,

Lukas
Bjorn Helgaas Oct. 18, 2023, 8:08 p.m. UTC | #2
On Tue, Oct 17, 2023 at 10:42:50AM +0300, Ido Schimmel wrote:
> The PCIe specification defines two methods to trigger a hot reset across
> a link: Bus reset and link disablement (r6.0.1, sec 7.1, sec 6.6.1). In
> the first method, the Secondary Bus Reset (SBR) bit in the Bridge
> Control Register of the Downstream Port is asserted for at least 1ms
> (r6.0.1, sec 7.5.1.3.13). In the second method, the Link Disable bit in
> the Link Control Register of the Downstream Port is asserted and then
> cleared to disable and enable the link (r6.0.1, sec 7.5.3.7).
> 
> While the two methods are identical from the perspective of the
> Downstream device, they are different as far as the host is concerned.
> In the first method, the Link Training and Status State Machine (LTSSM)
> of the Downstream Port is expected to be in the Hot Reset state as long
> as the SBR bit is asserted. In the second method, the LTSSM of the
> Downstream Port is expected to be in the Disabled state as long as the
> Link Disable bit is asserted.
> 
> This above difference is of importance because the specification
> requires the LTTSM to exit from the Hot Reset state to the Detect state
> within a 2ms timeout (r6.0.1, sec 4.2.7.11).

I don't read 4.2.7.11 quite that way.  Here's the text (from r6.0):

  • Lanes that were directed by a higher Layer to initiate Hot
    Reset:

    ◦ All Lanes in the configured Link transmit TS1 Ordered Sets
      with the Hot Reset bit asserted and the configured Link and
      Lane numbers.

    ◦ If two consecutive TS1 Ordered Sets are received on any
      Lane with the Hot Reset bit asserted and configured Link
      and Lane numbers, then:

      ▪ LinkUp = 0b (False)

      ▪ If no higher Layer is directing the Physical Layer to
        remain in Hot Reset, the next state is Detect

      ▪ Otherwise, all Lanes in the configured Link continue to
	transmit TS1 Ordered Sets with the Hot Reset bit asserted
	and the configured Link and Lane numbers.

    ◦ Otherwise, after a 2 ms timeout next state is Detect.

I assume that SBR being set constitutes a "higher Layer directing the
Physical Layer to remain in Hot Reset," so I would read this as saying
the LTSSM stays in Hot Reset as long as SBR is set.  Then, *after* a
2 ms timeout (not *within* 2 ms), the next state is Detect.

> NVIDIA Spectrum devices cannot guarantee it and a host enforcing
> such a behavior might fail to communicate with the device after
> issuing a Secondary Bus Reset.

I don't quite follow this.  What behavior is the host enforcing here?
I guess you're doing an SBR, and the Spectrum device doesn't respond
as expected afterwards?

It looks like pci_reset_secondary_bus() asserts SBR for at least
2 ms.  Then pci_bridge_wait_for_secondary_bus() should wait before
accessing the device, but maybe we don't wait long enough?

I guess this ends up back at d3cold_delay as suggested by Lukas.

> With the link disablement method, the host can leave the link
> disabled for enough time to allow the device to undergo a hot reset
> and reach the Detect state. After enabling the link, the host will
> exit from the Disabled state to Detect state (r6.0.1, sec 4.2.7.9)
> and observe that the device is already in the Detect state.
> 
> The PCI core only implements the first method, which might not work with
> NVIDIA Spectrum devices on certain hosts, as explained above. Therefore,
> implement the link disablement method as a device-specific method for
> NVIDIA Spectrum devices. Specifically, disable the link, wait for 500ms,
> enable the link and then wait for the device to become accessible.
> 
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> ---
>  drivers/pci/quirks.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 23f6bd2184e2..a6e308bb934c 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4182,6 +4182,31 @@ static int reset_hinic_vf_dev(struct pci_dev *pdev, bool probe)
>  	return 0;
>  }
>  
> +#define PCI_DEVICE_ID_MELLANOX_SPECTRUM		0xcb84
> +#define PCI_DEVICE_ID_MELLANOX_SPECTRUM2	0xcf6c
> +#define PCI_DEVICE_ID_MELLANOX_SPECTRUM3	0xcf70
> +#define PCI_DEVICE_ID_MELLANOX_SPECTRUM4	0xcf80
> +
> +static int reset_mlx(struct pci_dev *pdev, bool probe)
> +{
> +	struct pci_dev *bridge = pdev->bus->self;
> +
> +	if (probe)
> +		return 0;
> +
> +	/*
> +	 * Disable the link on the Downstream port in order to trigger a hot
> +	 * reset in the Downstream device. Wait for 500ms before enabling the
> +	 * link so that the firmware on the device will have enough time to
> +	 * transition the Upstream port to the Detect state.
> +	 */
> +	pcie_capability_set_word(bridge, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_LD);
> +	msleep(500);
> +	pcie_capability_clear_word(bridge, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_LD);
> +
> +	return pci_bridge_wait_for_secondary_bus(bridge, "link toggle");
> +}
> +
>  static const struct pci_dev_reset_methods pci_dev_reset_methods[] = {
>  	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82599_SFP_VF,
>  		 reset_intel_82599_sfp_virtfn },
> @@ -4197,6 +4222,10 @@ static const struct pci_dev_reset_methods pci_dev_reset_methods[] = {
>  		reset_chelsio_generic_dev },
>  	{ PCI_VENDOR_ID_HUAWEI, PCI_DEVICE_ID_HINIC_VF,
>  		reset_hinic_vf_dev },
> +	{ PCI_VENDOR_ID_MELLANOX, PCI_DEVICE_ID_MELLANOX_SPECTRUM, reset_mlx },
> +	{ PCI_VENDOR_ID_MELLANOX, PCI_DEVICE_ID_MELLANOX_SPECTRUM2, reset_mlx },
> +	{ PCI_VENDOR_ID_MELLANOX, PCI_DEVICE_ID_MELLANOX_SPECTRUM3, reset_mlx },
> +	{ PCI_VENDOR_ID_MELLANOX, PCI_DEVICE_ID_MELLANOX_SPECTRUM4, reset_mlx },
>  	{ 0 }
>  };
>  
> -- 
> 2.40.1
>
Ido Schimmel Oct. 25, 2023, 11:05 a.m. UTC | #3
On Wed, Oct 18, 2023 at 03:08:26PM -0500, Bjorn Helgaas wrote:
> On Tue, Oct 17, 2023 at 10:42:50AM +0300, Ido Schimmel wrote:
> > The PCIe specification defines two methods to trigger a hot reset across
> > a link: Bus reset and link disablement (r6.0.1, sec 7.1, sec 6.6.1). In
> > the first method, the Secondary Bus Reset (SBR) bit in the Bridge
> > Control Register of the Downstream Port is asserted for at least 1ms
> > (r6.0.1, sec 7.5.1.3.13). In the second method, the Link Disable bit in
> > the Link Control Register of the Downstream Port is asserted and then
> > cleared to disable and enable the link (r6.0.1, sec 7.5.3.7).
> > 
> > While the two methods are identical from the perspective of the
> > Downstream device, they are different as far as the host is concerned.
> > In the first method, the Link Training and Status State Machine (LTSSM)
> > of the Downstream Port is expected to be in the Hot Reset state as long
> > as the SBR bit is asserted. In the second method, the LTSSM of the
> > Downstream Port is expected to be in the Disabled state as long as the
> > Link Disable bit is asserted.
> > 
> > This above difference is of importance because the specification
> > requires the LTTSM to exit from the Hot Reset state to the Detect state
> > within a 2ms timeout (r6.0.1, sec 4.2.7.11).
> 
> I don't read 4.2.7.11 quite that way.  Here's the text (from r6.0):
> 
>   • Lanes that were directed by a higher Layer to initiate Hot
>     Reset:
> 
>     ◦ All Lanes in the configured Link transmit TS1 Ordered Sets
>       with the Hot Reset bit asserted and the configured Link and
>       Lane numbers.
> 
>     ◦ If two consecutive TS1 Ordered Sets are received on any
>       Lane with the Hot Reset bit asserted and configured Link
>       and Lane numbers, then:
> 
>       ▪ LinkUp = 0b (False)
> 
>       ▪ If no higher Layer is directing the Physical Layer to
>         remain in Hot Reset, the next state is Detect
> 
>       ▪ Otherwise, all Lanes in the configured Link continue to
> 	transmit TS1 Ordered Sets with the Hot Reset bit asserted
> 	and the configured Link and Lane numbers.
> 
>     ◦ Otherwise, after a 2 ms timeout next state is Detect.
> 
> I assume that SBR being set constitutes a "higher Layer directing the
> Physical Layer to remain in Hot Reset," so I would read this as saying
> the LTSSM stays in Hot Reset as long as SBR is set.  Then, *after* a
> 2 ms timeout (not *within* 2 ms), the next state is Detect.
> 
> > NVIDIA Spectrum devices cannot guarantee it and a host enforcing
> > such a behavior might fail to communicate with the device after
> > issuing a Secondary Bus Reset.
> 
> I don't quite follow this.  What behavior is the host enforcing here?
> I guess you're doing an SBR, and the Spectrum device doesn't respond
> as expected afterwards?
> 
> It looks like pci_reset_secondary_bus() asserts SBR for at least
> 2 ms.  Then pci_bridge_wait_for_secondary_bus() should wait before
> accessing the device, but maybe we don't wait long enough?
> 
> I guess this ends up back at d3cold_delay as suggested by Lukas.

I had a meeting with the PCI team before submitting this patch where I
stated that bus reset works fine (tested over 500 iterations) on the
hosts I have access to. They said that bus reset and link toggling are
identical from the perspective of the downstream device, but that in the
past they saw hosts that fail bus reset because of the time it takes the
downstream device to reach the Detect state. This was with a different
line of products that share the same PCI IP as Spectrum.

Given that I'm unable to reproduce this problem with Spectrum and that
your preference seems to be to reuse bus reset (or bus reset plus the
d3cold_delay quirk), I'll drop this patch for now. We can revisit this
patch in the future, if the problem manifests itself.

Regarding the other two PCI patches, I plan to submit this series after
net-next opens for v6.8. Are you OK with them being merged via net-next?

Thanks
diff mbox series

Patch

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 23f6bd2184e2..a6e308bb934c 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4182,6 +4182,31 @@  static int reset_hinic_vf_dev(struct pci_dev *pdev, bool probe)
 	return 0;
 }
 
+#define PCI_DEVICE_ID_MELLANOX_SPECTRUM		0xcb84
+#define PCI_DEVICE_ID_MELLANOX_SPECTRUM2	0xcf6c
+#define PCI_DEVICE_ID_MELLANOX_SPECTRUM3	0xcf70
+#define PCI_DEVICE_ID_MELLANOX_SPECTRUM4	0xcf80
+
+static int reset_mlx(struct pci_dev *pdev, bool probe)
+{
+	struct pci_dev *bridge = pdev->bus->self;
+
+	if (probe)
+		return 0;
+
+	/*
+	 * Disable the link on the Downstream port in order to trigger a hot
+	 * reset in the Downstream device. Wait for 500ms before enabling the
+	 * link so that the firmware on the device will have enough time to
+	 * transition the Upstream port to the Detect state.
+	 */
+	pcie_capability_set_word(bridge, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_LD);
+	msleep(500);
+	pcie_capability_clear_word(bridge, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_LD);
+
+	return pci_bridge_wait_for_secondary_bus(bridge, "link toggle");
+}
+
 static const struct pci_dev_reset_methods pci_dev_reset_methods[] = {
 	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82599_SFP_VF,
 		 reset_intel_82599_sfp_virtfn },
@@ -4197,6 +4222,10 @@  static const struct pci_dev_reset_methods pci_dev_reset_methods[] = {
 		reset_chelsio_generic_dev },
 	{ PCI_VENDOR_ID_HUAWEI, PCI_DEVICE_ID_HINIC_VF,
 		reset_hinic_vf_dev },
+	{ PCI_VENDOR_ID_MELLANOX, PCI_DEVICE_ID_MELLANOX_SPECTRUM, reset_mlx },
+	{ PCI_VENDOR_ID_MELLANOX, PCI_DEVICE_ID_MELLANOX_SPECTRUM2, reset_mlx },
+	{ PCI_VENDOR_ID_MELLANOX, PCI_DEVICE_ID_MELLANOX_SPECTRUM3, reset_mlx },
+	{ PCI_VENDOR_ID_MELLANOX, PCI_DEVICE_ID_MELLANOX_SPECTRUM4, reset_mlx },
 	{ 0 }
 };