| Message ID | 20240913183241.17320-1-gourry@gourry.net |
|---|---|
| State | New |
| Headers | show |
| Series | pci/doe: add a 1 second retry window to pci_doe | expand |
[ 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".
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
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
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
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
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 >
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; }
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(-)