| Message ID | 20220628041527.742333-9-ira.weiny@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | CXL: Read CDAT and DSMAS data | expand |
On Mon, 27 Jun 2022 21:15:26 -0700 ira.weiny@intel.com 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. > > 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 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 | 41 ++++++++++++++++++++++++++++++----------- > 1 file changed, 30 insertions(+), 11 deletions(-) > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c > index 6d775cc3dca1..d7c2a415cc5f 100644 > --- a/drivers/cxl/core/pci.c > +++ b/drivers/cxl/core/pci.c > @@ -618,36 +618,30 @@ static int cxl_cdat_read_table(struct device *dev, > 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) > +static int __read_cdat_data(struct cxl_port *port) > { > static struct pci_doe_mb *cdat_mb; > struct device *dev = &port->dev; > struct device *uport = port->uport; > size_t cdat_length; > - int ret; > + int ret = 0; Fairly sure there isn't a path in which ret isn't set... Mixing ret and rc is a bit inconsistent, maybe scrub patch set for one or the other. (My fault originally I think :) > > 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; > ret = cxl_cdat_read_table(dev, cdat_mb, &port->cdat); > @@ -658,5 +652,30 @@ void read_cdat_data(struct cxl_port *port) > port->cdat.length = 0; > dev_err(dev, "CDAT data read error\n"); > } > + > + return ret; > +} > + > +/** > + * 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);
On Tue, Jun 28, 2022 at 03:57:20PM +0100, Jonathan Cameron wrote: > On Mon, 27 Jun 2022 21:15:26 -0700 > ira.weiny@intel.com wrote: > [snip] > > > > -/** > > - * 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; > > struct device *uport = port->uport; > > size_t cdat_length; > > - int ret; > > + int ret = 0; > Fairly sure there isn't a path in which ret isn't set... > Yep. > > Mixing ret and rc is a bit inconsistent, maybe scrub patch set for > one or the other. (My fault originally I think :) Ok PCI uses both ret and rc. :-( But CXL seems to be consistent with rc. So I've used rc with the new series which I think satisfies both subsystems. Thanks again for the detail review of the series. Hopefully there will be a new version out tomorrow. Ira > > > > > > 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; > > ret = cxl_cdat_read_table(dev, cdat_mb, &port->cdat); > > @@ -658,5 +652,30 @@ void read_cdat_data(struct cxl_port *port) > > port->cdat.length = 0; > > dev_err(dev, "CDAT data read error\n"); > > } > > + > > + return ret; > > +} > > + > > +/** > > + * 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); >
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c index 6d775cc3dca1..d7c2a415cc5f 100644 --- a/drivers/cxl/core/pci.c +++ b/drivers/cxl/core/pci.c @@ -618,36 +618,30 @@ static int cxl_cdat_read_table(struct device *dev, 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) +static int __read_cdat_data(struct cxl_port *port) { static struct pci_doe_mb *cdat_mb; struct device *dev = &port->dev; struct device *uport = port->uport; size_t cdat_length; - int ret; + int ret = 0; 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; ret = cxl_cdat_read_table(dev, cdat_mb, &port->cdat); @@ -658,5 +652,30 @@ void read_cdat_data(struct cxl_port *port) port->cdat.length = 0; dev_err(dev, "CDAT data read error\n"); } + + return ret; +} + +/** + * 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);