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

On 21-11-29 11:18:36, Dan Williams wrote:

[snip]

quoted

quoted

quoted

quoted

quoted

-       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.

The message is wrong, but I think the logic is still valuable. How about:
"mbox: reported interface ready, but mbox not ready"

You mean check this every time even though the spec says the driver
only needs to check it once per-reset?

Unfortunately it does not say that. "... it shall remain set until the next
reset or the device encounters an error that prevents any mailbox
communication."

Once we have real error checking in place, this could go away, though I see no
harm in leaving it.

Right, there's no harm in the check, it just seems overly paranoid to
me if it was already checked once. Until a doorbell timeout happens
it's an extra MMIO cycle that can saved for a "what happened?" check
after a timeout.

Well I suspect we're just rearranging the deck chairs on the Titanic now, but...

I see doorbell timeouts as disconnected from whether or not the mailbox
interface is ready. If they were the same, we wouldn't need both bits and we
could just wait extra long for the doorbell when probing.

In other words, I expect if the interface goes unready, doorbell timeout will
occur, but I don't think we should assume if doorbell timeout occurs, the
interface is no longer ready. I don't purport to know why a doorbell timeout
might occur while the interface remains available (likely a firmware bug, I
presume).

It does seem interesting to check if the interface is no longer ready on timeout
though.