diff mbox series

[V13,8/9] cxl/port: Retry reading CDAT on failure

Message ID 20220705154932.2141021-9-ira.weiny@intel.com (mailing list archive)
State Handled Elsewhere
Headers show
Series CXL: Read CDAT and DSMAS data | expand

Commit Message

Ira Weiny July 5, 2022, 3:49 p.m. UTC 
From: Ira Weiny <ira.weiny@intel.com>

The CDAT read may fail for a number of reasons but mainly it is possible
to get different parts of a valid state.  The checksum in the CDAT table
protects against this.

Now that the cdat data is validated, issue a retry if the CDAT read
fails.  For now 5 retries are implemented.

Reviewed-by: Ben Widawsky <bwidawsk@kernel.org>
Reviewed-by: Alison Schofield <alison.schofield@intel.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Changes from V12
	Jonathan:
		Standardize on 'rc' for return code
		Don't set rc unnecessarily

Changes from V10
	Pick up review tag and fix commit message

Changes from V9
	Alison Schofield/Davidlohr Bueso
		Print debug on each iteration and error only after failure

Changes from V8
	Move code to cxl/core/pci.c

Changes from V6
	Move to pci.c
	Fix retries count
	Change to 5 retries

Changes from V5:
	New patch -- easy to push off or drop.
---
 drivers/cxl/core/pci.c | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

Comments

Dan Williams July 14, 2022, 4:27 p.m. UTC | #1 
ira.weiny@ wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> The CDAT read may fail for a number of reasons but mainly it is possible
> to get different parts of a valid state.  The checksum in the CDAT table
> protects against this.

I don't know what "different parts of a valid state" means. The CDAT
should not be changing as it is being read unless someone is issuing a
set-partition while the DOE operation is happening. Rather than
arbitrary retries, block out set-partition while CDAT is being read.

You can use {set,clear}_exclusive_cxl_commands() to temporarily lock out
set-partition while the CDAT read is happening.

...and since this series is only for enabling
Ira Weiny July 14, 2022, 8:05 p.m. UTC | #2 
On Thu, Jul 14, 2022 at 09:27:04AM -0700, Dan Williams wrote:
> ira.weiny@ wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > The CDAT read may fail for a number of reasons but mainly it is possible
> > to get different parts of a valid state.  The checksum in the CDAT table
> > protects against this.
> 
> I don't know what "different parts of a valid state" means.

This text is stale but given what I know about how other entities may be
issuing queries without the kernel knowledge I'm not 100% sure that the data
read back will always be valid.

Regardless, this has already caught a bug in QEMU.

So I'm inclined to leave this check in because the checksum is there and should
can be validated if only to detect broken hardware.

I can update the commit message to clarify this.

Ira

>
> The CDAT
> should not be changing as it is being read unless someone is issuing a
> set-partition while the DOE operation is happening. Rather than
> arbitrary retries, block out set-partition while CDAT is being read.
> 
> You can use {set,clear}_exclusive_cxl_commands() to temporarily lock out
> set-partition while the CDAT read is happening.
> 
> ...and since this series is only for enabling
Ira Weiny July 14, 2022, 8:13 p.m. UTC | #3 
On Thu, Jul 14, 2022 at 01:05:47PM -0700, Ira wrote:
> On Thu, Jul 14, 2022 at 09:27:04AM -0700, Dan Williams wrote:
> > ira.weiny@ wrote:
> > > From: Ira Weiny <ira.weiny@intel.com>
> > > 
> > > The CDAT read may fail for a number of reasons but mainly it is possible
> > > to get different parts of a valid state.  The checksum in the CDAT table
> > > protects against this.
> > 
> > I don't know what "different parts of a valid state" means.
> 
> This text is stale but given what I know about how other entities may be
> issuing queries without the kernel knowledge I'm not 100% sure that the data
> read back will always be valid.
> 
> Regardless, this has already caught a bug in QEMU.
> 
> So I'm inclined to leave this check in because the checksum is there and should
> can be validated if only to detect broken hardware.
> 
> I can update the commit message to clarify this.

Oh wait I thought this was the 'is valid' patch.

I can remove the retries if that was all you were concerned about.

Ira

> 
> Ira
> 
> >
> > The CDAT
> > should not be changing as it is being read unless someone is issuing a
> > set-partition while the DOE operation is happening. Rather than
> > arbitrary retries, block out set-partition while CDAT is being read.
> > 
> > You can use {set,clear}_exclusive_cxl_commands() to temporarily lock out
> > set-partition while the CDAT read is happening.
> > 
> > ...and since this series is only for enabling
Dan Williams July 14, 2022, 8:44 p.m. UTC | #4 
Ira Weiny wrote:
> On Thu, Jul 14, 2022 at 01:05:47PM -0700, Ira wrote:
> > On Thu, Jul 14, 2022 at 09:27:04AM -0700, Dan Williams wrote:
> > > ira.weiny@ wrote:
> > > > From: Ira Weiny <ira.weiny@intel.com>
> > > > 
> > > > The CDAT read may fail for a number of reasons but mainly it is possible
> > > > to get different parts of a valid state.  The checksum in the CDAT table
> > > > protects against this.
> > > 
> > > I don't know what "different parts of a valid state" means.
> > 
> > This text is stale but given what I know about how other entities may be
> > issuing queries without the kernel knowledge I'm not 100% sure that the data
> > read back will always be valid.
> > 
> > Regardless, this has already caught a bug in QEMU.
> > 
> > So I'm inclined to leave this check in because the checksum is there and should
> > can be validated if only to detect broken hardware.
> > 
> > I can update the commit message to clarify this.
> 
> Oh wait I thought this was the 'is valid' patch.
> 
> I can remove the retries if that was all you were concerned about.
> 

I was concerned that this patch was trying to accommodate CDAT changes
while the retrieval is running which should be obviated by not allowing
set-partition while the CDAT retrieval is running. So I want to see
single-shot CDAT retrieval underneath set-partition protection.
Jonathan Cameron July 19, 2022, 3:07 p.m. UTC | #5 
On Thu, 14 Jul 2022 09:27:04 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> ira.weiny@ wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > The CDAT read may fail for a number of reasons but mainly it is possible
> > to get different parts of a valid state.  The checksum in the CDAT table
> > protects against this.  
> 
> I don't know what "different parts of a valid state" means. The CDAT
> should not be changing as it is being read unless someone is issuing a
> set-partition while the DOE operation is happening.

Unfortunately not true. The device is allowed to change it with no input
from OS software at all.

From CDAT spec

"For Revision=1, the following changes are permitted during the
runtime
• Changes to the latency and bandwidth fields in DSLBIS
• Changes to the latency and bandwidth fields in SSLBIS
• Changes to the number of DSEMTS instances and their
contents
The changes to latency and bandwidth may represent events such
as failover or degradation that are internal to a component."

> Rather than
> arbitrary retries, block out set-partition while CDAT is being read.

Blocking that out is still useful even though we probably still need retries.

> 
> You can use {set,clear}_exclusive_cxl_commands() to temporarily lock out
> set-partition while the CDAT read is happening.
> 
> ...and since this series is only for enabling
diff mbox series

Patch

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 0a1620c302e1..0853885c5767 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -623,13 +623,7 @@  static int cxl_cdat_read_table(struct device *dev,
 	return 0;
 }
 
-/**
- * read_cdat_data - Read the CDAT data on this port
- * @port: Port to read data from
- *
- * This call will sleep waiting for responses from the DOE mailbox.
- */
-void read_cdat_data(struct cxl_port *port)
+static int __read_cdat_data(struct cxl_port *port)
 {
 	static struct pci_doe_mb *cdat_mb;
 	struct device *dev = &port->dev;
@@ -640,19 +634,19 @@  void read_cdat_data(struct cxl_port *port)
 	cdat_mb = find_cdat_mb(uport);
 	if (!cdat_mb) {
 		dev_dbg(dev, "No CDAT mailbox\n");
-		return;
+		return -EIO;
 	}
 
 	port->cdat_sup = true;
 
 	if (cxl_cdat_get_length(dev, cdat_mb, &cdat_length)) {
 		dev_dbg(dev, "No CDAT length\n");
-		return;
+		return -EIO;
 	}
 
 	port->cdat.table = devm_kzalloc(dev, cdat_length, GFP_KERNEL);
 	if (!port->cdat.table)
-		return;
+		return -ENOMEM;
 
 	port->cdat.length = cdat_length;
 	rc = cxl_cdat_read_table(dev, cdat_mb, &port->cdat);
@@ -663,5 +657,30 @@  void read_cdat_data(struct cxl_port *port)
 		port->cdat.length = 0;
 		dev_err(dev, "CDAT data read error\n");
 	}
+
+	return rc;
+}
+
+/**
+ * read_cdat_data - Read the CDAT data on this port
+ * @port: Port to read data from
+ *
+ * This call will sleep waiting for responses from the DOE mailbox.
+ */
+void read_cdat_data(struct cxl_port *port)
+{
+	int retries = 5;
+	int rc;
+
+	while (retries--) {
+		rc = __read_cdat_data(port);
+		if (!rc)
+			return;
+		dev_dbg(&port->dev,
+			"CDAT data read error rc=%d (retries %d)\n",
+			rc, retries);
+	}
+	dev_err(&port->dev, "CDAT data read failed after %d retries\n",
+		retries);
 }
 EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);