Messages in this thread | | | Date | Tue, 28 Nov 2023 12:06:27 -0800 | From | Ira Weiny <> | Subject | Re: [PATCH] cxl/pci: Get rid of pointer arithmetic reading CDAT table |
| |
Robert Richter wrote: > On 17.11.23 21:09:18, Robert Richter wrote: > > I will send an on-top patch for 6.8 that reworks that code area to > > remove the pointer arithmetic. > > Here it is: > > From 13787f72c20b8c54754ae86015d982307eae0397 Mon Sep 17 00:00:00 2001 > From: Robert Richter <rrichter@amd.com> > Subject: [PATCH] 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 (see fix of devm_kfree() in > read_cdat_data()). > > Rework code to avoid calculations with sizeof(__le32). Introduce > struct cdat_doe 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.
Thanks for this.
> > Cc: Lukas Wunner <lukas@wunner.de> > Cc: Dave Jiang <dave.jiang@intel.com> > Cc: Fan Ni <nifan.cxl@gmail.com> > Signed-off-by: Robert Richter <rrichter@amd.com> > --- > drivers/cxl/core/pci.c | 80 ++++++++++++++++++++---------------------- > drivers/cxl/cxlpci.h | 19 ++++++++++ > 2 files changed, 57 insertions(+), 42 deletions(-) > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c > index 5aaa0b36c42a..f900740c6dea 100644 > --- a/drivers/cxl/core/pci.c > +++ b/drivers/cxl/core/pci.c > @@ -517,14 +517,14 @@ EXPORT_SYMBOL_NS_GPL(cxl_hdm_decode_init, CXL); > FIELD_PREP(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE, (entry_handle))) > > static int cxl_cdat_get_length(struct device *dev, > - struct pci_doe_mb *cdat_doe, > + struct pci_doe_mb *doe_mb,
NIT: Why change the variable name here?
> size_t *length) > { > __le32 request = CDAT_DOE_REQ(0); > __le32 response[2]; > int rc; > > - rc = pci_doe(cdat_doe, PCI_DVSEC_VENDOR_ID_CXL, > + rc = pci_doe(doe_mb, PCI_DVSEC_VENDOR_ID_CXL, > CXL_DOE_PROTOCOL_TABLE_ACCESS, > &request, sizeof(request), > &response, sizeof(response)); > @@ -542,56 +542,54 @@ static int cxl_cdat_get_length(struct device *dev, > } > > static int cxl_cdat_read_table(struct device *dev, > - struct pci_doe_mb *cdat_doe, > - void *cdat_table, size_t *cdat_length) > + struct pci_doe_mb *doe_mb, > + struct cdat_doe *doe, size_t *length) > { > - size_t length = *cdat_length + sizeof(__le32); > - __le32 *data = cdat_table; > + size_t received, remaining = *length; > int entry_handle = 0; > __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(cdat_doe, PCI_DVSEC_VENDOR_ID_CXL, > + rc = pci_doe(doe_mb, PCI_DVSEC_VENDOR_ID_CXL, > CXL_DOE_PROTOCOL_TABLE_ACCESS, > &request, sizeof(request), > - data, length); > + doe, sizeof(*doe) + 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 (rc < sizeof(*doe)) > + return -EIO; > + > + received = rc - sizeof(*doe);
This is not ideal. See comments on struct cdat_doe below.
> + > if ((entry_handle == 0 && > - rc != sizeof(__le32) + sizeof(struct cdat_header)) || > + received != sizeof(doe->header[0])) || > (entry_handle > 0 && > - (rc < sizeof(__le32) + sizeof(*entry) || > - rc != sizeof(__le32) + le16_to_cpu(entry->length)))) > + (received < sizeof(doe->entry[0]) || > + received != le16_to_cpu(doe->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(doe->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; > + doe->doe_header = saved_dw; > + remaining -= received; > + doe = (void *)doe + received; > + saved_dw = doe->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; > } > @@ -616,11 +614,11 @@ void read_cdat_data(struct cxl_port *port) > { > struct device *uport = port->uport_dev; > struct device *dev = &port->dev; > - struct pci_doe_mb *cdat_doe; > + 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 *doe;
Since we are trying to make this cleaner I would prefer a struct and variable name of cdat_doe_rsp.
So... struct cdat_doe_rsp *cdat_doe_rsp;
> + size_t length; > int rc; > > if (is_cxl_memdev(uport)) { > @@ -637,40 +635,38 @@ void read_cdat_data(struct cxl_port *port) > if (!pdev) > return; > > - cdat_doe = pci_find_doe_mailbox(pdev, PCI_DVSEC_VENDOR_ID_CXL, > - CXL_DOE_PROTOCOL_TABLE_ACCESS); > - if (!cdat_doe) { > + doe_mb = pci_find_doe_mailbox(pdev, PCI_DVSEC_VENDOR_ID_CXL, > + CXL_DOE_PROTOCOL_TABLE_ACCESS); > + if (!doe_mb) { > dev_dbg(dev, "No CDAT mailbox\n"); > return; > } > > port->cdat_available = true; > > - if (cxl_cdat_get_length(dev, cdat_doe, &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; > + doe = devm_kzalloc(dev, sizeof(*doe) + length, GFP_KERNEL); > + if (!doe) > + goto err; > > - rc = cxl_cdat_read_table(dev, cdat_doe, cdat_buf, &cdat_length); > + rc = cxl_cdat_read_table(dev, doe_mb, doe, &length); > if (rc) > goto err; > > - cdat_table = cdat_buf + sizeof(__le32); > - if (cdat_checksum(cdat_table, cdat_length)) > + if (cdat_checksum(doe->table, length)) > goto err; > > - port->cdat.table = cdat_table; > - port->cdat.length = cdat_length; > - return; > + port->cdat.table = doe->table;
As an aside: the type of port->cdat may need to change at some point too.
> + port->cdat.length = length; > > + return; > err: > /* Don't leave table data allocated on error */ > - devm_kfree(dev, cdat_buf); > + devm_kfree(dev, doe); > 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 0fa4799ea316..d12ed9d8dec1 100644 > --- a/drivers/cxl/cxlpci.h > +++ b/drivers/cxl/cxlpci.h > @@ -85,6 +85,25 @@ struct cdat_entry_header { > __le16 length; > } __packed; > > +/* > + * Response contains the CDAT only response header of the DOE. The > + * response payload is a CDAT structure (either CDAT header or entry), > + * it may also mark the beginning of the CDAT table. > + * > + * Spec refs: > + * > + * CXL 3.1 Table 8-14: Read Entry Response > + * CDAT Specification 1.03: 2 CDAT Data Structures > + */ > +struct cdat_doe { > + __le32 doe_header; > + union { > + u8 table[0];
At a minimum we need to use flexible arrays here. See:
.../Documentation/process/deprecated.rst 'Zero-length and one-element arrays'
> + struct cdat_header header[0]; > + struct cdat_entry_header entry[0];
So this would need to be:
DECLARE_FLEX_ARRAY(u8, table); DECLARE_FLEX_ARRAY(struct cdat_header, header); DECLARE_FLEX_ARRAY(struct cdat_entry_header, entry);
Also I think it would be best to use sizeof_field().
That said I got crossed up when this structure was used to represent not only the individual DOE responses but also the cdat table as a whole.
I think it would be best to call this cdat_doe_rsp and somehow make a distinction in read_cdat_data() when assigning to the entire port cdat table.
Ira
> + }; > +} __packed; > + > int devm_cxl_port_enumerate_dports(struct cxl_port *port); > struct cxl_dev_state; > int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm, > -- > 2.39.2 > >
| |