cxl/pci: Get rid of pointer arithmetic reading CDAT table

Reading the CDAT table using DOE requires a Table Access Response
Header in addition to the CDAT entry. In current implementation this
has caused offsets with sizeof(__le32) to the actual buffers. This led
to hardly readable code and even bugs. E.g., see fix of devm_kfree()
in read_cdat_data():

 commit c65efe3685f5 ("cxl/cdat: Free correct buffer on checksum error")

Rework code to avoid calculations with sizeof(__le32). Introduce
struct cdat_doe_rsp for this which contains the Table Access Response
Header and a variable payload size for various data structures
afterwards to access the CDAT table and its CDAT Data Structures
without recalculating buffer offsets.

Cc: Lukas Wunner <lukas@wunner.de>
Cc: Fan Ni <nifan.cxl@gmail.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Robert Richter <rrichter@amd.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Link: https://lore.kernel.org/r/20240216155844.406996-3-rrichter@amd.com
Signed-off-by: Dan Williams <dan.j.williams@intel.com>

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index c8214990c230ac5ca31dfbd27bc124a2dc2e2b48..184deff887d0340c2e8a1cecd8f86d36a193c729 100644 (file)
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -544,55 +544,57 @@ static int cxl_cdat_get_length(struct device *dev,
 
 static int cxl_cdat_read_table(struct device *dev,
                               struct pci_doe_mb *doe_mb,
-                              void *cdat_table, size_t *cdat_length)
+                              struct cdat_doe_rsp *rsp, size_t *length)
 {
-       size_t length = *cdat_length + sizeof(__le32);
-       __le32 *data = cdat_table;
-       int entry_handle = 0;
+       size_t received, remaining = *length;
+       unsigned int entry_handle = 0;
+       union cdat_data *data;
        __le32 saved_dw = 0;
 
        do {
                __le32 request = CDAT_DOE_REQ(entry_handle);
-               struct cdat_entry_header *entry;
-               size_t entry_dw;
                int rc;
 
                rc = pci_doe(doe_mb, PCI_DVSEC_VENDOR_ID_CXL,
                             CXL_DOE_PROTOCOL_TABLE_ACCESS,
                             &request, sizeof(request),
-                            data, length);
+                            rsp, sizeof(*rsp) + remaining);
                if (rc < 0) {
                        dev_err(dev, "DOE failed: %d", rc);
                        return rc;
                }
 
-               /* 1 DW Table Access Response Header + CDAT entry */
-               entry = (struct cdat_entry_header *)(data + 1);
-               if ((entry_handle == 0 &&
-                    rc != sizeof(__le32) + sizeof(struct cdat_header)) ||
-                   (entry_handle > 0 &&
-                    (rc < sizeof(__le32) + sizeof(*entry) ||
-                     rc != sizeof(__le32) + le16_to_cpu(entry->length))))
+               if (rc < sizeof(*rsp))
                        return -EIO;
 
+               data = (union cdat_data *)rsp->data;
+               received = rc - sizeof(*rsp);
+
+               if (entry_handle == 0) {
+                       if (received != sizeof(data->header))
+                               return -EIO;
+               } else {
+                       if (received < sizeof(data->entry) ||
+                           received != le16_to_cpu(data->entry.length))
+                               return -EIO;
+               }
+
                /* Get the CXL table access header entry handle */
                entry_handle = FIELD_GET(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE,
-                                        le32_to_cpu(data[0]));
-               entry_dw = rc / sizeof(__le32);
-               /* Skip Header */
-               entry_dw -= 1;
+                                        le32_to_cpu(rsp->doe_header));
+
                /*
                 * Table Access Response Header overwrote the last DW of
                 * previous entry, so restore that DW
                 */
-               *data = saved_dw;
-               length -= entry_dw * sizeof(__le32);
-               data += entry_dw;
-               saved_dw = *data;
+               rsp->doe_header = saved_dw;
+               remaining -= received;
+               rsp = (void *)rsp + received;
+               saved_dw = rsp->doe_header;
        } while (entry_handle != CXL_DOE_TABLE_ACCESS_LAST_ENTRY);
 
        /* Length in CDAT header may exceed concatenation of CDAT entries */
-       *cdat_length -= length - sizeof(__le32);
+       *length -= remaining;
 
        return 0;
 }
@@ -620,8 +622,8 @@ void read_cdat_data(struct cxl_port *port)
        struct pci_doe_mb *doe_mb;
        struct pci_dev *pdev = NULL;
        struct cxl_memdev *cxlmd;
-       size_t cdat_length;
-       void *cdat_table, *cdat_buf;
+       struct cdat_doe_rsp *buf;
+       size_t length;
        int rc;
 
        if (is_cxl_memdev(uport)) {
@@ -647,30 +649,33 @@ void read_cdat_data(struct cxl_port *port)
 
        port->cdat_available = true;
 
-       if (cxl_cdat_get_length(dev, doe_mb, &cdat_length)) {
+       if (cxl_cdat_get_length(dev, doe_mb, &length)) {
                dev_dbg(dev, "No CDAT length\n");
                return;
        }
 
-       cdat_buf = devm_kzalloc(dev, cdat_length + sizeof(__le32), GFP_KERNEL);
-       if (!cdat_buf)
-               return;
+       /*
+        * The begin of the CDAT buffer needs space for additional 4
+        * bytes for the DOE header. Table data starts afterwards.
+        */
+       buf = devm_kzalloc(dev, sizeof(*buf) + length, GFP_KERNEL);
+       if (!buf)
+               goto err;
 
-       rc = cxl_cdat_read_table(dev, doe_mb, cdat_buf, &cdat_length);
+       rc = cxl_cdat_read_table(dev, doe_mb, buf, &length);
        if (rc)
                goto err;
 
-       cdat_table = cdat_buf + sizeof(__le32);
-       if (cdat_checksum(cdat_table, cdat_length))
+       if (cdat_checksum(buf->data, length))
                goto err;
 
-       port->cdat.table = cdat_table;
-       port->cdat.length = cdat_length;
-       return;
+       port->cdat.table = buf->data;
+       port->cdat.length = length;
 
+       return;
 err:
        /* Don't leave table data allocated on error */
-       devm_kfree(dev, cdat_buf);
+       devm_kfree(dev, buf);
        dev_err(dev, "Failed to read/validate CDAT.\n");
 }
 EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);

diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
index 711b05d9a370e91b49beffbc5494542d8861d31e..93992a1c8eecf683edb72a5035e4a25872e64668 100644 (file)
--- a/drivers/cxl/cxlpci.h
+++ b/drivers/cxl/cxlpci.h
@@ -71,6 +71,15 @@ enum cxl_regloc_type {
        CXL_REGLOC_RBI_TYPES
 };
 
+/*
+ * Table Access DOE, CDAT Read Entry Response
+ *
+ * Spec refs:
+ *
+ * CXL 3.1 8.1.11, Table 8-14: Read Entry Response
+ * CDAT Specification 1.03: 2 CDAT Data Structures
+ */
+
 struct cdat_header {
        __le32 length;
        u8 revision;
@@ -85,6 +94,21 @@ struct cdat_entry_header {
        __le16 length;
 } __packed;
 
+/*
+ * The DOE CDAT read response contains a CDAT read entry (either the
+ * CDAT header or a structure).
+ */
+union cdat_data {
+       struct cdat_header header;
+       struct cdat_entry_header entry;
+} __packed;
+
+/* There is an additional CDAT response header of 4 bytes. */
+struct cdat_doe_rsp {
+       __le32 doe_header;
+       u8 data[];
+} __packed;
+
 /*
  * CXL v3.0 6.2.3 Table 6-4
  * The table indicates that if PCIe Flit Mode is set, then CXL is in 256B flits