[PATCH] pci/doe: add a 1 second retry window to pci_doe

[PATCH] pci/doe: add a 1 second retry window to pci_doe

[Gregory Price]

Depending on the device, sometimes firmware clears the busy flag
later than expected.  This can cause the device to appear busy when
calling multiple commands in quick sucession. Add a 1 second retry
window to all doe commands that end with -EBUSY.

Signed-off-by: Gregory Price <gourry@gourry.net>
---
 drivers/pci/doe.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
index 652d63df9d22..5573fa1a0008 100644
--- a/drivers/pci/doe.c
+++ b/drivers/pci/doe.c
@@ -647,12 +647,16 @@ int pci_doe(struct pci_doe_mb *doe_mb, u16 vendor, u8 type,
 		.private = &c,
 	};
 	int rc;
+	unsigned long timeout_jiffies = jiffies + (PCI_DOE_TIMEOUT * 1);
 
-	rc = pci_doe_submit_task(doe_mb, &task);
-	if (rc)
-		return rc;
+	do {
+		rc = pci_doe_submit_task(doe_mb, &task);
+
+		if (rc)
+			return rc;
 
-	wait_for_completion(&c);
+		wait_for_completion(&c);
+	} while (task.rv == -EBUSY && !time_after(jiffies, timeout_jiffies));
 
 	return task.rv;
 }
-- 
2.43.0

Re: [PATCH] pci/doe: add a 1 second retry window to pci_doe

[Dan Williams]

[ add linux-pci and Lukas ]

Gregory Price wrote:
> Depending on the device, sometimes firmware clears the busy flag
> later than expected.  This can cause the device to appear busy when
> calling multiple commands in quick sucession. Add a 1 second retry
> window to all doe commands that end with -EBUSY.

I would have expected this to be handled as part of finishing off
pci_doe_recv_resp() not retrying on a new submission.

It also occurs to me that instead of warning "another entity is sending conflicting
requests" message, the doe core should just ensure that it is the only
agent using the mailbox. Something like hold the PCI config lock over
DOE transactions. Then it will remove ambiguity of "conflicting agent"
vs "device is slow to clear BUSY".

Re: [PATCH] pci/doe: add a 1 second retry window to pci_doe

[Jonathan Cameron]

On Fri, 13 Sep 2024 22:32:28 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> [ add linux-pci and Lukas ]
> 
> Gregory Price wrote:
> > Depending on the device, sometimes firmware clears the busy flag
> > later than expected.  This can cause the device to appear busy when
> > calling multiple commands in quick sucession. Add a 1 second retry
> > window to all doe commands that end with -EBUSY.  
> 
> I would have expected this to be handled as part of finishing off
> pci_doe_recv_resp() not retrying on a new submission.
> 
> It also occurs to me that instead of warning "another entity is sending conflicting
> requests" message, the doe core should just ensure that it is the only
> agent using the mailbox. Something like hold the PCI config lock over
> DOE transactions. Then it will remove ambiguity of "conflicting agent"
> vs "device is slow to clear BUSY".
> 

I believe we put that dance in to not fail too horribly
if a firmware was messing with the DOE behind our backs rather than
another OS level actor was messing with it.

We wouldn't expect firmware to be using a DOE that Linux wants, but
the problem is the discovery protocol which the firmware might run
to find the DOE it does want to use.

My memory might be wrong though as this was a while back.

Jonathan

Re: [PATCH] pci/doe: add a 1 second retry window to pci_doe

[Gregory Price]

On Mon, Sep 16, 2024 at 10:15:57AM +0100, Jonathan Cameron wrote:
> On Fri, 13 Sep 2024 22:32:28 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > [ add linux-pci and Lukas ]
> > 
> > Gregory Price wrote:
> > > Depending on the device, sometimes firmware clears the busy flag
> > > later than expected.  This can cause the device to appear busy when
> > > calling multiple commands in quick sucession. Add a 1 second retry
> > > window to all doe commands that end with -EBUSY.  
> > 
> > I would have expected this to be handled as part of finishing off
> > pci_doe_recv_resp() not retrying on a new submission.
> > 
> > It also occurs to me that instead of warning "another entity is sending conflicting
> > requests" message, the doe core should just ensure that it is the only
> > agent using the mailbox. Something like hold the PCI config lock over
> > DOE transactions. Then it will remove ambiguity of "conflicting agent"
> > vs "device is slow to clear BUSY".
> > 
> 
> I believe we put that dance in to not fail too horribly
> if a firmware was messing with the DOE behind our backs rather than
> another OS level actor was messing with it.
> 
> We wouldn't expect firmware to be using a DOE that Linux wants, but
> the problem is the discovery protocol which the firmware might run
> to find the DOE it does want to use.
> 
> My memory might be wrong though as this was a while back.
> 
> Jonathan

Just following up here, it sounds like everyone is unsure of this change.

I can confirm that this handles the CDAT retry issue I am seeing, and that
the BUSY bit is set upon entry into the initial call. Only 1 or 2 retries
are attempted before it is cleared and returns successfully.

I'd explored putting the retry logic in the CDAT code that calls into here,
but that just seemed wrong.  Is there a suggestion or a nak here?

Trying to find a path forward.

~Gregory

Re: [PATCH] pci/doe: add a 1 second retry window to pci_doe

[Lukas Wunner]

On Tue, Oct 01, 2024 at 11:13:17AM -0400, Gregory Price wrote:
> > > Gregory Price wrote:
> > > > Depending on the device, sometimes firmware clears the busy flag
> > > > later than expected.  This can cause the device to appear busy when
> > > > calling multiple commands in quick sucession. Add a 1 second retry
> > > > window to all doe commands that end with -EBUSY.  
> 
> Just following up here, it sounds like everyone is unsure of this change.
> 
> I can confirm that this handles the CDAT retry issue I am seeing, and that
> the BUSY bit is set upon entry into the initial call. Only 1 or 2 retries
> are attempted before it is cleared and returns successfully.
> 
> I'd explored putting the retry logic in the CDAT code that calls into here,
> but that just seemed wrong.  Is there a suggestion or a nak here?
> 
> Trying to find a path forward.

The PCIe Base Spec doesn't prescribe a maximum timeout for the
DOE BUSY bit to clear.  Thus it seems fine to me in principle
to add a (or raise the) timeout if it turns out to be necessary
for real-life hardware.

That said, the proposed patch has room for improvement:

* The patch seems to wait for DOE BUSY bit to clear *after*
  completion.  That's odd.  The kernel waits for DOE Busy bit
  to clear *before* sending a new request, in pci_doe_send_req().
  My expectation would have been that you'd add a loop there which
  polls for DOE Busy bit to clear before sending a request.

  It seems that polling is the only option as no interrupt is
  raised on DOE Busy bit clear, per PCIe r6.2 sec 6.30.3.
  (Please add this bit of information to the commit message.)

* The commit message should clearly specify the device(s)
  affected by the issue (Vendor and Device ID plus name).
  Comments such as "Depending on the device, sometimes ..."
  are a little too vague.

* The "1 or 2 retries" bit of information you're mentioning
  above should likewise be in the commit message.

* Please use "PCI/DOE:" as subject prefix to match previous
  commits which touched drivers/pci/doe.c.

* Please adhere to spec language, e.g. use "DOE Busy bit"
  instead of "busy bit" so it's unambiguous for readers
  what you're referring to.

Thanks,

Lukas

Re: [PATCH] pci/doe: add a 1 second retry window to pci_doe

[Gregory Price]

On Tue, Oct 01, 2024 at 05:47:03PM +0200, Lukas Wunner wrote:
> On Tue, Oct 01, 2024 at 11:13:17AM -0400, Gregory Price wrote:
> > > > Gregory Price wrote:
> > > > > Depending on the device, sometimes firmware clears the busy flag
> > > > > later than expected.  This can cause the device to appear busy when
> > > > > calling multiple commands in quick sucession. Add a 1 second retry
> > > > > window to all doe commands that end with -EBUSY.  
> > 
> > Just following up here, it sounds like everyone is unsure of this change.
> > 
> > I can confirm that this handles the CDAT retry issue I am seeing, and that
> > the BUSY bit is set upon entry into the initial call. Only 1 or 2 retries
> > are attempted before it is cleared and returns successfully.
> > 
> > I'd explored putting the retry logic in the CDAT code that calls into here,
> > but that just seemed wrong.  Is there a suggestion or a nak here?
> > 
> > Trying to find a path forward.
> 
> The PCIe Base Spec doesn't prescribe a maximum timeout for the
> DOE BUSY bit to clear.  Thus it seems fine to me in principle
> to add a (or raise the) timeout if it turns out to be necessary
> for real-life hardware.
> 
> That said, the proposed patch has room for improvement:

Will address and resubmit, thanks!

> 
> * The patch seems to wait for DOE BUSY bit to clear *after*
>   completion.  That's odd.  The kernel waits for DOE Busy bit
>   to clear *before* sending a new request, in pci_doe_send_req().
>   My expectation would have been that you'd add a loop there which
>   polls for DOE Busy bit to clear before sending a request.
> 
>   It seems that polling is the only option as no interrupt is
>   raised on DOE Busy bit clear, per PCIe r6.2 sec 6.30.3.
>   (Please add this bit of information to the commit message.)
> 
> * The commit message should clearly specify the device(s)
>   affected by the issue (Vendor and Device ID plus name).
>   Comments such as "Depending on the device, sometimes ..."
>   are a little too vague.
> 
> * The "1 or 2 retries" bit of information you're mentioning
>   above should likewise be in the commit message.
> 
> * Please use "PCI/DOE:" as subject prefix to match previous
>   commits which touched drivers/pci/doe.c.
> 
> * Please adhere to spec language, e.g. use "DOE Busy bit"
>   instead of "busy bit" so it's unambiguous for readers
>   what you're referring to.
> 
> Thanks,
> 
> Lukas

Re: [PATCH] pci/doe: add a 1 second retry window to pci_doe

[Jonathan Cameron]

On Tue, 1 Oct 2024 17:47:03 +0200
Lukas Wunner <lukas@wunner.de> wrote:

> On Tue, Oct 01, 2024 at 11:13:17AM -0400, Gregory Price wrote:
> > > > Gregory Price wrote:  
> > > > > Depending on the device, sometimes firmware clears the busy flag
> > > > > later than expected.  This can cause the device to appear busy when
> > > > > calling multiple commands in quick sucession. Add a 1 second retry
> > > > > window to all doe commands that end with -EBUSY.    
> > 
> > Just following up here, it sounds like everyone is unsure of this change.
> > 
> > I can confirm that this handles the CDAT retry issue I am seeing, and that
> > the BUSY bit is set upon entry into the initial call. Only 1 or 2 retries
> > are attempted before it is cleared and returns successfully.
> > 
> > I'd explored putting the retry logic in the CDAT code that calls into here,
> > but that just seemed wrong.  Is there a suggestion or a nak here?
> > 
> > Trying to find a path forward.  
> 
> The PCIe Base Spec doesn't prescribe a maximum timeout for the
> DOE BUSY bit to clear.  Thus it seems fine to me in principle
> to add a (or raise the) timeout if it turns out to be necessary
> for real-life hardware.
> 
> That said, the proposed patch has room for improvement:
> 
> * The patch seems to wait for DOE BUSY bit to clear *after*
>   completion.  That's odd.  The kernel waits for DOE Busy bit
>   to clear *before* sending a new request, in pci_doe_send_req().
>   My expectation would have been that you'd add a loop there which
>   polls for DOE Busy bit to clear before sending a request.
> 
>   It seems that polling is the only option as no interrupt is
>   raised on DOE Busy bit clear, per PCIe r6.2 sec 6.30.3.
>   (Please add this bit of information to the commit message.)

This changed at some point.  In PCI 6.0 the clearing
of this bit is explicitly called out in DOE interrupt status
as a reason to trigger the interrupt. By 6.1 that's gone.
This was a problem for the original implementation as we had
to assume that we'd get random spurious interrupts because
of that delight.

Anyhow, hopefully doesn't matter to us here as you are correct
that we have to poll for it.

Mind you we still have to allow for spurious garbage interrupts
and eat them silently. :(

> 
> * The commit message should clearly specify the device(s)
>   affected by the issue (Vendor and Device ID plus name).
>   Comments such as "Depending on the device, sometimes ..."
>   are a little too vague.
> 
> * The "1 or 2 retries" bit of information you're mentioning
>   above should likewise be in the commit message.
> 
> * Please use "PCI/DOE:" as subject prefix to match previous
>   commits which touched drivers/pci/doe.c.
> 
> * Please adhere to spec language, e.g. use "DOE Busy bit"
>   instead of "busy bit" so it's unambiguous for readers
>   what you're referring to.
> 
> Thanks,
> 
> Lukas
>