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@xxxxxxxxx>
> > Signed-off-by: Gregory Price <gourry@xxxxxxxxxx>
> > ---
> > 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
> >
