[PATCH] PCI/DOE: Poll DOE Busy bit for up to 1 second in pci_doe_send_req

[PATCH] PCI/DOE: Poll DOE Busy bit for up to 1 second in pci_doe_send_req

[Gregory Price]

During initial device probe, the PCI DOE busy bit for some CXL
devices may be left set for a longer period than expected by the
current driver logic. Despite local comments stating DOE Busy is
unlikely to be detected, it appears commonly specifically during
boot when CXL devices are being probed.

This was observed on a single socket AMD platform with 2 CXL memory
expanders attached to the single socket. It was not the case that
concurrent accesses were being made, as validated by monitoring
mailbox commands on the device side.

This behavior has been observed with multiple CXL memory expanders
from different vendors - so it appears unrelated to the model.

In all observed tests, only a small period of the retry window is
actually used - typically only a handful of loop iterations.

Polling on the PCI DOE Busy Bit for (at max) one PCI DOE timeout
interval (1 second), resolves this issues cleanly.

Per PCIe r6.2 sec 6.30.3, the DOE Busy Bit being cleared does not
raise an interrupt, so polling is the best option in this scenario.

Subsqeuent code in doe_statemachine_work and abort paths also wait
for up to 1 PCI DOE timeout interval, so this order of (potential)
additional delay is presumed acceptable.

Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Gregory Price <gourry@gourry.net>
---
 drivers/pci/doe.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
index 652d63df9d22..27ba5d281384 100644
--- a/drivers/pci/doe.c
+++ b/drivers/pci/doe.c
@@ -149,14 +149,26 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb,
 	size_t length, remainder;
 	u32 val;
 	int i;
+	unsigned long timeout_jiffies;
 
 	/*
 	 * Check the DOE busy bit is not set. If it is set, this could indicate
 	 * someone other than Linux (e.g. firmware) is using the mailbox. Note
 	 * it is expected that firmware and OS will negotiate access rights via
 	 * an, as yet to be defined, method.
+	 *
+	 * Wait up to one PCI_DOE_TIMEOUT period to allow the prior command to
+	 * finish. Otherwise, simply error out as unable to field the request.
+	 *
+	 * PCIe r6.2 sec 6.30.3 states no interrupt is raised when the DOE Busy
+	 * bit is cleared, so polling here is our best option for the moment.
 	 */
-	pci_read_config_dword(pdev, offset + PCI_DOE_STATUS, &val);
+	timeout_jiffies = jiffies + PCI_DOE_TIMEOUT;
+	do {
+		pci_read_config_dword(pdev, offset + PCI_DOE_STATUS, &val);
+	} while (FIELD_GET(PCI_DOE_STATUS_BUSY, val) &&
+		 !time_after(jiffies, timeout_jiffies));
+
 	if (FIELD_GET(PCI_DOE_STATUS_BUSY, val))
 		return -EBUSY;
 
-- 
2.43.0

Re: [PATCH] PCI/DOE: Poll DOE Busy bit for up to 1 second in pci_doe_send_req

[Lukas Wunner]

On Fri, Oct 04, 2024 at 12:28:28PM -0400, Gregory Price wrote:
> Polling on the PCI DOE Busy Bit for (at max) one PCI DOE timeout
> interval (1 second), resolves this issues cleanly.

Nit: s/issues/issue/

> Subsqeuent code in doe_statemachine_work and abort paths also wait

Nit: s/Subsqeuent/Subsequent/

> --- a/drivers/pci/doe.c
> +++ b/drivers/pci/doe.c
> @@ -149,14 +149,26 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb,
>  	size_t length, remainder;
>  	u32 val;
>  	int i;
> +	unsigned long timeout_jiffies;

Nit: Reverse Christmas tree.

With that addressed,
Reviewed-by: Lukas Wunner <lukas@wunner.de>

Re: [PATCH] PCI/DOE: Poll DOE Busy bit for up to 1 second in pci_doe_send_req

[Jonathan Cameron]

On Thu, 10 Oct 2024 12:38:08 +0200
Lukas Wunner <lukas@wunner.de> wrote:

> On Fri, Oct 04, 2024 at 12:28:28PM -0400, Gregory Price wrote:
> > Polling on the PCI DOE Busy Bit for (at max) one PCI DOE timeout
> > interval (1 second), resolves this issues cleanly.  
> 
> Nit: s/issues/issue/
> 
> > Subsqeuent code in doe_statemachine_work and abort paths also wait  
> 
> Nit: s/Subsqeuent/Subsequent/
> 
> > --- a/drivers/pci/doe.c
> > +++ b/drivers/pci/doe.c
> > @@ -149,14 +149,26 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb,
> >  	size_t length, remainder;
> >  	u32 val;
> >  	int i;
> > +	unsigned long timeout_jiffies;  
> 
> Nit: Reverse Christmas tree.
> 
> With that addressed,
> Reviewed-by: Lukas Wunner <lukas@wunner.de>
> 
Looks good to me with Lukas' nits tidied up.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH] PCI/DOE: Poll DOE Busy bit for up to 1 second in pci_doe_send_req

[Bjorn Helgaas]

On Fri, Oct 04, 2024 at 12:28:28PM -0400, Gregory Price wrote:
> During initial device probe, the PCI DOE busy bit for some CXL
> devices may be left set for a longer period than expected by the
> current driver logic. Despite local comments stating DOE Busy is
> unlikely to be detected, it appears commonly specifically during
> boot when CXL devices are being probed.
> 
> This was observed on a single socket AMD platform with 2 CXL memory
> expanders attached to the single socket. It was not the case that
> concurrent accesses were being made, as validated by monitoring
> mailbox commands on the device side.
> 
> This behavior has been observed with multiple CXL memory expanders
> from different vendors - so it appears unrelated to the model.
> 
> In all observed tests, only a small period of the retry window is
> actually used - typically only a handful of loop iterations.
> 
> Polling on the PCI DOE Busy Bit for (at max) one PCI DOE timeout
> interval (1 second), resolves this issues cleanly.
> 
> Per PCIe r6.2 sec 6.30.3, the DOE Busy Bit being cleared does not
> raise an interrupt, so polling is the best option in this scenario.
> 
> Subsqeuent code in doe_statemachine_work and abort paths also wait
> for up to 1 PCI DOE timeout interval, so this order of (potential)
> additional delay is presumed acceptable.

I provisionally applied this to pci/doe for v6.13 with Lukas and
Jonathan's reviewed-by.  

Can we include a sample of any dmesg logging or other errors users
would see because of this problem?  I'll update the commit log with
any of this information to help users connect an issue with this fix.

> Suggested-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Gregory Price <gourry@gourry.net>
> ---
>  drivers/pci/doe.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> index 652d63df9d22..27ba5d281384 100644
> --- a/drivers/pci/doe.c
> +++ b/drivers/pci/doe.c
> @@ -149,14 +149,26 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb,
>  	size_t length, remainder;
>  	u32 val;
>  	int i;
> +	unsigned long timeout_jiffies;
>  
>  	/*
>  	 * Check the DOE busy bit is not set. If it is set, this could indicate
>  	 * someone other than Linux (e.g. firmware) is using the mailbox. Note
>  	 * it is expected that firmware and OS will negotiate access rights via
>  	 * an, as yet to be defined, method.
> +	 *
> +	 * Wait up to one PCI_DOE_TIMEOUT period to allow the prior command to
> +	 * finish. Otherwise, simply error out as unable to field the request.
> +	 *
> +	 * PCIe r6.2 sec 6.30.3 states no interrupt is raised when the DOE Busy
> +	 * bit is cleared, so polling here is our best option for the moment.
>  	 */
> -	pci_read_config_dword(pdev, offset + PCI_DOE_STATUS, &val);
> +	timeout_jiffies = jiffies + PCI_DOE_TIMEOUT;
> +	do {
> +		pci_read_config_dword(pdev, offset + PCI_DOE_STATUS, &val);
> +	} while (FIELD_GET(PCI_DOE_STATUS_BUSY, val) &&
> +		 !time_after(jiffies, timeout_jiffies));
> +
>  	if (FIELD_GET(PCI_DOE_STATUS_BUSY, val))
>  		return -EBUSY;
>  
> -- 
> 2.43.0
> 

Re: [PATCH] PCI/DOE: Poll DOE Busy bit for up to 1 second in pci_doe_send_req

[Gregory Price]

On Thu, Oct 10, 2024 at 05:16:28PM -0500, Bjorn Helgaas wrote:
> On Fri, Oct 04, 2024 at 12:28:28PM -0400, Gregory Price wrote:
> > During initial device probe, the PCI DOE busy bit for some CXL
> > devices may be left set for a longer period than expected by the
> > current driver logic. Despite local comments stating DOE Busy is
> > unlikely to be detected, it appears commonly specifically during
> > boot when CXL devices are being probed.
> > 
> > This was observed on a single socket AMD platform with 2 CXL memory
> > expanders attached to the single socket. It was not the case that
> > concurrent accesses were being made, as validated by monitoring
> > mailbox commands on the device side.
> > 
> > This behavior has been observed with multiple CXL memory expanders
> > from different vendors - so it appears unrelated to the model.
> > 
> > In all observed tests, only a small period of the retry window is
> > actually used - typically only a handful of loop iterations.
> > 
> > Polling on the PCI DOE Busy Bit for (at max) one PCI DOE timeout
> > interval (1 second), resolves this issues cleanly.
> > 
> > Per PCIe r6.2 sec 6.30.3, the DOE Busy Bit being cleared does not
> > raise an interrupt, so polling is the best option in this scenario.
> > 
> > Subsqeuent code in doe_statemachine_work and abort paths also wait
> > for up to 1 PCI DOE timeout interval, so this order of (potential)
> > additional delay is presumed acceptable.
> 
> I provisionally applied this to pci/doe for v6.13 with Lukas and
> Jonathan's reviewed-by.  
> 
> Can we include a sample of any dmesg logging or other errors users
> would see because of this problem?  I'll update the commit log with
> any of this information to help users connect an issue with this fix.
>

The only indication in dmesg you will see is a line like

[   24.542625] endpoint6: DOE failed -EBUSY

produced by cxl_cdat_get_length or cxl_cdat_read_table


Do you want an updated patch with the nits fixed?
 
> > Suggested-by: Lukas Wunner <lukas@wunner.de>
> > Signed-off-by: Gregory Price <gourry@gourry.net>
> > ---
> >  drivers/pci/doe.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> > index 652d63df9d22..27ba5d281384 100644
> > --- a/drivers/pci/doe.c
> > +++ b/drivers/pci/doe.c
> > @@ -149,14 +149,26 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb,
> >  	size_t length, remainder;
> >  	u32 val;
> >  	int i;
> > +	unsigned long timeout_jiffies;
> >  
> >  	/*
> >  	 * Check the DOE busy bit is not set. If it is set, this could indicate
> >  	 * someone other than Linux (e.g. firmware) is using the mailbox. Note
> >  	 * it is expected that firmware and OS will negotiate access rights via
> >  	 * an, as yet to be defined, method.
> > +	 *
> > +	 * Wait up to one PCI_DOE_TIMEOUT period to allow the prior command to
> > +	 * finish. Otherwise, simply error out as unable to field the request.
> > +	 *
> > +	 * PCIe r6.2 sec 6.30.3 states no interrupt is raised when the DOE Busy
> > +	 * bit is cleared, so polling here is our best option for the moment.
> >  	 */
> > -	pci_read_config_dword(pdev, offset + PCI_DOE_STATUS, &val);
> > +	timeout_jiffies = jiffies + PCI_DOE_TIMEOUT;
> > +	do {
> > +		pci_read_config_dword(pdev, offset + PCI_DOE_STATUS, &val);
> > +	} while (FIELD_GET(PCI_DOE_STATUS_BUSY, val) &&
> > +		 !time_after(jiffies, timeout_jiffies));
> > +
> >  	if (FIELD_GET(PCI_DOE_STATUS_BUSY, val))
> >  		return -EBUSY;
> >  
> > -- 
> > 2.43.0
> > 

Re: [PATCH] PCI/DOE: Poll DOE Busy bit for up to 1 second in pci_doe_send_req

[Lukas Wunner]

On Fri, Oct 11, 2024 at 09:59:12AM -0400, Gregory Price wrote:
> On Thu, Oct 10, 2024 at 05:16:28PM -0500, Bjorn Helgaas wrote:
> > I provisionally applied this to pci/doe for v6.13 with Lukas and
> > Jonathan's reviewed-by.  
> > 
> > Can we include a sample of any dmesg logging or other errors users
> > would see because of this problem?  I'll update the commit log with
> > any of this information to help users connect an issue with this fix.
[...]
> Do you want an updated patch with the nits fixed?

You need to keep an eye on pci.git when contributing to the
PCI subsystem... ;)

https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git

Your patch is on the "doe" topic branch, with all nits already
addressed by Bjorn.  So you don't need to do anything. :)

Thanks,

Lukas

Re: [PATCH] PCI/DOE: Poll DOE Busy bit for up to 1 second in pci_doe_send_req

[Bjorn Helgaas]

On Fri, Oct 11, 2024 at 09:59:12AM -0400, Gregory Price wrote:
> On Thu, Oct 10, 2024 at 05:16:28PM -0500, Bjorn Helgaas wrote:
> > On Fri, Oct 04, 2024 at 12:28:28PM -0400, Gregory Price wrote:
> > > During initial device probe, the PCI DOE busy bit for some CXL
> > > devices may be left set for a longer period than expected by the
> > > current driver logic. Despite local comments stating DOE Busy is
> > > unlikely to be detected, it appears commonly specifically during
> > > boot when CXL devices are being probed.
> > > 
> > > This was observed on a single socket AMD platform with 2 CXL memory
> > > expanders attached to the single socket. It was not the case that
> > > concurrent accesses were being made, as validated by monitoring
> > > mailbox commands on the device side.
> > > 
> > > This behavior has been observed with multiple CXL memory expanders
> > > from different vendors - so it appears unrelated to the model.
> > > 
> > > In all observed tests, only a small period of the retry window is
> > > actually used - typically only a handful of loop iterations.
> > > 
> > > Polling on the PCI DOE Busy Bit for (at max) one PCI DOE timeout
> > > interval (1 second), resolves this issues cleanly.
> > > 
> > > Per PCIe r6.2 sec 6.30.3, the DOE Busy Bit being cleared does not
> > > raise an interrupt, so polling is the best option in this scenario.
> > > 
> > > Subsqeuent code in doe_statemachine_work and abort paths also wait
> > > for up to 1 PCI DOE timeout interval, so this order of (potential)
> > > additional delay is presumed acceptable.
> > 
> > I provisionally applied this to pci/doe for v6.13 with Lukas and
> > Jonathan's reviewed-by.  
> > 
> > Can we include a sample of any dmesg logging or other errors users
> > would see because of this problem?  I'll update the commit log with
> > any of this information to help users connect an issue with this fix.
> >
> 
> The only indication in dmesg you will see is a line like
> 
> [   24.542625] endpoint6: DOE failed -EBUSY
> 
> produced by cxl_cdat_get_length or cxl_cdat_read_table
> 
> 
> Do you want an updated patch with the nits fixed?

No need, I fixed the nits and added the dmesg line to the commit log.
Thank you!

Bjorn