Thread (127 messages) 127 messages, 9 authors, 2021-12-07

Re: [PATCH 05/23] cxl/pci: Don't poll doorbell for mailbox access

flat view

From: Dan Williams <hidden>
Date: 2021-11-24 21:55:16
Also in: linux-cxl

On Fri, Nov 19, 2021 at 4:03 PM Ben Widawsky [off-list ref] wrote:
quoted hunk ↗ jump to hunk
The expectation is that the mailbox interface ready bit is the first
step in access through the mailbox interface. Therefore, waiting for the
doorbell busy bit to be clear would imply that the mailbox interface is
ready. The original driver implementation used the doorbell timeout for
the Mailbox Interface Ready bit to piggyback off of, since the latter
doesn't have a defined timeout (introduced in 8adaf747c9f0 ("cxl/mem:
Find device capabilities"), a timeout has since been defined with an ECN
to the 2.0 spec). With the current driver waiting for mailbox interface
ready as a part of probe() it's no longer necessary to use the
piggyback.

With the piggybacking no longer necessary it doesn't make sense to check
doorbell status when acquiring the mailbox. It will be checked during
the normal mailbox exchange protocol.

Signed-off-by: Ben Widawsky <redacted>
---
This patch did not exist in RFCv2
---
 drivers/cxl/pci.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 2cef9fec8599..869b4fc18e27 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -221,27 +221,14 @@ static int cxl_pci_mbox_get(struct cxl_dev_state *cxlds)

        /*
         * XXX: There is some amount of ambiguity in the 2.0 version of the spec
-        * around the mailbox interface ready (8.2.8.5.1.1).  The purpose of the
+        * around the mailbox interface ready (8.2.8.5.1.1). The purpose of the
         * bit is to allow firmware running on the device to notify the driver
-        * that it's ready to receive commands. It is unclear if the bit needs
-        * to be read for each transaction mailbox, ie. the firmware can switch
-        * it on and off as needed. Second, there is no defined timeout for
-        * mailbox ready, like there is for the doorbell interface.
-        *
-        * Assumptions:
-        * 1. The firmware might toggle the Mailbox Interface Ready bit, check
-        *    it for every command.
-        *
-        * 2. If the doorbell is clear, the firmware should have first set the
-        *    Mailbox Interface Ready bit. Therefore, waiting for the doorbell
-        *    to be ready is sufficient.
+        * that it's ready to receive commands. The spec does not clearly define
+        * under what conditions the bit may get set or cleared. As of the 2.0
+        * base specification there was no defined timeout for mailbox ready,
+        * like there is for the doorbell interface. This was fixed with an ECN,
+        * but it's possible early devices implemented this before the ECN.
Can we just drop comment block altogether? Outside of
cxl_pci_setup_mailbox() the only time the mailbox status should be
checked is after a doorbell timeout after submitting a command.
quoted hunk ↗ jump to hunk
         */
-       rc = cxl_pci_mbox_wait_for_doorbell(cxlds);
-       if (rc) {
-               dev_warn(dev, "Mailbox interface not ready\n");
-               goto out;
-       }
-
        md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
        if (!(md_status & CXLMDEV_MBOX_IF_READY && CXLMDEV_READY(md_status))) {
                dev_err(dev, "mbox: reported doorbell ready, but not mbox ready\n");
This error message is obsolete since nothing is pre-checking the
mailbox anymore, and per above I see no problem waiting to check the
status until after the mailbox has failed to respond after a timeout.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help