[PATCH v4 00/17] Collection of DOE material

[PATCH v4 00/17] Collection of DOE material

[Lukas Wunner]

Collection of DOE material, v4:

Migrate to synchronous API, create mailboxes in PCI core instead of
in drivers, relax restrictions on request/response size.

This should probably go in via the cxl tree because Dave Jiang is
basing his cxl work for the next merge window on it.

The first 6 patches are fixes, so could be applied immediately.

Thanks!


Changes v3 -> v4:

* [PATCH v4 01/17] cxl/pci: Fix CDAT retrieval on big endian
  * In pci_doe_discovery(), add request_pl_le / response_pl_le variables
    to avoid typecasts in pci_doe_task initializer (Jonathan)
  * In cxl_cdat_read_table(), use __le32 instead of u32 for "*data"
    variable (Jonathan)
  * Use sizeof(__le32) instead of sizeof(u32) (Jonathan)

* [PATCH v4 03/17] cxl/pci: Handle truncated CDAT entries
  * Check for sizeof(*entry) instead of sizeof(struct cdat_entry_header)
    for clarity (Jonathan)

* [PATCH v4 12/17] PCI/DOE: Create mailboxes on device enumeration
  * Amend commit message with additional justification for the commit
    (Alexey)

* [PATCH v4 16/17] cxl/pci: cxl/pci: Simplify CDAT retrieval error path
  * Newly added patch in v4 on popular request (Jonathan, Dave)

* [PATCH v4 17/17] cxl/pci: Rightsize CDAT response allocation
  * Amend commit message with spec reference to the Table Access
    Response Header (Ira)
  * In cxl_cdat_get_length(), check for sizeof(response) instead of
    2 * sizeof(u32) for clarity

Link to v3:

https://lore.kernel.org/linux-pci/cover.1676043318.git.lukas@wunner.de/


Dave Jiang (1):
  cxl/pci: Simplify CDAT retrieval error path

Lukas Wunner (16):
  cxl/pci: Fix CDAT retrieval on big endian
  cxl/pci: Handle truncated CDAT header
  cxl/pci: Handle truncated CDAT entries
  cxl/pci: Handle excessive CDAT length
  PCI/DOE: Silence WARN splat with CONFIG_DEBUG_OBJECTS=y
  PCI/DOE: Fix memory leak with CONFIG_DEBUG_OBJECTS=y
  PCI/DOE: Provide synchronous API and use it internally
  cxl/pci: Use synchronous API for DOE
  PCI/DOE: Make asynchronous API private
  PCI/DOE: Deduplicate mailbox flushing
  PCI/DOE: Allow mailbox creation without devres management
  PCI/DOE: Create mailboxes on device enumeration
  cxl/pci: Use CDAT DOE mailbox created by PCI core
  PCI/DOE: Make mailbox creation API private
  PCI/DOE: Relax restrictions on request and response size
  cxl/pci: Rightsize CDAT response allocation

 .clang-format           |   1 -
 drivers/cxl/core/pci.c  | 140 +++++++---------
 drivers/cxl/cxlmem.h    |   3 -
 drivers/cxl/cxlpci.h    |  14 ++
 drivers/cxl/pci.c       |  49 ------
 drivers/pci/doe.c       | 342 ++++++++++++++++++++++++++++++----------
 drivers/pci/pci.h       |  11 ++
 drivers/pci/probe.c     |   1 +
 drivers/pci/remove.c    |   1 +
 include/linux/pci-doe.h |  62 +-------
 include/linux/pci.h     |   3 +
 11 files changed, 350 insertions(+), 277 deletions(-)

-- 
2.39.1

[PATCH v4 01/17] cxl/pci: Fix CDAT retrieval on big endian

[Lukas Wunner]

The CDAT exposed in sysfs differs between little endian and big endian
arches:  On big endian, every 4 bytes are byte-swapped.

PCI Configuration Space is little endian (PCI r3.0 sec 6.1).  Accessors
such as pci_read_config_dword() implicitly swap bytes on big endian.
That way, the macros in include/uapi/linux/pci_regs.h work regardless of
the arch's endianness.  For an example of implicit byte-swapping, see
ppc4xx_pciex_read_config(), which calls in_le32(), which uses lwbrx
(Load Word Byte-Reverse Indexed).

DOE Read/Write Data Mailbox Registers are unlike other registers in
Configuration Space in that they contain or receive a 4 byte portion of
an opaque byte stream (a "Data Object" per PCIe r6.0 sec 7.9.24.5f).
They need to be copied to or from the request/response buffer verbatim.
So amend pci_doe_send_req() and pci_doe_recv_resp() to undo the implicit
byte-swapping.

The CXL_DOE_TABLE_ACCESS_* and PCI_DOE_DATA_OBJECT_DISC_* macros assume
implicit byte-swapping.  Byte-swap requests after constructing them with
those macros and byte-swap responses before parsing them.

Change the request and response type to __le32 to avoid sparse warnings.
Per a request from Jonathan, replace sizeof(u32) with sizeof(__le32) for
consistency.

Fixes: c97006046c79 ("cxl/port: Read CDAT table")
Tested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Cc: stable@vger.kernel.org # v6.0+
---
Changes v3 -> v4:
 * In pci_doe_discovery(), add request_pl_le / response_pl_le variables
   to avoid typecasts in pci_doe_task initializer (Jonathan)
 * In cxl_cdat_read_table(), use __le32 instead of u32 for "*data"
   variable (Jonathan)
 * Use sizeof(__le32) instead of sizeof(u32) (Jonathan)

 drivers/cxl/core/pci.c  | 26 +++++++++++++-------------
 drivers/pci/doe.c       | 25 ++++++++++++++-----------
 include/linux/pci-doe.h |  8 ++++++--
 3 files changed, 33 insertions(+), 26 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 7328a2552411..49a99a84b6aa 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -462,7 +462,7 @@ static struct pci_doe_mb *find_cdat_doe(struct device *uport)
 	return NULL;
 }
 
-#define CDAT_DOE_REQ(entry_handle)					\
+#define CDAT_DOE_REQ(entry_handle) cpu_to_le32				\
 	(FIELD_PREP(CXL_DOE_TABLE_ACCESS_REQ_CODE,			\
 		    CXL_DOE_TABLE_ACCESS_REQ_CODE_READ) |		\
 	 FIELD_PREP(CXL_DOE_TABLE_ACCESS_TABLE_TYPE,			\
@@ -475,8 +475,8 @@ static void cxl_doe_task_complete(struct pci_doe_task *task)
 }
 
 struct cdat_doe_task {
-	u32 request_pl;
-	u32 response_pl[32];
+	__le32 request_pl;
+	__le32 response_pl[32];
 	struct completion c;
 	struct pci_doe_task task;
 };
@@ -510,10 +510,10 @@ static int cxl_cdat_get_length(struct device *dev,
 		return rc;
 	}
 	wait_for_completion(&t.c);
-	if (t.task.rv < sizeof(u32))
+	if (t.task.rv < sizeof(__le32))
 		return -EIO;
 
-	*length = t.response_pl[1];
+	*length = le32_to_cpu(t.response_pl[1]);
 	dev_dbg(dev, "CDAT length %zu\n", *length);
 
 	return 0;
@@ -524,13 +524,13 @@ static int cxl_cdat_read_table(struct device *dev,
 			       struct cxl_cdat *cdat)
 {
 	size_t length = cdat->length;
-	u32 *data = cdat->table;
+	__le32 *data = cdat->table;
 	int entry_handle = 0;
 
 	do {
 		DECLARE_CDAT_DOE_TASK(CDAT_DOE_REQ(entry_handle), t);
 		size_t entry_dw;
-		u32 *entry;
+		__le32 *entry;
 		int rc;
 
 		rc = pci_doe_submit_task(cdat_doe, &t.task);
@@ -540,21 +540,21 @@ static int cxl_cdat_read_table(struct device *dev,
 		}
 		wait_for_completion(&t.c);
 		/* 1 DW header + 1 DW data min */
-		if (t.task.rv < (2 * sizeof(u32)))
+		if (t.task.rv < (2 * sizeof(__le32)))
 			return -EIO;
 
 		/* Get the CXL table access header entry handle */
 		entry_handle = FIELD_GET(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE,
-					 t.response_pl[0]);
+					 le32_to_cpu(t.response_pl[0]));
 		entry = t.response_pl + 1;
-		entry_dw = t.task.rv / sizeof(u32);
+		entry_dw = t.task.rv / sizeof(__le32);
 		/* Skip Header */
 		entry_dw -= 1;
-		entry_dw = min(length / sizeof(u32), entry_dw);
+		entry_dw = min(length / sizeof(__le32), entry_dw);
 		/* Prevent length < 1 DW from causing a buffer overflow */
 		if (entry_dw) {
-			memcpy(data, entry, entry_dw * sizeof(u32));
-			length -= entry_dw * sizeof(u32);
+			memcpy(data, entry, entry_dw * sizeof(__le32));
+			length -= entry_dw * sizeof(__le32);
 			data += entry_dw;
 		}
 	} while (entry_handle != CXL_DOE_TABLE_ACCESS_LAST_ENTRY);
diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
index 66d9ab288646..6f097932ccbf 100644
--- a/drivers/pci/doe.c
+++ b/drivers/pci/doe.c
@@ -128,7 +128,7 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb,
 		return -EIO;
 
 	/* Length is 2 DW of header + length of payload in DW */
-	length = 2 + task->request_pl_sz / sizeof(u32);
+	length = 2 + task->request_pl_sz / sizeof(__le32);
 	if (length > PCI_DOE_MAX_LENGTH)
 		return -EIO;
 	if (length == PCI_DOE_MAX_LENGTH)
@@ -141,9 +141,9 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb,
 	pci_write_config_dword(pdev, offset + PCI_DOE_WRITE,
 			       FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH,
 					  length));
-	for (i = 0; i < task->request_pl_sz / sizeof(u32); i++)
+	for (i = 0; i < task->request_pl_sz / sizeof(__le32); i++)
 		pci_write_config_dword(pdev, offset + PCI_DOE_WRITE,
-				       task->request_pl[i]);
+				       le32_to_cpu(task->request_pl[i]));
 
 	pci_doe_write_ctrl(doe_mb, PCI_DOE_CTRL_GO);
 
@@ -195,11 +195,11 @@ static int pci_doe_recv_resp(struct pci_doe_mb *doe_mb, struct pci_doe_task *tas
 
 	/* First 2 dwords have already been read */
 	length -= 2;
-	payload_length = min(length, task->response_pl_sz / sizeof(u32));
+	payload_length = min(length, task->response_pl_sz / sizeof(__le32));
 	/* Read the rest of the response payload */
 	for (i = 0; i < payload_length; i++) {
-		pci_read_config_dword(pdev, offset + PCI_DOE_READ,
-				      &task->response_pl[i]);
+		pci_read_config_dword(pdev, offset + PCI_DOE_READ, &val);
+		task->response_pl[i] = cpu_to_le32(val);
 		/* Prior to the last ack, ensure Data Object Ready */
 		if (i == (payload_length - 1) && !pci_doe_data_obj_ready(doe_mb))
 			return -EIO;
@@ -217,7 +217,7 @@ static int pci_doe_recv_resp(struct pci_doe_mb *doe_mb, struct pci_doe_task *tas
 	if (FIELD_GET(PCI_DOE_STATUS_ERROR, val))
 		return -EIO;
 
-	return min(length, task->response_pl_sz / sizeof(u32)) * sizeof(u32);
+	return min(length, task->response_pl_sz / sizeof(__le32)) * sizeof(__le32);
 }
 
 static void signal_task_complete(struct pci_doe_task *task, int rv)
@@ -317,14 +317,16 @@ static int pci_doe_discovery(struct pci_doe_mb *doe_mb, u8 *index, u16 *vid,
 {
 	u32 request_pl = FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX,
 				    *index);
+	__le32 request_pl_le = cpu_to_le32(request_pl);
+	__le32 response_pl_le;
 	u32 response_pl;
 	DECLARE_COMPLETION_ONSTACK(c);
 	struct pci_doe_task task = {
 		.prot.vid = PCI_VENDOR_ID_PCI_SIG,
 		.prot.type = PCI_DOE_PROTOCOL_DISCOVERY,
-		.request_pl = &request_pl,
+		.request_pl = &request_pl_le,
 		.request_pl_sz = sizeof(request_pl),
-		.response_pl = &response_pl,
+		.response_pl = &response_pl_le,
 		.response_pl_sz = sizeof(response_pl),
 		.complete = pci_doe_task_complete,
 		.private = &c,
@@ -340,6 +342,7 @@ static int pci_doe_discovery(struct pci_doe_mb *doe_mb, u8 *index, u16 *vid,
 	if (task.rv != sizeof(response_pl))
 		return -EIO;
 
+	response_pl = le32_to_cpu(response_pl_le);
 	*vid = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_VID, response_pl);
 	*protocol = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_PROTOCOL,
 			      response_pl);
@@ -533,8 +536,8 @@ int pci_doe_submit_task(struct pci_doe_mb *doe_mb, struct pci_doe_task *task)
 	 * DOE requests must be a whole number of DW and the response needs to
 	 * be big enough for at least 1 DW
 	 */
-	if (task->request_pl_sz % sizeof(u32) ||
-	    task->response_pl_sz < sizeof(u32))
+	if (task->request_pl_sz % sizeof(__le32) ||
+	    task->response_pl_sz < sizeof(__le32))
 		return -EINVAL;
 
 	if (test_bit(PCI_DOE_FLAG_DEAD, &doe_mb->flags))
diff --git a/include/linux/pci-doe.h b/include/linux/pci-doe.h
index ed9b4df792b8..43765eaf2342 100644
--- a/include/linux/pci-doe.h
+++ b/include/linux/pci-doe.h
@@ -34,6 +34,10 @@ struct pci_doe_mb;
  * @work: Used internally by the mailbox
  * @doe_mb: Used internally by the mailbox
  *
+ * Payloads are treated as opaque byte streams which are transmitted verbatim,
+ * without byte-swapping.  If payloads contain little-endian register values,
+ * the caller is responsible for conversion with cpu_to_le32() / le32_to_cpu().
+ *
  * The payload sizes and rv are specified in bytes with the following
  * restrictions concerning the protocol.
  *
@@ -45,9 +49,9 @@ struct pci_doe_mb;
  */
 struct pci_doe_task {
 	struct pci_doe_protocol prot;
-	u32 *request_pl;
+	__le32 *request_pl;
 	size_t request_pl_sz;
-	u32 *response_pl;
+	__le32 *response_pl;
 	size_t response_pl_sz;
 	int rv;
 	void (*complete)(struct pci_doe_task *task);
-- 
2.39.1

[PATCH v4 02/17] cxl/pci: Handle truncated CDAT header

[Lukas Wunner]

cxl_cdat_get_length() only checks whether the DOE response size is
sufficient for the Table Access response header (1 dword), but not the
succeeding CDAT header (1 dword length plus other fields).

It thus returns whatever uninitialized memory happens to be on the stack
if a truncated DOE response with only 1 dword was received.  Fix it.

Fixes: c97006046c79 ("cxl/port: Read CDAT table")
Reported-by: Ming Li <ming4.li@intel.com>
Tested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Ming Li <ming4.li@intel.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: stable@vger.kernel.org # v6.0+
---
 drivers/cxl/core/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 49a99a84b6aa..87da8c935185 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -510,7 +510,7 @@ static int cxl_cdat_get_length(struct device *dev,
 		return rc;
 	}
 	wait_for_completion(&t.c);
-	if (t.task.rv < sizeof(__le32))
+	if (t.task.rv < 2 * sizeof(__le32))
 		return -EIO;
 
 	*length = le32_to_cpu(t.response_pl[1]);
-- 
2.39.1

[PATCH v4 03/17] cxl/pci: Handle truncated CDAT entries

[Lukas Wunner]

If truncated CDAT entries are received from a device, the concatenation
of those entries constitutes a corrupt CDAT, yet is happily exposed to
user space.

Avoid by verifying response lengths and erroring out if truncation is
detected.

The last CDAT entry may still be truncated despite the checks introduced
herein if the length in the CDAT header is too small.  However, that is
easily detectable by user space because it reaches EOF prematurely.
A subsequent commit which rightsizes the CDAT response allocation closes
that remaining loophole.

The two lines introduced here which exceed 80 chars are shortened to
less than 80 chars by a subsequent commit which migrates to a
synchronous DOE API and replaces "t.task.rv" by "rc".

The existing acpi_cdat_header and acpi_table_cdat struct definitions
provided by ACPICA cannot be used because they do not employ __le16 or
__le32 types.  I believe that cannot be changed because those types are
Linux-specific and ACPI is specified for little endian platforms only,
hence doesn't care about endianness.  So duplicate the structs.

Fixes: c97006046c79 ("cxl/port: Read CDAT table")
Tested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: stable@vger.kernel.org # v6.0+
---
Changes v3 -> v4:
 * Check for sizeof(*entry) instead of sizeof(struct cdat_entry_header)
   for clarity (Jonathan)

 drivers/cxl/core/pci.c | 13 +++++++++----
 drivers/cxl/cxlpci.h   | 14 ++++++++++++++
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 87da8c935185..fb600dfbf5a6 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -529,8 +529,8 @@ static int cxl_cdat_read_table(struct device *dev,
 
 	do {
 		DECLARE_CDAT_DOE_TASK(CDAT_DOE_REQ(entry_handle), t);
+		struct cdat_entry_header *entry;
 		size_t entry_dw;
-		__le32 *entry;
 		int rc;
 
 		rc = pci_doe_submit_task(cdat_doe, &t.task);
@@ -539,14 +539,19 @@ static int cxl_cdat_read_table(struct device *dev,
 			return rc;
 		}
 		wait_for_completion(&t.c);
-		/* 1 DW header + 1 DW data min */
-		if (t.task.rv < (2 * sizeof(__le32)))
+
+		/* 1 DW Table Access Response Header + CDAT entry */
+		entry = (struct cdat_entry_header *)(t.response_pl + 1);
+		if ((entry_handle == 0 &&
+		     t.task.rv != sizeof(__le32) + sizeof(struct cdat_header)) ||
+		    (entry_handle > 0 &&
+		     (t.task.rv < sizeof(__le32) + sizeof(*entry) ||
+		      t.task.rv != sizeof(__le32) + le16_to_cpu(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(t.response_pl[0]));
-		entry = t.response_pl + 1;
 		entry_dw = t.task.rv / sizeof(__le32);
 		/* Skip Header */
 		entry_dw -= 1;
diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
index be6a2ef3cce3..0465ef963cd6 100644
--- a/drivers/cxl/cxlpci.h
+++ b/drivers/cxl/cxlpci.h
@@ -68,6 +68,20 @@ enum cxl_regloc_type {
 	CXL_REGLOC_RBI_TYPES
 };
 
+struct cdat_header {
+	__le32 length;
+	u8 revision;
+	u8 checksum;
+	u8 reserved[6];
+	__le32 sequence;
+} __packed;
+
+struct cdat_entry_header {
+	u8 type;
+	u8 reserved;
+	__le16 length;
+} __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.1

[PATCH v4 04/17] cxl/pci: Handle excessive CDAT length

[Lukas Wunner]

If the length in the CDAT header is larger than the concatenation of the
header and all table entries, then the CDAT exposed to user space
contains trailing null bytes.

Not every consumer may be able to handle that.  Per Postel's robustness
principle, "be liberal in what you accept" and silently reduce the
cached length to avoid exposing those null bytes.

Fixes: c97006046c79 ("cxl/port: Read CDAT table")
Tested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: stable@vger.kernel.org # v6.0+
---
 drivers/cxl/core/pci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index fb600dfbf5a6..523d5b9fd7fc 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -564,6 +564,9 @@ static int cxl_cdat_read_table(struct device *dev,
 		}
 	} while (entry_handle != CXL_DOE_TABLE_ACCESS_LAST_ENTRY);
 
+	/* Length in CDAT header may exceed concatenation of CDAT entries */
+	cdat->length -= length;
+
 	return 0;
 }
 
-- 
2.39.1

[PATCH v4 05/17] PCI/DOE: Silence WARN splat with CONFIG_DEBUG_OBJECTS=y

[Lukas Wunner]

Gregory Price reports a WARN splat with CONFIG_DEBUG_OBJECTS=y upon CXL
probing because pci_doe_submit_task() invokes INIT_WORK() instead of
INIT_WORK_ONSTACK() for a work_struct that was allocated on the stack.

All callers of pci_doe_submit_task() allocate the work_struct on the
stack, so replace INIT_WORK() with INIT_WORK_ONSTACK() as a backportable
short-term fix.

The long-term fix implemented by a subsequent commit is to move to a
synchronous API which allocates the work_struct internally in the DOE
library.

Stacktrace for posterity:

WARNING: CPU: 0 PID: 23 at lib/debugobjects.c:545 __debug_object_init.cold+0x18/0x183
CPU: 0 PID: 23 Comm: kworker/u2:1 Not tainted 6.1.0-0.rc1.20221019gitaae703b02f92.17.fc38.x86_64 #1
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
Call Trace:
 pci_doe_submit_task+0x5d/0xd0
 pci_doe_discovery+0xb4/0x100
 pcim_doe_create_mb+0x219/0x290
 cxl_pci_probe+0x192/0x430
 local_pci_probe+0x41/0x80
 pci_device_probe+0xb3/0x220
 really_probe+0xde/0x380
 __driver_probe_device+0x78/0x170
 driver_probe_device+0x1f/0x90
 __driver_attach_async_helper+0x5c/0xe0
 async_run_entry_fn+0x30/0x130
 process_one_work+0x294/0x5b0

Fixes: 9d24322e887b ("PCI/DOE: Add DOE mailbox support functions")
Link: https://lore.kernel.org/linux-cxl/Y1bOniJliOFszvIK@memverge.com/
Reported-by: Gregory Price <gregory.price@memverge.com>
Tested-by: Ira Weiny <ira.weiny@intel.com>
Tested-by: Gregory Price <gregory.price@memverge.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Gregory Price <gregory.price@memverge.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huwei.com>
Cc: stable@vger.kernel.org # v6.0+
---
 drivers/pci/doe.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
index 6f097932ccbf..c14ffdf23f87 100644
--- a/drivers/pci/doe.c
+++ b/drivers/pci/doe.c
@@ -523,6 +523,8 @@ EXPORT_SYMBOL_GPL(pci_doe_supports_prot);
  * task->complete will be called when the state machine is done processing this
  * task.
  *
+ * @task must be allocated on the stack.
+ *
  * Excess data will be discarded.
  *
  * RETURNS: 0 when task has been successfully queued, -ERRNO on error
@@ -544,7 +546,7 @@ int pci_doe_submit_task(struct pci_doe_mb *doe_mb, struct pci_doe_task *task)
 		return -EIO;
 
 	task->doe_mb = doe_mb;
-	INIT_WORK(&task->work, doe_statemachine_work);
+	INIT_WORK_ONSTACK(&task->work, doe_statemachine_work);
 	queue_work(doe_mb->work_queue, &task->work);
 	return 0;
 }
-- 
2.39.1

[PATCH v4 06/17] PCI/DOE: Fix memory leak with CONFIG_DEBUG_OBJECTS=y

[Lukas Wunner]

After a pci_doe_task completes, its work_struct needs to be destroyed
to avoid a memory leak with CONFIG_DEBUG_OBJECTS=y.

Fixes: 9d24322e887b ("PCI/DOE: Add DOE mailbox support functions")
Tested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: stable@vger.kernel.org # v6.0+
---
 drivers/pci/doe.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
index c14ffdf23f87..e5e9b287b976 100644
--- a/drivers/pci/doe.c
+++ b/drivers/pci/doe.c
@@ -224,6 +224,7 @@ static void signal_task_complete(struct pci_doe_task *task, int rv)
 {
 	task->rv = rv;
 	task->complete(task);
+	destroy_work_on_stack(&task->work);
 }
 
 static void signal_task_abort(struct pci_doe_task *task, int rv)
-- 
2.39.1

[PATCH v4 07/17] PCI/DOE: Provide synchronous API and use it internally

[Lukas Wunner]

The DOE API only allows asynchronous exchanges and forces callers to
provide a completion callback.  Yet all existing callers only perform
synchronous exchanges.  Upcoming commits for CMA (Component Measurement
and Authentication, PCIe r6.0 sec 6.31) likewise require only
synchronous DOE exchanges.

Provide a synchronous pci_doe() API call which builds on the internal
asynchronous machinery.

Convert the internal pci_doe_discovery() to the new call.

The new API allows submission of const-declared requests, necessitating
the addition of a const qualifier in struct pci_doe_task.

Tested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Ming Li <ming4.li@intel.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/pci/doe.c       | 69 ++++++++++++++++++++++++++++++++---------
 include/linux/pci-doe.h |  6 +++-
 2 files changed, 59 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
index e5e9b287b976..adde8c66499c 100644
--- a/drivers/pci/doe.c
+++ b/drivers/pci/doe.c
@@ -321,26 +321,15 @@ static int pci_doe_discovery(struct pci_doe_mb *doe_mb, u8 *index, u16 *vid,
 	__le32 request_pl_le = cpu_to_le32(request_pl);
 	__le32 response_pl_le;
 	u32 response_pl;
-	DECLARE_COMPLETION_ONSTACK(c);
-	struct pci_doe_task task = {
-		.prot.vid = PCI_VENDOR_ID_PCI_SIG,
-		.prot.type = PCI_DOE_PROTOCOL_DISCOVERY,
-		.request_pl = &request_pl_le,
-		.request_pl_sz = sizeof(request_pl),
-		.response_pl = &response_pl_le,
-		.response_pl_sz = sizeof(response_pl),
-		.complete = pci_doe_task_complete,
-		.private = &c,
-	};
 	int rc;
 
-	rc = pci_doe_submit_task(doe_mb, &task);
+	rc = pci_doe(doe_mb, PCI_VENDOR_ID_PCI_SIG, PCI_DOE_PROTOCOL_DISCOVERY,
+		     &request_pl_le, sizeof(request_pl_le),
+		     &response_pl_le, sizeof(response_pl_le));
 	if (rc < 0)
 		return rc;
 
-	wait_for_completion(&c);
-
-	if (task.rv != sizeof(response_pl))
+	if (rc != sizeof(response_pl_le))
 		return -EIO;
 
 	response_pl = le32_to_cpu(response_pl_le);
@@ -552,3 +541,53 @@ int pci_doe_submit_task(struct pci_doe_mb *doe_mb, struct pci_doe_task *task)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(pci_doe_submit_task);
+
+/**
+ * pci_doe() - Perform Data Object Exchange
+ *
+ * @doe_mb: DOE Mailbox
+ * @vendor: Vendor ID
+ * @type: Data Object Type
+ * @request: Request payload
+ * @request_sz: Size of request payload (bytes)
+ * @response: Response payload
+ * @response_sz: Size of response payload (bytes)
+ *
+ * Submit @request to @doe_mb and store the @response.
+ * The DOE exchange is performed synchronously and may therefore sleep.
+ *
+ * Payloads are treated as opaque byte streams which are transmitted verbatim,
+ * without byte-swapping.  If payloads contain little-endian register values,
+ * the caller is responsible for conversion with cpu_to_le32() / le32_to_cpu().
+ *
+ * RETURNS: Length of received response or negative errno.
+ * Received data in excess of @response_sz is discarded.
+ * The length may be smaller than @response_sz and the caller
+ * is responsible for checking that.
+ */
+int pci_doe(struct pci_doe_mb *doe_mb, u16 vendor, u8 type,
+	    const void *request, size_t request_sz,
+	    void *response, size_t response_sz)
+{
+	DECLARE_COMPLETION_ONSTACK(c);
+	struct pci_doe_task task = {
+		.prot.vid = vendor,
+		.prot.type = type,
+		.request_pl = request,
+		.request_pl_sz = request_sz,
+		.response_pl = response,
+		.response_pl_sz = response_sz,
+		.complete = pci_doe_task_complete,
+		.private = &c,
+	};
+	int rc;
+
+	rc = pci_doe_submit_task(doe_mb, &task);
+	if (rc)
+		return rc;
+
+	wait_for_completion(&c);
+
+	return task.rv;
+}
+EXPORT_SYMBOL_GPL(pci_doe);
diff --git a/include/linux/pci-doe.h b/include/linux/pci-doe.h
index 43765eaf2342..5dcd54f892e5 100644
--- a/include/linux/pci-doe.h
+++ b/include/linux/pci-doe.h
@@ -49,7 +49,7 @@ struct pci_doe_mb;
  */
 struct pci_doe_task {
 	struct pci_doe_protocol prot;
-	__le32 *request_pl;
+	const __le32 *request_pl;
 	size_t request_pl_sz;
 	__le32 *response_pl;
 	size_t response_pl_sz;
@@ -78,4 +78,8 @@ struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset);
 bool pci_doe_supports_prot(struct pci_doe_mb *doe_mb, u16 vid, u8 type);
 int pci_doe_submit_task(struct pci_doe_mb *doe_mb, struct pci_doe_task *task);
 
+int pci_doe(struct pci_doe_mb *doe_mb, u16 vendor, u8 type,
+	    const void *request, size_t request_sz,
+	    void *response, size_t response_sz);
+
 #endif
-- 
2.39.1

[PATCH v4 08/17] cxl/pci: Use synchronous API for DOE

[Lukas Wunner]

A synchronous API for DOE has just been introduced.  Convert CXL CDAT
retrieval over to it.

Tested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/pci.c | 66 ++++++++++++++----------------------------
 1 file changed, 22 insertions(+), 44 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 523d5b9fd7fc..8575eaadd522 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -469,51 +469,26 @@ static struct pci_doe_mb *find_cdat_doe(struct device *uport)
 		    CXL_DOE_TABLE_ACCESS_TABLE_TYPE_CDATA) |		\
 	 FIELD_PREP(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE, (entry_handle)))
 
-static void cxl_doe_task_complete(struct pci_doe_task *task)
-{
-	complete(task->private);
-}
-
-struct cdat_doe_task {
-	__le32 request_pl;
-	__le32 response_pl[32];
-	struct completion c;
-	struct pci_doe_task task;
-};
-
-#define DECLARE_CDAT_DOE_TASK(req, cdt)                       \
-struct cdat_doe_task cdt = {                                  \
-	.c = COMPLETION_INITIALIZER_ONSTACK(cdt.c),           \
-	.request_pl = req,				      \
-	.task = {                                             \
-		.prot.vid = PCI_DVSEC_VENDOR_ID_CXL,        \
-		.prot.type = CXL_DOE_PROTOCOL_TABLE_ACCESS, \
-		.request_pl = &cdt.request_pl,                \
-		.request_pl_sz = sizeof(cdt.request_pl),      \
-		.response_pl = cdt.response_pl,               \
-		.response_pl_sz = sizeof(cdt.response_pl),    \
-		.complete = cxl_doe_task_complete,            \
-		.private = &cdt.c,                            \
-	}                                                     \
-}
-
 static int cxl_cdat_get_length(struct device *dev,
 			       struct pci_doe_mb *cdat_doe,
 			       size_t *length)
 {
-	DECLARE_CDAT_DOE_TASK(CDAT_DOE_REQ(0), t);
+	__le32 request = CDAT_DOE_REQ(0);
+	__le32 response[32];
 	int rc;
 
-	rc = pci_doe_submit_task(cdat_doe, &t.task);
+	rc = pci_doe(cdat_doe, PCI_DVSEC_VENDOR_ID_CXL,
+		     CXL_DOE_PROTOCOL_TABLE_ACCESS,
+		     &request, sizeof(request),
+		     &response, sizeof(response));
 	if (rc < 0) {
-		dev_err(dev, "DOE submit failed: %d", rc);
+		dev_err(dev, "DOE failed: %d", rc);
 		return rc;
 	}
-	wait_for_completion(&t.c);
-	if (t.task.rv < 2 * sizeof(__le32))
+	if (rc < 2 * sizeof(__le32))
 		return -EIO;
 
-	*length = le32_to_cpu(t.response_pl[1]);
+	*length = le32_to_cpu(response[1]);
 	dev_dbg(dev, "CDAT length %zu\n", *length);
 
 	return 0;
@@ -528,31 +503,34 @@ static int cxl_cdat_read_table(struct device *dev,
 	int entry_handle = 0;
 
 	do {
-		DECLARE_CDAT_DOE_TASK(CDAT_DOE_REQ(entry_handle), t);
+		__le32 request = CDAT_DOE_REQ(entry_handle);
 		struct cdat_entry_header *entry;
+		__le32 response[32];
 		size_t entry_dw;
 		int rc;
 
-		rc = pci_doe_submit_task(cdat_doe, &t.task);
+		rc = pci_doe(cdat_doe, PCI_DVSEC_VENDOR_ID_CXL,
+			     CXL_DOE_PROTOCOL_TABLE_ACCESS,
+			     &request, sizeof(request),
+			     &response, sizeof(response));
 		if (rc < 0) {
-			dev_err(dev, "DOE submit failed: %d", rc);
+			dev_err(dev, "DOE failed: %d", rc);
 			return rc;
 		}
-		wait_for_completion(&t.c);
 
 		/* 1 DW Table Access Response Header + CDAT entry */
-		entry = (struct cdat_entry_header *)(t.response_pl + 1);
+		entry = (struct cdat_entry_header *)(response + 1);
 		if ((entry_handle == 0 &&
-		     t.task.rv != sizeof(__le32) + sizeof(struct cdat_header)) ||
+		     rc != sizeof(__le32) + sizeof(struct cdat_header)) ||
 		    (entry_handle > 0 &&
-		     (t.task.rv < sizeof(__le32) + sizeof(*entry) ||
-		      t.task.rv != sizeof(__le32) + le16_to_cpu(entry->length))))
+		     (rc < sizeof(__le32) + sizeof(*entry) ||
+		      rc != sizeof(__le32) + le16_to_cpu(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(t.response_pl[0]));
-		entry_dw = t.task.rv / sizeof(__le32);
+					 le32_to_cpu(response[0]));
+		entry_dw = rc / sizeof(__le32);
 		/* Skip Header */
 		entry_dw -= 1;
 		entry_dw = min(length / sizeof(__le32), entry_dw);
-- 
2.39.1

[PATCH v4 09/17] PCI/DOE: Make asynchronous API private

[Lukas Wunner]

A synchronous API for DOE has just been introduced.  CXL (the only
in-tree DOE user so far) was converted to use it instead of the
asynchronous API.

Consequently, pci_doe_submit_task() as well as the pci_doe_task struct
are only used internally, so make them private.

Tested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Ming Li <ming4.li@intel.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/pci/doe.c       | 45 ++++++++++++++++++++++++++++++++++++--
 include/linux/pci-doe.h | 48 -----------------------------------------
 2 files changed, 43 insertions(+), 50 deletions(-)

diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
index adde8c66499c..dfdec73f6abc 100644
--- a/drivers/pci/doe.c
+++ b/drivers/pci/doe.c
@@ -56,6 +56,47 @@ struct pci_doe_mb {
 	unsigned long flags;
 };
 
+struct pci_doe_protocol {
+	u16 vid;
+	u8 type;
+};
+
+/**
+ * struct pci_doe_task - represents a single query/response
+ *
+ * @prot: DOE Protocol
+ * @request_pl: The request payload
+ * @request_pl_sz: Size of the request payload (bytes)
+ * @response_pl: The response payload
+ * @response_pl_sz: Size of the response payload (bytes)
+ * @rv: Return value.  Length of received response or error (bytes)
+ * @complete: Called when task is complete
+ * @private: Private data for the consumer
+ * @work: Used internally by the mailbox
+ * @doe_mb: Used internally by the mailbox
+ *
+ * The payload sizes and rv are specified in bytes with the following
+ * restrictions concerning the protocol.
+ *
+ *	1) The request_pl_sz must be a multiple of double words (4 bytes)
+ *	2) The response_pl_sz must be >= a single double word (4 bytes)
+ *	3) rv is returned as bytes but it will be a multiple of double words
+ */
+struct pci_doe_task {
+	struct pci_doe_protocol prot;
+	const __le32 *request_pl;
+	size_t request_pl_sz;
+	__le32 *response_pl;
+	size_t response_pl_sz;
+	int rv;
+	void (*complete)(struct pci_doe_task *task);
+	void *private;
+
+	/* initialized by pci_doe_submit_task() */
+	struct work_struct work;
+	struct pci_doe_mb *doe_mb;
+};
+
 static int pci_doe_wait(struct pci_doe_mb *doe_mb, unsigned long timeout)
 {
 	if (wait_event_timeout(doe_mb->wq,
@@ -519,7 +560,8 @@ EXPORT_SYMBOL_GPL(pci_doe_supports_prot);
  *
  * RETURNS: 0 when task has been successfully queued, -ERRNO on error
  */
-int pci_doe_submit_task(struct pci_doe_mb *doe_mb, struct pci_doe_task *task)
+static int pci_doe_submit_task(struct pci_doe_mb *doe_mb,
+			       struct pci_doe_task *task)
 {
 	if (!pci_doe_supports_prot(doe_mb, task->prot.vid, task->prot.type))
 		return -EINVAL;
@@ -540,7 +582,6 @@ int pci_doe_submit_task(struct pci_doe_mb *doe_mb, struct pci_doe_task *task)
 	queue_work(doe_mb->work_queue, &task->work);
 	return 0;
 }
-EXPORT_SYMBOL_GPL(pci_doe_submit_task);
 
 /**
  * pci_doe() - Perform Data Object Exchange
diff --git a/include/linux/pci-doe.h b/include/linux/pci-doe.h
index 5dcd54f892e5..7f16749c6aa3 100644
--- a/include/linux/pci-doe.h
+++ b/include/linux/pci-doe.h
@@ -13,55 +13,8 @@
 #ifndef LINUX_PCI_DOE_H
 #define LINUX_PCI_DOE_H
 
-struct pci_doe_protocol {
-	u16 vid;
-	u8 type;
-};
-
 struct pci_doe_mb;
 
-/**
- * struct pci_doe_task - represents a single query/response
- *
- * @prot: DOE Protocol
- * @request_pl: The request payload
- * @request_pl_sz: Size of the request payload (bytes)
- * @response_pl: The response payload
- * @response_pl_sz: Size of the response payload (bytes)
- * @rv: Return value.  Length of received response or error (bytes)
- * @complete: Called when task is complete
- * @private: Private data for the consumer
- * @work: Used internally by the mailbox
- * @doe_mb: Used internally by the mailbox
- *
- * Payloads are treated as opaque byte streams which are transmitted verbatim,
- * without byte-swapping.  If payloads contain little-endian register values,
- * the caller is responsible for conversion with cpu_to_le32() / le32_to_cpu().
- *
- * The payload sizes and rv are specified in bytes with the following
- * restrictions concerning the protocol.
- *
- *	1) The request_pl_sz must be a multiple of double words (4 bytes)
- *	2) The response_pl_sz must be >= a single double word (4 bytes)
- *	3) rv is returned as bytes but it will be a multiple of double words
- *
- * NOTE there is no need for the caller to initialize work or doe_mb.
- */
-struct pci_doe_task {
-	struct pci_doe_protocol prot;
-	const __le32 *request_pl;
-	size_t request_pl_sz;
-	__le32 *response_pl;
-	size_t response_pl_sz;
-	int rv;
-	void (*complete)(struct pci_doe_task *task);
-	void *private;
-
-	/* No need for the user to initialize these fields */
-	struct work_struct work;
-	struct pci_doe_mb *doe_mb;
-};
-
 /**
  * pci_doe_for_each_off - Iterate each DOE capability
  * @pdev: struct pci_dev to iterate
@@ -76,7 +29,6 @@ struct pci_doe_task {
 
 struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset);
 bool pci_doe_supports_prot(struct pci_doe_mb *doe_mb, u16 vid, u8 type);
-int pci_doe_submit_task(struct pci_doe_mb *doe_mb, struct pci_doe_task *task);
 
 int pci_doe(struct pci_doe_mb *doe_mb, u16 vendor, u8 type,
 	    const void *request, size_t request_sz,
-- 
2.39.1

[PATCH v4 10/17] PCI/DOE: Deduplicate mailbox flushing

[Lukas Wunner]

When a DOE mailbox is torn down, its workqueue is flushed once in
pci_doe_flush_mb() through a call to flush_workqueue() and subsequently
flushed once more in pci_doe_destroy_workqueue() through a call to
destroy_workqueue().

Deduplicate by dropping flush_workqueue() from pci_doe_flush_mb().

Rename pci_doe_flush_mb() to pci_doe_cancel_tasks() to more aptly
describe what it now does.

Tested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Ming Li <ming4.li@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/pci/doe.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
index dfdec73f6abc..d9f3676bce29 100644
--- a/drivers/pci/doe.c
+++ b/drivers/pci/doe.c
@@ -429,7 +429,7 @@ static void pci_doe_destroy_workqueue(void *mb)
 	destroy_workqueue(doe_mb->work_queue);
 }
 
-static void pci_doe_flush_mb(void *mb)
+static void pci_doe_cancel_tasks(void *mb)
 {
 	struct pci_doe_mb *doe_mb = mb;
 
@@ -439,9 +439,6 @@ static void pci_doe_flush_mb(void *mb)
 	/* Cancel an in progress work item, if necessary */
 	set_bit(PCI_DOE_FLAG_CANCEL, &doe_mb->flags);
 	wake_up(&doe_mb->wq);
-
-	/* Flush all work items */
-	flush_workqueue(doe_mb->work_queue);
 }
 
 /**
@@ -498,9 +495,9 @@ struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset)
 
 	/*
 	 * The state machine and the mailbox should be in sync now;
-	 * Set up mailbox flush prior to using the mailbox to query protocols.
+	 * Set up cancel tasks prior to using the mailbox to query protocols.
 	 */
-	rc = devm_add_action_or_reset(dev, pci_doe_flush_mb, doe_mb);
+	rc = devm_add_action_or_reset(dev, pci_doe_cancel_tasks, doe_mb);
 	if (rc)
 		return ERR_PTR(rc);
 
-- 
2.39.1

[PATCH v4 11/17] PCI/DOE: Allow mailbox creation without devres management

[Lukas Wunner]

DOE mailbox creation is currently only possible through a devres-managed
API.  The lifetime of mailboxes thus ends with driver unbinding.

An upcoming commit will create DOE mailboxes upon device enumeration by
the PCI core.  Their lifetime shall not be limited by a driver.

Therefore rework pcim_doe_create_mb() into the non-devres-managed
pci_doe_create_mb().  Add pci_doe_destroy_mb() for mailbox destruction
on device removal.

Provide a devres-managed wrapper under the existing pcim_doe_create_mb()
name.

The error path of pcim_doe_create_mb() previously called xa_destroy() if
alloc_ordered_workqueue() failed.  That's unnecessary because the xarray
is still empty at that point.  It doesn't need to be destroyed until
it's been populated by pci_doe_cache_protocols().  Arrange the error
path of the new pci_doe_create_mb() accordingly.

pci_doe_cancel_tasks() is no longer used as callback for
devm_add_action(), so refactor it to accept a struct pci_doe_mb pointer
instead of a generic void pointer.

Tested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Ming Li <ming4.li@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/pci/doe.c | 103 +++++++++++++++++++++++++++++-----------------
 1 file changed, 66 insertions(+), 37 deletions(-)

diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
index d9f3676bce29..7539e72db5cb 100644
--- a/drivers/pci/doe.c
+++ b/drivers/pci/doe.c
@@ -37,7 +37,7 @@
  *
  * This state is used to manage a single DOE mailbox capability.  All fields
  * should be considered opaque to the consumers and the structure passed into
- * the helpers below after being created by devm_pci_doe_create()
+ * the helpers below after being created by pci_doe_create_mb().
  *
  * @pdev: PCI device this mailbox belongs to
  * @cap_offset: Capability offset
@@ -415,24 +415,8 @@ static int pci_doe_cache_protocols(struct pci_doe_mb *doe_mb)
 	return 0;
 }
 
-static void pci_doe_xa_destroy(void *mb)
+static void pci_doe_cancel_tasks(struct pci_doe_mb *doe_mb)
 {
-	struct pci_doe_mb *doe_mb = mb;
-
-	xa_destroy(&doe_mb->prots);
-}
-
-static void pci_doe_destroy_workqueue(void *mb)
-{
-	struct pci_doe_mb *doe_mb = mb;
-
-	destroy_workqueue(doe_mb->work_queue);
-}
-
-static void pci_doe_cancel_tasks(void *mb)
-{
-	struct pci_doe_mb *doe_mb = mb;
-
 	/* Stop all pending work items from starting */
 	set_bit(PCI_DOE_FLAG_DEAD, &doe_mb->flags);
 
@@ -442,7 +426,7 @@ static void pci_doe_cancel_tasks(void *mb)
 }
 
 /**
- * pcim_doe_create_mb() - Create a DOE mailbox object
+ * pci_doe_create_mb() - Create a DOE mailbox object
  *
  * @pdev: PCI device to create the DOE mailbox for
  * @cap_offset: Offset of the DOE mailbox
@@ -453,24 +437,20 @@ static void pci_doe_cancel_tasks(void *mb)
  * RETURNS: created mailbox object on success
  *	    ERR_PTR(-errno) on failure
  */
-struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset)
+static struct pci_doe_mb *pci_doe_create_mb(struct pci_dev *pdev,
+					    u16 cap_offset)
 {
 	struct pci_doe_mb *doe_mb;
-	struct device *dev = &pdev->dev;
 	int rc;
 
-	doe_mb = devm_kzalloc(dev, sizeof(*doe_mb), GFP_KERNEL);
+	doe_mb = kzalloc(sizeof(*doe_mb), GFP_KERNEL);
 	if (!doe_mb)
 		return ERR_PTR(-ENOMEM);
 
 	doe_mb->pdev = pdev;
 	doe_mb->cap_offset = cap_offset;
 	init_waitqueue_head(&doe_mb->wq);
-
 	xa_init(&doe_mb->prots);
-	rc = devm_add_action(dev, pci_doe_xa_destroy, doe_mb);
-	if (rc)
-		return ERR_PTR(rc);
 
 	doe_mb->work_queue = alloc_ordered_workqueue("%s %s DOE [%x]", 0,
 						dev_driver_string(&pdev->dev),
@@ -479,36 +459,85 @@ struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset)
 	if (!doe_mb->work_queue) {
 		pci_err(pdev, "[%x] failed to allocate work queue\n",
 			doe_mb->cap_offset);
-		return ERR_PTR(-ENOMEM);
+		rc = -ENOMEM;
+		goto err_free;
 	}
-	rc = devm_add_action_or_reset(dev, pci_doe_destroy_workqueue, doe_mb);
-	if (rc)
-		return ERR_PTR(rc);
 
 	/* Reset the mailbox by issuing an abort */
 	rc = pci_doe_abort(doe_mb);
 	if (rc) {
 		pci_err(pdev, "[%x] failed to reset mailbox with abort command : %d\n",
 			doe_mb->cap_offset, rc);
-		return ERR_PTR(rc);
+		goto err_destroy_wq;
 	}
 
 	/*
 	 * The state machine and the mailbox should be in sync now;
-	 * Set up cancel tasks prior to using the mailbox to query protocols.
+	 * Use the mailbox to query protocols.
 	 */
-	rc = devm_add_action_or_reset(dev, pci_doe_cancel_tasks, doe_mb);
-	if (rc)
-		return ERR_PTR(rc);
-
 	rc = pci_doe_cache_protocols(doe_mb);
 	if (rc) {
 		pci_err(pdev, "[%x] failed to cache protocols : %d\n",
 			doe_mb->cap_offset, rc);
-		return ERR_PTR(rc);
+		goto err_cancel;
 	}
 
 	return doe_mb;
+
+err_cancel:
+	pci_doe_cancel_tasks(doe_mb);
+	xa_destroy(&doe_mb->prots);
+err_destroy_wq:
+	destroy_workqueue(doe_mb->work_queue);
+err_free:
+	kfree(doe_mb);
+	return ERR_PTR(rc);
+}
+
+/**
+ * pci_doe_destroy_mb() - Destroy a DOE mailbox object
+ *
+ * @ptr: Pointer to DOE mailbox
+ *
+ * Destroy all internal data structures created for the DOE mailbox.
+ */
+static void pci_doe_destroy_mb(void *ptr)
+{
+	struct pci_doe_mb *doe_mb = ptr;
+
+	pci_doe_cancel_tasks(doe_mb);
+	xa_destroy(&doe_mb->prots);
+	destroy_workqueue(doe_mb->work_queue);
+	kfree(doe_mb);
+}
+
+/**
+ * pcim_doe_create_mb() - Create a DOE mailbox object
+ *
+ * @pdev: PCI device to create the DOE mailbox for
+ * @cap_offset: Offset of the DOE mailbox
+ *
+ * Create a single mailbox object to manage the mailbox protocol at the
+ * cap_offset specified.  The mailbox will automatically be destroyed on
+ * driver unbinding from @pdev.
+ *
+ * RETURNS: created mailbox object on success
+ *	    ERR_PTR(-errno) on failure
+ */
+struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset)
+{
+	struct pci_doe_mb *doe_mb;
+	int rc;
+
+	doe_mb = pci_doe_create_mb(pdev, cap_offset);
+	if (IS_ERR(doe_mb))
+		return doe_mb;
+
+	rc = devm_add_action_or_reset(&pdev->dev, pci_doe_destroy_mb, doe_mb);
+	if (rc)
+		return ERR_PTR(rc);
+
+	return doe_mb;
 }
 EXPORT_SYMBOL_GPL(pcim_doe_create_mb);
 
-- 
2.39.1

[PATCH v4 12/17] PCI/DOE: Create mailboxes on device enumeration

[Lukas Wunner]

Currently a DOE instance cannot be shared by multiple drivers because
each driver creates its own pci_doe_mb struct for a given DOE instance.
For the same reason a DOE instance cannot be shared between the PCI core
and a driver.

Moreover, finding out which protocols a DOE instance supports requires
creating a pci_doe_mb for it.  If a device has multiple DOE instances,
a driver looking for a specific protocol may need to create a pci_doe_mb
for each of the device's DOE instances and then destroy those which
do not support the desired protocol.  That's obviously an inefficient
way to do things.

Overcome these issues by creating mailboxes in the PCI core on device
enumeration.

Provide a pci_find_doe_mailbox() API call to allow drivers to get a
pci_doe_mb for a given (pci_dev, vendor, protocol) triple.  This API is
modeled after pci_find_capability() and can later be amended with a
pci_find_next_doe_mailbox() call to iterate over all mailboxes of a
given pci_dev which support a specific protocol.

On removal, destroy the mailboxes in pci_destroy_dev(), after the driver
is unbound.  This allows drivers to use DOE in their ->remove() hook.

On surprise removal, cancel ongoing DOE exchanges and prevent new ones
from being scheduled.  Thereby ensure that a hot-removed device doesn't
needlessly wait for a running exchange to time out.

Tested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Ming Li <ming4.li@intel.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 Changes v3 -> v4:
 * Amend commit message with additional justification for the commit
   (Alexey)

 drivers/pci/doe.c       | 73 +++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h       | 11 +++++++
 drivers/pci/probe.c     |  1 +
 drivers/pci/remove.c    |  1 +
 include/linux/pci-doe.h |  2 ++
 include/linux/pci.h     |  3 ++
 6 files changed, 91 insertions(+)

diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
index 7539e72db5cb..9c577f5b3878 100644
--- a/drivers/pci/doe.c
+++ b/drivers/pci/doe.c
@@ -20,6 +20,8 @@
 #include <linux/pci-doe.h>
 #include <linux/workqueue.h>
 
+#include "pci.h"
+
 #define PCI_DOE_PROTOCOL_DISCOVERY 0
 
 /* Timeout of 1 second from 6.30.2 Operation, PCI Spec r6.0 */
@@ -658,3 +660,74 @@ int pci_doe(struct pci_doe_mb *doe_mb, u16 vendor, u8 type,
 	return task.rv;
 }
 EXPORT_SYMBOL_GPL(pci_doe);
+
+/**
+ * pci_find_doe_mailbox() - Find Data Object Exchange mailbox
+ *
+ * @pdev: PCI device
+ * @vendor: Vendor ID
+ * @type: Data Object Type
+ *
+ * Find first DOE mailbox of a PCI device which supports the given protocol.
+ *
+ * RETURNS: Pointer to the DOE mailbox or NULL if none was found.
+ */
+struct pci_doe_mb *pci_find_doe_mailbox(struct pci_dev *pdev, u16 vendor,
+					u8 type)
+{
+	struct pci_doe_mb *doe_mb;
+	unsigned long index;
+
+	xa_for_each(&pdev->doe_mbs, index, doe_mb)
+		if (pci_doe_supports_prot(doe_mb, vendor, type))
+			return doe_mb;
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(pci_find_doe_mailbox);
+
+void pci_doe_init(struct pci_dev *pdev)
+{
+	struct pci_doe_mb *doe_mb;
+	u16 offset = 0;
+	int rc;
+
+	xa_init(&pdev->doe_mbs);
+
+	while ((offset = pci_find_next_ext_capability(pdev, offset,
+						      PCI_EXT_CAP_ID_DOE))) {
+		doe_mb = pci_doe_create_mb(pdev, offset);
+		if (IS_ERR(doe_mb)) {
+			pci_err(pdev, "[%x] failed to create mailbox: %ld\n",
+				offset, PTR_ERR(doe_mb));
+			continue;
+		}
+
+		rc = xa_insert(&pdev->doe_mbs, offset, doe_mb, GFP_KERNEL);
+		if (rc) {
+			pci_err(pdev, "[%x] failed to insert mailbox: %d\n",
+				offset, rc);
+			pci_doe_destroy_mb(doe_mb);
+		}
+	}
+}
+
+void pci_doe_destroy(struct pci_dev *pdev)
+{
+	struct pci_doe_mb *doe_mb;
+	unsigned long index;
+
+	xa_for_each(&pdev->doe_mbs, index, doe_mb)
+		pci_doe_destroy_mb(doe_mb);
+
+	xa_destroy(&pdev->doe_mbs);
+}
+
+void pci_doe_disconnected(struct pci_dev *pdev)
+{
+	struct pci_doe_mb *doe_mb;
+	unsigned long index;
+
+	xa_for_each(&pdev->doe_mbs, index, doe_mb)
+		pci_doe_cancel_tasks(doe_mb);
+}
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index d2c08670a20e..815a4d2a41da 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -318,6 +318,16 @@ struct pci_sriov {
 	bool		drivers_autoprobe; /* Auto probing of VFs by driver */
 };
 
+#ifdef CONFIG_PCI_DOE
+void pci_doe_init(struct pci_dev *pdev);
+void pci_doe_destroy(struct pci_dev *pdev);
+void pci_doe_disconnected(struct pci_dev *pdev);
+#else
+static inline void pci_doe_init(struct pci_dev *pdev) { }
+static inline void pci_doe_destroy(struct pci_dev *pdev) { }
+static inline void pci_doe_disconnected(struct pci_dev *pdev) { }
+#endif
+
 /**
  * pci_dev_set_io_state - Set the new error state if possible.
  *
@@ -354,6 +364,7 @@ static inline bool pci_dev_set_io_state(struct pci_dev *dev,
 static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
 {
 	pci_dev_set_io_state(dev, pci_channel_io_perm_failure);
+	pci_doe_disconnected(dev);
 
 	return 0;
 }
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index a3f68b6ba6ac..02d2bf80eedb 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2479,6 +2479,7 @@ static void pci_init_capabilities(struct pci_dev *dev)
 	pci_aer_init(dev);		/* Advanced Error Reporting */
 	pci_dpc_init(dev);		/* Downstream Port Containment */
 	pci_rcec_init(dev);		/* Root Complex Event Collector */
+	pci_doe_init(dev);		/* Data Object Exchange */
 
 	pcie_report_downtraining(dev);
 	pci_init_reset_methods(dev);
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 0145aef1b930..f25acf50879f 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -39,6 +39,7 @@ static void pci_destroy_dev(struct pci_dev *dev)
 	list_del(&dev->bus_list);
 	up_write(&pci_bus_sem);
 
+	pci_doe_destroy(dev);
 	pcie_aspm_exit_link_state(dev);
 	pci_bridge_d3_update(dev);
 	pci_free_resources(dev);
diff --git a/include/linux/pci-doe.h b/include/linux/pci-doe.h
index 7f16749c6aa3..d6192ee0ac07 100644
--- a/include/linux/pci-doe.h
+++ b/include/linux/pci-doe.h
@@ -29,6 +29,8 @@ struct pci_doe_mb;
 
 struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset);
 bool pci_doe_supports_prot(struct pci_doe_mb *doe_mb, u16 vid, u8 type);
+struct pci_doe_mb *pci_find_doe_mailbox(struct pci_dev *pdev, u16 vendor,
+					u8 type);
 
 int pci_doe(struct pci_doe_mb *doe_mb, u16 vendor, u8 type,
 	    const void *request, size_t request_sz,
diff --git a/include/linux/pci.h b/include/linux/pci.h
index fafd8020c6d7..ddccff4fe97e 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -511,6 +511,9 @@ struct pci_dev {
 #endif
 #ifdef CONFIG_PCI_P2PDMA
 	struct pci_p2pdma __rcu *p2pdma;
+#endif
+#ifdef CONFIG_PCI_DOE
+	struct xarray	doe_mbs;	/* Data Object Exchange mailboxes */
 #endif
 	u16		acs_cap;	/* ACS Capability offset */
 	phys_addr_t	rom;		/* Physical address if not from BAR */
-- 
2.39.1

[PATCH v4 13/17] cxl/pci: Use CDAT DOE mailbox created by PCI core

[Lukas Wunner]

The PCI core has just been amended to create a pci_doe_mb struct for
every DOE instance on device enumeration.

Drop creation of a (duplicate) CDAT DOE mailbox on cxl probing in favor
of the one already created by the PCI core.

Tested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/cxl/core/pci.c | 27 +++++------------------
 drivers/cxl/cxlmem.h   |  3 ---
 drivers/cxl/pci.c      | 49 ------------------------------------------
 3 files changed, 5 insertions(+), 74 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 8575eaadd522..c868f4a2f1de 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -441,27 +441,6 @@ EXPORT_SYMBOL_NS_GPL(cxl_hdm_decode_init, CXL);
 #define CXL_DOE_TABLE_ACCESS_LAST_ENTRY		0xffff
 #define CXL_DOE_PROTOCOL_TABLE_ACCESS 2
 
-static struct pci_doe_mb *find_cdat_doe(struct device *uport)
-{
-	struct cxl_memdev *cxlmd;
-	struct cxl_dev_state *cxlds;
-	unsigned long index;
-	void *entry;
-
-	cxlmd = to_cxl_memdev(uport);
-	cxlds = cxlmd->cxlds;
-
-	xa_for_each(&cxlds->doe_mbs, index, entry) {
-		struct pci_doe_mb *cur = entry;
-
-		if (pci_doe_supports_prot(cur, PCI_DVSEC_VENDOR_ID_CXL,
-					  CXL_DOE_PROTOCOL_TABLE_ACCESS))
-			return cur;
-	}
-
-	return NULL;
-}
-
 #define CDAT_DOE_REQ(entry_handle) cpu_to_le32				\
 	(FIELD_PREP(CXL_DOE_TABLE_ACCESS_REQ_CODE,			\
 		    CXL_DOE_TABLE_ACCESS_REQ_CODE_READ) |		\
@@ -559,10 +538,14 @@ void read_cdat_data(struct cxl_port *port)
 	struct pci_doe_mb *cdat_doe;
 	struct device *dev = &port->dev;
 	struct device *uport = port->uport;
+	struct cxl_memdev *cxlmd = to_cxl_memdev(uport);
+	struct cxl_dev_state *cxlds = cxlmd->cxlds;
+	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
 	size_t cdat_length;
 	int rc;
 
-	cdat_doe = find_cdat_doe(uport);
+	cdat_doe = pci_find_doe_mailbox(pdev, PCI_DVSEC_VENDOR_ID_CXL,
+					CXL_DOE_PROTOCOL_TABLE_ACCESS);
 	if (!cdat_doe) {
 		dev_dbg(dev, "No CDAT mailbox\n");
 		return;
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 090acebba4fa..001dabf0231b 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -249,7 +249,6 @@ struct cxl_event_state {
  * @component_reg_phys: register base of component registers
  * @info: Cached DVSEC information about the device.
  * @serial: PCIe Device Serial Number
- * @doe_mbs: PCI DOE mailbox array
  * @event: event log driver state
  * @mbox_send: @dev specific transport for transmitting mailbox commands
  *
@@ -287,8 +286,6 @@ struct cxl_dev_state {
 	resource_size_t component_reg_phys;
 	u64 serial;
 
-	struct xarray doe_mbs;
-
 	struct cxl_event_state event;
 
 	int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd);
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 60b23624d167..ea38bd49b0cf 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -8,7 +8,6 @@
 #include <linux/mutex.h>
 #include <linux/list.h>
 #include <linux/pci.h>
-#include <linux/pci-doe.h>
 #include <linux/aer.h>
 #include <linux/io.h>
 #include "cxlmem.h"
@@ -357,52 +356,6 @@ static int cxl_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
 	return rc;
 }
 
-static void cxl_pci_destroy_doe(void *mbs)
-{
-	xa_destroy(mbs);
-}
-
-static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds)
-{
-	struct device *dev = cxlds->dev;
-	struct pci_dev *pdev = to_pci_dev(dev);
-	u16 off = 0;
-
-	xa_init(&cxlds->doe_mbs);
-	if (devm_add_action(&pdev->dev, cxl_pci_destroy_doe, &cxlds->doe_mbs)) {
-		dev_err(dev, "Failed to create XArray for DOE's\n");
-		return;
-	}
-
-	/*
-	 * Mailbox creation is best effort.  Higher layers must determine if
-	 * the lack of a mailbox for their protocol is a device failure or not.
-	 */
-	pci_doe_for_each_off(pdev, off) {
-		struct pci_doe_mb *doe_mb;
-
-		doe_mb = pcim_doe_create_mb(pdev, off);
-		if (IS_ERR(doe_mb)) {
-			dev_err(dev, "Failed to create MB object for MB @ %x\n",
-				off);
-			continue;
-		}
-
-		if (!pci_request_config_region_exclusive(pdev, off,
-							 PCI_DOE_CAP_SIZEOF,
-							 dev_name(dev)))
-			pci_err(pdev, "Failed to exclude DOE registers\n");
-
-		if (xa_insert(&cxlds->doe_mbs, off, doe_mb, GFP_KERNEL)) {
-			dev_err(dev, "xa_insert failed to insert MB @ %x\n",
-				off);
-			continue;
-		}
-
-		dev_dbg(dev, "Created DOE mailbox @%x\n", off);
-	}
-}
-
 /*
  * Assume that any RCIEP that emits the CXL memory expander class code
  * is an RCD
@@ -750,8 +703,6 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	cxlds->component_reg_phys = map.resource;
 
-	devm_cxl_pci_create_doe(cxlds);
-
 	rc = cxl_map_component_regs(&pdev->dev, &cxlds->regs.component,
 				    &map, BIT(CXL_CM_CAP_CAP_ID_RAS));
 	if (rc)
-- 
2.39.1

[PATCH v4 14/17] PCI/DOE: Make mailbox creation API private

[Lukas Wunner]

The PCI core has just been amended to create a pci_doe_mb struct for
every DOE instance on device enumeration.  CXL (the only in-tree DOE
user so far) has been migrated to use those mailboxes instead of
creating its own.

That leaves pcim_doe_create_mb() and pci_doe_for_each_off() without any
callers, so drop them.

pci_doe_supports_prot() is now only used internally, so declare it
static.

pci_doe_destroy_mb() is no longer used as callback for
devm_add_action(), so refactor it to accept a struct pci_doe_mb pointer
instead of a generic void pointer.

Because pci_doe_create_mb() is only called on device enumeration, i.e.
before driver binding, the workqueue name never contains a driver name.
So replace dev_driver_string() with dev_bus_name() when generating the
workqueue name.

Tested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Ming Li <ming4.li@intel.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 .clang-format           |  1 -
 drivers/pci/doe.c       | 41 ++++-------------------------------------
 include/linux/pci-doe.h | 14 --------------
 3 files changed, 4 insertions(+), 52 deletions(-)

diff --git a/.clang-format b/.clang-format
index 2c61b4553374..8e464008b06f 100644
--- a/.clang-format
+++ b/.clang-format
@@ -521,7 +521,6 @@ ForEachMacros:
   - 'of_property_for_each_string'
   - 'of_property_for_each_u32'
   - 'pci_bus_for_each_resource'
-  - 'pci_doe_for_each_off'
   - 'pcl_for_each_chunk'
   - 'pcl_for_each_segment'
   - 'pcm_for_each_format'
diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
index 9c577f5b3878..db9a6b0c33c7 100644
--- a/drivers/pci/doe.c
+++ b/drivers/pci/doe.c
@@ -455,7 +455,7 @@ static struct pci_doe_mb *pci_doe_create_mb(struct pci_dev *pdev,
 	xa_init(&doe_mb->prots);
 
 	doe_mb->work_queue = alloc_ordered_workqueue("%s %s DOE [%x]", 0,
-						dev_driver_string(&pdev->dev),
+						dev_bus_name(&pdev->dev),
 						pci_name(pdev),
 						doe_mb->cap_offset);
 	if (!doe_mb->work_queue) {
@@ -499,50 +499,18 @@ static struct pci_doe_mb *pci_doe_create_mb(struct pci_dev *pdev,
 /**
  * pci_doe_destroy_mb() - Destroy a DOE mailbox object
  *
- * @ptr: Pointer to DOE mailbox
+ * @doe_mb: DOE mailbox
  *
  * Destroy all internal data structures created for the DOE mailbox.
  */
-static void pci_doe_destroy_mb(void *ptr)
+static void pci_doe_destroy_mb(struct pci_doe_mb *doe_mb)
 {
-	struct pci_doe_mb *doe_mb = ptr;
-
 	pci_doe_cancel_tasks(doe_mb);
 	xa_destroy(&doe_mb->prots);
 	destroy_workqueue(doe_mb->work_queue);
 	kfree(doe_mb);
 }
 
-/**
- * pcim_doe_create_mb() - Create a DOE mailbox object
- *
- * @pdev: PCI device to create the DOE mailbox for
- * @cap_offset: Offset of the DOE mailbox
- *
- * Create a single mailbox object to manage the mailbox protocol at the
- * cap_offset specified.  The mailbox will automatically be destroyed on
- * driver unbinding from @pdev.
- *
- * RETURNS: created mailbox object on success
- *	    ERR_PTR(-errno) on failure
- */
-struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset)
-{
-	struct pci_doe_mb *doe_mb;
-	int rc;
-
-	doe_mb = pci_doe_create_mb(pdev, cap_offset);
-	if (IS_ERR(doe_mb))
-		return doe_mb;
-
-	rc = devm_add_action_or_reset(&pdev->dev, pci_doe_destroy_mb, doe_mb);
-	if (rc)
-		return ERR_PTR(rc);
-
-	return doe_mb;
-}
-EXPORT_SYMBOL_GPL(pcim_doe_create_mb);
-
 /**
  * pci_doe_supports_prot() - Return if the DOE instance supports the given
  *			     protocol
@@ -552,7 +520,7 @@ EXPORT_SYMBOL_GPL(pcim_doe_create_mb);
  *
  * RETURNS: True if the DOE mailbox supports the protocol specified
  */
-bool pci_doe_supports_prot(struct pci_doe_mb *doe_mb, u16 vid, u8 type)
+static bool pci_doe_supports_prot(struct pci_doe_mb *doe_mb, u16 vid, u8 type)
 {
 	unsigned long index;
 	void *entry;
@@ -567,7 +535,6 @@ bool pci_doe_supports_prot(struct pci_doe_mb *doe_mb, u16 vid, u8 type)
 
 	return false;
 }
-EXPORT_SYMBOL_GPL(pci_doe_supports_prot);
 
 /**
  * pci_doe_submit_task() - Submit a task to be processed by the state machine
diff --git a/include/linux/pci-doe.h b/include/linux/pci-doe.h
index d6192ee0ac07..1f14aed4354b 100644
--- a/include/linux/pci-doe.h
+++ b/include/linux/pci-doe.h
@@ -15,20 +15,6 @@
 
 struct pci_doe_mb;
 
-/**
- * pci_doe_for_each_off - Iterate each DOE capability
- * @pdev: struct pci_dev to iterate
- * @off: u16 of config space offset of each mailbox capability found
- */
-#define pci_doe_for_each_off(pdev, off) \
-	for (off = pci_find_next_ext_capability(pdev, off, \
-					PCI_EXT_CAP_ID_DOE); \
-		off > 0; \
-		off = pci_find_next_ext_capability(pdev, off, \
-					PCI_EXT_CAP_ID_DOE))
-
-struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset);
-bool pci_doe_supports_prot(struct pci_doe_mb *doe_mb, u16 vid, u8 type);
 struct pci_doe_mb *pci_find_doe_mailbox(struct pci_dev *pdev, u16 vendor,
 					u8 type);
 
-- 
2.39.1

[PATCH v4 15/17] PCI/DOE: Relax restrictions on request and response size

[Lukas Wunner]

An upcoming user of DOE is CMA (Component Measurement and Authentication,
PCIe r6.0 sec 6.31).

It builds on SPDM (Security Protocol and Data Model):
https://www.dmtf.org/dsp/DSP0274

SPDM message sizes are not always a multiple of dwords.  To transport
them over DOE without using bounce buffers, allow sending requests and
receiving responses whose final dword is only partially populated.

To be clear, PCIe r6.0 sec 6.30.1 specifies the Data Object Header 2
"Length" in dwords and pci_doe_send_req() and pci_doe_recv_resp()
read/write dwords.  So from a spec point of view, DOE is still specified
in dwords and allowing non-dword request/response buffers is merely for
the convenience of callers.

Tested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Ming Li <ming4.li@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/pci/doe.c | 74 +++++++++++++++++++++++++++++++----------------
 1 file changed, 49 insertions(+), 25 deletions(-)

diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
index db9a6b0c33c7..1b97a5ab71a9 100644
--- a/drivers/pci/doe.c
+++ b/drivers/pci/doe.c
@@ -76,13 +76,6 @@ struct pci_doe_protocol {
  * @private: Private data for the consumer
  * @work: Used internally by the mailbox
  * @doe_mb: Used internally by the mailbox
- *
- * The payload sizes and rv are specified in bytes with the following
- * restrictions concerning the protocol.
- *
- *	1) The request_pl_sz must be a multiple of double words (4 bytes)
- *	2) The response_pl_sz must be >= a single double word (4 bytes)
- *	3) rv is returned as bytes but it will be a multiple of double words
  */
 struct pci_doe_task {
 	struct pci_doe_protocol prot;
@@ -153,7 +146,7 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb,
 {
 	struct pci_dev *pdev = doe_mb->pdev;
 	int offset = doe_mb->cap_offset;
-	size_t length;
+	size_t length, remainder;
 	u32 val;
 	int i;
 
@@ -171,7 +164,7 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb,
 		return -EIO;
 
 	/* Length is 2 DW of header + length of payload in DW */
-	length = 2 + task->request_pl_sz / sizeof(__le32);
+	length = 2 + DIV_ROUND_UP(task->request_pl_sz, sizeof(__le32));
 	if (length > PCI_DOE_MAX_LENGTH)
 		return -EIO;
 	if (length == PCI_DOE_MAX_LENGTH)
@@ -184,10 +177,21 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb,
 	pci_write_config_dword(pdev, offset + PCI_DOE_WRITE,
 			       FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH,
 					  length));
+
+	/* Write payload */
 	for (i = 0; i < task->request_pl_sz / sizeof(__le32); i++)
 		pci_write_config_dword(pdev, offset + PCI_DOE_WRITE,
 				       le32_to_cpu(task->request_pl[i]));
 
+	/* Write last payload dword */
+	remainder = task->request_pl_sz % sizeof(__le32);
+	if (remainder) {
+		val = 0;
+		memcpy(&val, &task->request_pl[i], remainder);
+		le32_to_cpus(&val);
+		pci_write_config_dword(pdev, offset + PCI_DOE_WRITE, val);
+	}
+
 	pci_doe_write_ctrl(doe_mb, PCI_DOE_CTRL_GO);
 
 	return 0;
@@ -207,11 +211,11 @@ static bool pci_doe_data_obj_ready(struct pci_doe_mb *doe_mb)
 
 static int pci_doe_recv_resp(struct pci_doe_mb *doe_mb, struct pci_doe_task *task)
 {
+	size_t length, payload_length, remainder, received;
 	struct pci_dev *pdev = doe_mb->pdev;
 	int offset = doe_mb->cap_offset;
-	size_t length, payload_length;
+	int i = 0;
 	u32 val;
-	int i;
 
 	/* Read the first dword to get the protocol */
 	pci_read_config_dword(pdev, offset + PCI_DOE_READ, &val);
@@ -238,15 +242,38 @@ static int pci_doe_recv_resp(struct pci_doe_mb *doe_mb, struct pci_doe_task *tas
 
 	/* First 2 dwords have already been read */
 	length -= 2;
-	payload_length = min(length, task->response_pl_sz / sizeof(__le32));
-	/* Read the rest of the response payload */
-	for (i = 0; i < payload_length; i++) {
+	received = task->response_pl_sz;
+	payload_length = DIV_ROUND_UP(task->response_pl_sz, sizeof(__le32));
+	remainder = task->response_pl_sz % sizeof(__le32);
+
+	/* remainder signifies number of data bytes in last payload dword */
+	if (!remainder)
+		remainder = sizeof(__le32);
+
+	if (length < payload_length) {
+		received = length * sizeof(__le32);
+		payload_length = length;
+		remainder = sizeof(__le32);
+	}
+
+	if (payload_length) {
+		/* Read all payload dwords except the last */
+		for (; i < payload_length - 1; i++) {
+			pci_read_config_dword(pdev, offset + PCI_DOE_READ,
+					      &val);
+			task->response_pl[i] = cpu_to_le32(val);
+			pci_write_config_dword(pdev, offset + PCI_DOE_READ, 0);
+		}
+
+		/* Read last payload dword */
 		pci_read_config_dword(pdev, offset + PCI_DOE_READ, &val);
-		task->response_pl[i] = cpu_to_le32(val);
+		cpu_to_le32s(&val);
+		memcpy(&task->response_pl[i], &val, remainder);
 		/* Prior to the last ack, ensure Data Object Ready */
-		if (i == (payload_length - 1) && !pci_doe_data_obj_ready(doe_mb))
+		if (!pci_doe_data_obj_ready(doe_mb))
 			return -EIO;
 		pci_write_config_dword(pdev, offset + PCI_DOE_READ, 0);
+		i++;
 	}
 
 	/* Flush excess length */
@@ -260,7 +287,7 @@ static int pci_doe_recv_resp(struct pci_doe_mb *doe_mb, struct pci_doe_task *tas
 	if (FIELD_GET(PCI_DOE_STATUS_ERROR, val))
 		return -EIO;
 
-	return min(length, task->response_pl_sz / sizeof(__le32)) * sizeof(__le32);
+	return received;
 }
 
 static void signal_task_complete(struct pci_doe_task *task, int rv)
@@ -561,14 +588,6 @@ static int pci_doe_submit_task(struct pci_doe_mb *doe_mb,
 	if (!pci_doe_supports_prot(doe_mb, task->prot.vid, task->prot.type))
 		return -EINVAL;
 
-	/*
-	 * DOE requests must be a whole number of DW and the response needs to
-	 * be big enough for at least 1 DW
-	 */
-	if (task->request_pl_sz % sizeof(__le32) ||
-	    task->response_pl_sz < sizeof(__le32))
-		return -EINVAL;
-
 	if (test_bit(PCI_DOE_FLAG_DEAD, &doe_mb->flags))
 		return -EIO;
 
@@ -596,6 +615,11 @@ static int pci_doe_submit_task(struct pci_doe_mb *doe_mb,
  * without byte-swapping.  If payloads contain little-endian register values,
  * the caller is responsible for conversion with cpu_to_le32() / le32_to_cpu().
  *
+ * For convenience, arbitrary payload sizes are allowed even though PCIe r6.0
+ * sec 6.30.1 specifies the Data Object Header 2 "Length" in dwords.  The last
+ * (partial) dword is copied with byte granularity and padded with zeroes if
+ * necessary.  Callers are thus relieved of using dword-sized bounce buffers.
+ *
  * RETURNS: Length of received response or negative errno.
  * Received data in excess of @response_sz is discarded.
  * The length may be smaller than @response_sz and the caller
-- 
2.39.1

[PATCH v4 16/17] cxl/pci: Simplify CDAT retrieval error path

[Lukas Wunner]

From: Dave Jiang <dave.jiang@intel.com>

The cdat.table and cdat.length fields in struct cxl_port are set before
CDAT retrieval and must therefore be unset on failure.

Simplify by setting only on success.

Suggested-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Link: https://lore.kernel.org/linux-cxl/20230209113422.00007017@Huawei.com/
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
[lukas: rebase and rephrase commit message]
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
---
Changes v3 -> v4:
 * Newly added patch in v4 on popular request (Jonathan, Dave)

 drivers/cxl/core/pci.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index c868f4a2f1de..0609bd629073 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -475,10 +475,10 @@ static int cxl_cdat_get_length(struct device *dev,
 
 static int cxl_cdat_read_table(struct device *dev,
 			       struct pci_doe_mb *cdat_doe,
-			       struct cxl_cdat *cdat)
+			       void *cdat_table, size_t *cdat_length)
 {
-	size_t length = cdat->length;
-	__le32 *data = cdat->table;
+	size_t length = *cdat_length;
+	__le32 *data = cdat_table;
 	int entry_handle = 0;
 
 	do {
@@ -522,7 +522,7 @@ static int cxl_cdat_read_table(struct device *dev,
 	} while (entry_handle != CXL_DOE_TABLE_ACCESS_LAST_ENTRY);
 
 	/* Length in CDAT header may exceed concatenation of CDAT entries */
-	cdat->length -= length;
+	*cdat_length -= length;
 
 	return 0;
 }
@@ -542,6 +542,7 @@ void read_cdat_data(struct cxl_port *port)
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
 	size_t cdat_length;
+	void *cdat_table;
 	int rc;
 
 	cdat_doe = pci_find_doe_mailbox(pdev, PCI_DVSEC_VENDOR_ID_CXL,
@@ -558,19 +559,19 @@ void read_cdat_data(struct cxl_port *port)
 		return;
 	}
 
-	port->cdat.table = devm_kzalloc(dev, cdat_length, GFP_KERNEL);
-	if (!port->cdat.table)
+	cdat_table = devm_kzalloc(dev, cdat_length, GFP_KERNEL);
+	if (!cdat_table)
 		return;
 
-	port->cdat.length = cdat_length;
-	rc = cxl_cdat_read_table(dev, cdat_doe, &port->cdat);
+	rc = cxl_cdat_read_table(dev, cdat_doe, cdat_table, &cdat_length);
 	if (rc) {
 		/* Don't leave table data allocated on error */
-		devm_kfree(dev, port->cdat.table);
-		port->cdat.table = NULL;
-		port->cdat.length = 0;
+		devm_kfree(dev, cdat_table);
 		dev_err(dev, "CDAT data read error\n");
 	}
+
+	port->cdat.table = cdat_table;
+	port->cdat.length = cdat_length;
 }
 EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);
 
-- 
2.39.1

[PATCH v4 17/17] cxl/pci: Rightsize CDAT response allocation

[Lukas Wunner]

Jonathan notes that cxl_cdat_get_length() and cxl_cdat_read_table()
allocate 32 dwords for the DOE response even though it may be smaller.

In the case of cxl_cdat_get_length(), only the second dword of the
response is of interest (it contains the length).  So reduce the
allocation to 2 dwords and let DOE discard the remainder.

In the case of cxl_cdat_read_table(), a correctly sized allocation for
the full CDAT already exists.  Let DOE write each table entry directly
into that allocation.  There's a snag in that the table entry is
preceded by a Table Access Response Header (1 dword, CXL 3.0 table 8-14).
Save the last dword of the previous table entry, let DOE overwrite it
with the header of the next entry and restore it afterwards.

The resulting CDAT is preceded by 4 unavoidable useless bytes.  Increase
the allocation size accordingly.

The buffer overflow check in cxl_cdat_read_table() becomes unnecessary
because the remaining bytes in the allocation are tracked in "length",
which is passed to DOE and limits how many bytes it writes to the
allocation.  Additionally, cxl_cdat_read_table() bails out if the DOE
response is truncated due to insufficient space.

Tested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Dave Jiang <dave.jiang@intel.com>
---
Changes v3 -> v4:
 * Amend commit message with spec reference to the Table Access
   Response Header (Ira)
 * In cxl_cdat_get_length(), check for sizeof(response) instead of
   2 * sizeof(u32) for clarity

 drivers/cxl/core/pci.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 0609bd629073..25b7e8125d5d 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -453,7 +453,7 @@ static int cxl_cdat_get_length(struct device *dev,
 			       size_t *length)
 {
 	__le32 request = CDAT_DOE_REQ(0);
-	__le32 response[32];
+	__le32 response[2];
 	int rc;
 
 	rc = pci_doe(cdat_doe, PCI_DVSEC_VENDOR_ID_CXL,
@@ -464,7 +464,7 @@ static int cxl_cdat_get_length(struct device *dev,
 		dev_err(dev, "DOE failed: %d", rc);
 		return rc;
 	}
-	if (rc < 2 * sizeof(__le32))
+	if (rc < sizeof(response))
 		return -EIO;
 
 	*length = le32_to_cpu(response[1]);
@@ -477,28 +477,28 @@ static int cxl_cdat_read_table(struct device *dev,
 			       struct pci_doe_mb *cdat_doe,
 			       void *cdat_table, size_t *cdat_length)
 {
-	size_t length = *cdat_length;
+	size_t length = *cdat_length + sizeof(__le32);
 	__le32 *data = cdat_table;
 	int entry_handle = 0;
+	__le32 saved_dw = 0;
 
 	do {
 		__le32 request = CDAT_DOE_REQ(entry_handle);
 		struct cdat_entry_header *entry;
-		__le32 response[32];
 		size_t entry_dw;
 		int rc;
 
 		rc = pci_doe(cdat_doe, PCI_DVSEC_VENDOR_ID_CXL,
 			     CXL_DOE_PROTOCOL_TABLE_ACCESS,
 			     &request, sizeof(request),
-			     &response, sizeof(response));
+			     data, length);
 		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 *)(response + 1);
+		entry = (struct cdat_entry_header *)(data + 1);
 		if ((entry_handle == 0 &&
 		     rc != sizeof(__le32) + sizeof(struct cdat_header)) ||
 		    (entry_handle > 0 &&
@@ -508,21 +508,22 @@ static int cxl_cdat_read_table(struct device *dev,
 
 		/* Get the CXL table access header entry handle */
 		entry_handle = FIELD_GET(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE,
-					 le32_to_cpu(response[0]));
+					 le32_to_cpu(data[0]));
 		entry_dw = rc / sizeof(__le32);
 		/* Skip Header */
 		entry_dw -= 1;
-		entry_dw = min(length / sizeof(__le32), entry_dw);
-		/* Prevent length < 1 DW from causing a buffer overflow */
-		if (entry_dw) {
-			memcpy(data, entry, entry_dw * sizeof(__le32));
-			length -= entry_dw * sizeof(__le32);
-			data += entry_dw;
-		}
+		/*
+		 * 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;
 	} while (entry_handle != CXL_DOE_TABLE_ACCESS_LAST_ENTRY);
 
 	/* Length in CDAT header may exceed concatenation of CDAT entries */
-	*cdat_length -= length;
+	*cdat_length -= length - sizeof(__le32);
 
 	return 0;
 }
@@ -559,7 +560,8 @@ void read_cdat_data(struct cxl_port *port)
 		return;
 	}
 
-	cdat_table = devm_kzalloc(dev, cdat_length, GFP_KERNEL);
+	cdat_table = devm_kzalloc(dev, cdat_length + sizeof(__le32),
+				  GFP_KERNEL);
 	if (!cdat_table)
 		return;
 
@@ -570,7 +572,7 @@ void read_cdat_data(struct cxl_port *port)
 		dev_err(dev, "CDAT data read error\n");
 	}
 
-	port->cdat.table = cdat_table;
+	port->cdat.table = cdat_table + sizeof(__le32);
 	port->cdat.length = cdat_length;
 }
 EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);
-- 
2.39.1

Re: [PATCH v4 02/17] cxl/pci: Handle truncated CDAT header

[Sathyanarayanan Kuppuswamy]

On 3/11/23 6:40 AM, Lukas Wunner wrote:
> cxl_cdat_get_length() only checks whether the DOE response size is
> sufficient for the Table Access response header (1 dword), but not the
> succeeding CDAT header (1 dword length plus other fields).
> 
> It thus returns whatever uninitialized memory happens to be on the stack
> if a truncated DOE response with only 1 dword was received.  Fix it.
> 
> Fixes: c97006046c79 ("cxl/port: Read CDAT table")
> Reported-by: Ming Li <ming4.li@intel.com>
> Tested-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Reviewed-by: Ming Li <ming4.li@intel.com>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: stable@vger.kernel.org # v6.0+
> ---

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

>  drivers/cxl/core/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 49a99a84b6aa..87da8c935185 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -510,7 +510,7 @@ static int cxl_cdat_get_length(struct device *dev,
>  		return rc;
>  	}
>  	wait_for_completion(&t.c);
> -	if (t.task.rv < sizeof(__le32))
> +	if (t.task.rv < 2 * sizeof(__le32))
>  		return -EIO;

I think adding a comment about the size requirement would be helpful. But
it is up to you.

>  
>  	*length = le32_to_cpu(t.response_pl[1]);

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v4 00/17] Collection of DOE material

[Bjorn Helgaas]

On Sat, Mar 11, 2023 at 03:40:00PM +0100, Lukas Wunner wrote:
> Collection of DOE material, v4:
> 
> Migrate to synchronous API, create mailboxes in PCI core instead of
> in drivers, relax restrictions on request/response size.
> 
> This should probably go in via the cxl tree because Dave Jiang is
> basing his cxl work for the next merge window on it.
> 
> The first 6 patches are fixes, so could be applied immediately.
> 
> Thanks!
> 
> 
> Changes v3 -> v4:
> 
> * [PATCH v4 01/17] cxl/pci: Fix CDAT retrieval on big endian
>   * In pci_doe_discovery(), add request_pl_le / response_pl_le variables
>     to avoid typecasts in pci_doe_task initializer (Jonathan)
>   * In cxl_cdat_read_table(), use __le32 instead of u32 for "*data"
>     variable (Jonathan)
>   * Use sizeof(__le32) instead of sizeof(u32) (Jonathan)
> 
> * [PATCH v4 03/17] cxl/pci: Handle truncated CDAT entries
>   * Check for sizeof(*entry) instead of sizeof(struct cdat_entry_header)
>     for clarity (Jonathan)
> 
> * [PATCH v4 12/17] PCI/DOE: Create mailboxes on device enumeration
>   * Amend commit message with additional justification for the commit
>     (Alexey)
> 
> * [PATCH v4 16/17] cxl/pci: cxl/pci: Simplify CDAT retrieval error path
>   * Newly added patch in v4 on popular request (Jonathan, Dave)
> 
> * [PATCH v4 17/17] cxl/pci: Rightsize CDAT response allocation
>   * Amend commit message with spec reference to the Table Access
>     Response Header (Ira)
>   * In cxl_cdat_get_length(), check for sizeof(response) instead of
>     2 * sizeof(u32) for clarity
> 
> Link to v3:
> 
> https://lore.kernel.org/linux-pci/cover.1676043318.git.lukas@wunner.de/
> 
> 
> Dave Jiang (1):
>   cxl/pci: Simplify CDAT retrieval error path
> 
> Lukas Wunner (16):
>   cxl/pci: Fix CDAT retrieval on big endian
>   cxl/pci: Handle truncated CDAT header
>   cxl/pci: Handle truncated CDAT entries
>   cxl/pci: Handle excessive CDAT length
>   PCI/DOE: Silence WARN splat with CONFIG_DEBUG_OBJECTS=y
>   PCI/DOE: Fix memory leak with CONFIG_DEBUG_OBJECTS=y
>   PCI/DOE: Provide synchronous API and use it internally
>   cxl/pci: Use synchronous API for DOE
>   PCI/DOE: Make asynchronous API private
>   PCI/DOE: Deduplicate mailbox flushing
>   PCI/DOE: Allow mailbox creation without devres management
>   PCI/DOE: Create mailboxes on device enumeration
>   cxl/pci: Use CDAT DOE mailbox created by PCI core
>   PCI/DOE: Make mailbox creation API private
>   PCI/DOE: Relax restrictions on request and response size
>   cxl/pci: Rightsize CDAT response allocation

Acked-by: Bjorn Helgaas <bhelgaas@google.com> for the PCI/DOE patches,
and I assume these will all be merged via the cxl tree.

>  .clang-format           |   1 -
>  drivers/cxl/core/pci.c  | 140 +++++++---------
>  drivers/cxl/cxlmem.h    |   3 -
>  drivers/cxl/cxlpci.h    |  14 ++
>  drivers/cxl/pci.c       |  49 ------
>  drivers/pci/doe.c       | 342 ++++++++++++++++++++++++++++++----------
>  drivers/pci/pci.h       |  11 ++
>  drivers/pci/probe.c     |   1 +
>  drivers/pci/remove.c    |   1 +
>  include/linux/pci-doe.h |  62 +-------
>  include/linux/pci.h     |   3 +
>  11 files changed, 350 insertions(+), 277 deletions(-)
> 
> -- 
> 2.39.1
> 

Re: [PATCH v4 01/17] cxl/pci: Fix CDAT retrieval on big endian

[Jonathan Cameron]

On Sat, 11 Mar 2023 15:40:01 +0100
Lukas Wunner <lukas@wunner.de> wrote:

> The CDAT exposed in sysfs differs between little endian and big endian
> arches:  On big endian, every 4 bytes are byte-swapped.
> 
> PCI Configuration Space is little endian (PCI r3.0 sec 6.1).  Accessors
> such as pci_read_config_dword() implicitly swap bytes on big endian.
> That way, the macros in include/uapi/linux/pci_regs.h work regardless of
> the arch's endianness.  For an example of implicit byte-swapping, see
> ppc4xx_pciex_read_config(), which calls in_le32(), which uses lwbrx
> (Load Word Byte-Reverse Indexed).
> 
> DOE Read/Write Data Mailbox Registers are unlike other registers in
> Configuration Space in that they contain or receive a 4 byte portion of
> an opaque byte stream (a "Data Object" per PCIe r6.0 sec 7.9.24.5f).
> They need to be copied to or from the request/response buffer verbatim.
> So amend pci_doe_send_req() and pci_doe_recv_resp() to undo the implicit
> byte-swapping.
> 
> The CXL_DOE_TABLE_ACCESS_* and PCI_DOE_DATA_OBJECT_DISC_* macros assume
> implicit byte-swapping.  Byte-swap requests after constructing them with
> those macros and byte-swap responses before parsing them.
> 
> Change the request and response type to __le32 to avoid sparse warnings.
> Per a request from Jonathan, replace sizeof(u32) with sizeof(__le32) for
> consistency.
> 
> Fixes: c97006046c79 ("cxl/port: Read CDAT table")
> Tested-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> Cc: stable@vger.kernel.org # v6.0+

LGTM
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH v4 16/17] cxl/pci: Simplify CDAT retrieval error path

[Jonathan Cameron]

On Sat, 11 Mar 2023 15:40:16 +0100
Lukas Wunner <lukas@wunner.de> wrote:

> From: Dave Jiang <dave.jiang@intel.com>
> 
> The cdat.table and cdat.length fields in struct cxl_port are set before
> CDAT retrieval and must therefore be unset on failure.
> 
> Simplify by setting only on success.
> 
> Suggested-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Link: https://lore.kernel.org/linux-cxl/20230209113422.00007017@Huawei.com/
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> [lukas: rebase and rephrase commit message]
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
> Changes v3 -> v4:
>  * Newly added patch in v4 on popular request (Jonathan, Dave)
> 
>  drivers/cxl/core/pci.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index c868f4a2f1de..0609bd629073 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -475,10 +475,10 @@ static int cxl_cdat_get_length(struct device *dev,
>  
>  static int cxl_cdat_read_table(struct device *dev,
>  			       struct pci_doe_mb *cdat_doe,
> -			       struct cxl_cdat *cdat)
> +			       void *cdat_table, size_t *cdat_length)
>  {
> -	size_t length = cdat->length;
> -	__le32 *data = cdat->table;
> +	size_t length = *cdat_length;
> +	__le32 *data = cdat_table;
>  	int entry_handle = 0;
>  
>  	do {
> @@ -522,7 +522,7 @@ static int cxl_cdat_read_table(struct device *dev,
>  	} while (entry_handle != CXL_DOE_TABLE_ACCESS_LAST_ENTRY);
>  
>  	/* Length in CDAT header may exceed concatenation of CDAT entries */
> -	cdat->length -= length;
> +	*cdat_length -= length;
>  
>  	return 0;
>  }
> @@ -542,6 +542,7 @@ void read_cdat_data(struct cxl_port *port)
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
>  	size_t cdat_length;
> +	void *cdat_table;
>  	int rc;
>  
>  	cdat_doe = pci_find_doe_mailbox(pdev, PCI_DVSEC_VENDOR_ID_CXL,
> @@ -558,19 +559,19 @@ void read_cdat_data(struct cxl_port *port)
>  		return;
>  	}
>  
> -	port->cdat.table = devm_kzalloc(dev, cdat_length, GFP_KERNEL);
> -	if (!port->cdat.table)
> +	cdat_table = devm_kzalloc(dev, cdat_length, GFP_KERNEL);
> +	if (!cdat_table)
>  		return;
>  
> -	port->cdat.length = cdat_length;
> -	rc = cxl_cdat_read_table(dev, cdat_doe, &port->cdat);
> +	rc = cxl_cdat_read_table(dev, cdat_doe, cdat_table, &cdat_length);
>  	if (rc) {
>  		/* Don't leave table data allocated on error */
> -		devm_kfree(dev, port->cdat.table);
> -		port->cdat.table = NULL;
> -		port->cdat.length = 0;
> +		devm_kfree(dev, cdat_table);
>  		dev_err(dev, "CDAT data read error\n");
>  	}
> +
> +	port->cdat.table = cdat_table;
> +	port->cdat.length = cdat_length;
>  }
>  EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);
>  

Re: [PATCH v4 05/17] PCI/DOE: Silence WARN splat with CONFIG_DEBUG_OBJECTS=y

[Alexey Kardashevskiy]

On 12/3/23 01:40, Lukas Wunner wrote:
> Gregory Price reports a WARN splat with CONFIG_DEBUG_OBJECTS=y upon CXL
> probing because pci_doe_submit_task() invokes INIT_WORK() instead of
> INIT_WORK_ONSTACK() for a work_struct that was allocated on the stack.
> 
> All callers of pci_doe_submit_task() allocate the work_struct on the
> stack, so replace INIT_WORK() with INIT_WORK_ONSTACK() as a backportable
> short-term fix.
> 
> The long-term fix implemented by a subsequent commit is to move to a
> synchronous API which allocates the work_struct internally in the DOE
> library.
> 
> Stacktrace for posterity:
> 
> WARNING: CPU: 0 PID: 23 at lib/debugobjects.c:545 __debug_object_init.cold+0x18/0x183
> CPU: 0 PID: 23 Comm: kworker/u2:1 Not tainted 6.1.0-0.rc1.20221019gitaae703b02f92.17.fc38.x86_64 #1
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
> Call Trace:
>   pci_doe_submit_task+0x5d/0xd0
>   pci_doe_discovery+0xb4/0x100
>   pcim_doe_create_mb+0x219/0x290
>   cxl_pci_probe+0x192/0x430
>   local_pci_probe+0x41/0x80
>   pci_device_probe+0xb3/0x220
>   really_probe+0xde/0x380
>   __driver_probe_device+0x78/0x170
>   driver_probe_device+0x1f/0x90
>   __driver_attach_async_helper+0x5c/0xe0
>   async_run_entry_fn+0x30/0x130
>   process_one_work+0x294/0x5b0
> 
> Fixes: 9d24322e887b ("PCI/DOE: Add DOE mailbox support functions")
> Link: https://lore.kernel.org/linux-cxl/Y1bOniJliOFszvIK@memverge.com/
> Reported-by: Gregory Price <gregory.price@memverge.com>
> Tested-by: Ira Weiny <ira.weiny@intel.com>
> Tested-by: Gregory Price <gregory.price@memverge.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> Reviewed-by: Gregory Price <gregory.price@memverge.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huwei.com>
                                                   ^^^^^

huwei? :)


-- 
Alexey

Re: [PATCH v4 05/17] PCI/DOE: Silence WARN splat with CONFIG_DEBUG_OBJECTS=y

[Jonathan Cameron]

On Tue, 21 Mar 2023 14:42:01 +1100
Alexey Kardashevskiy <aik@amd.com> wrote:

> On 12/3/23 01:40, Lukas Wunner wrote:
> > Gregory Price reports a WARN splat with CONFIG_DEBUG_OBJECTS=y upon CXL
> > probing because pci_doe_submit_task() invokes INIT_WORK() instead of
> > INIT_WORK_ONSTACK() for a work_struct that was allocated on the stack.
> > 
> > All callers of pci_doe_submit_task() allocate the work_struct on the
> > stack, so replace INIT_WORK() with INIT_WORK_ONSTACK() as a backportable
> > short-term fix.
> > 
> > The long-term fix implemented by a subsequent commit is to move to a
> > synchronous API which allocates the work_struct internally in the DOE
> > library.
> > 
> > Stacktrace for posterity:
> > 
> > WARNING: CPU: 0 PID: 23 at lib/debugobjects.c:545 __debug_object_init.cold+0x18/0x183
> > CPU: 0 PID: 23 Comm: kworker/u2:1 Not tainted 6.1.0-0.rc1.20221019gitaae703b02f92.17.fc38.x86_64 #1
> > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
> > Call Trace:
> >   pci_doe_submit_task+0x5d/0xd0
> >   pci_doe_discovery+0xb4/0x100
> >   pcim_doe_create_mb+0x219/0x290
> >   cxl_pci_probe+0x192/0x430
> >   local_pci_probe+0x41/0x80
> >   pci_device_probe+0xb3/0x220
> >   really_probe+0xde/0x380
> >   __driver_probe_device+0x78/0x170
> >   driver_probe_device+0x1f/0x90
> >   __driver_attach_async_helper+0x5c/0xe0
> >   async_run_entry_fn+0x30/0x130
> >   process_one_work+0x294/0x5b0
> > 
> > Fixes: 9d24322e887b ("PCI/DOE: Add DOE mailbox support functions")
> > Link: https://lore.kernel.org/linux-cxl/Y1bOniJliOFszvIK@memverge.com/
> > Reported-by: Gregory Price <gregory.price@memverge.com>
> > Tested-by: Ira Weiny <ira.weiny@intel.com>
> > Tested-by: Gregory Price <gregory.price@memverge.com>
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > Reviewed-by: Gregory Price <gregory.price@memverge.com>
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huwei.com>  
>                                                    ^^^^^
> 
> huwei? :)
Doh.  I normally type my own name wrong ;)

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks,

Jonathan

> 
> 

Re: [PATCH v4 05/17] PCI/DOE: Silence WARN splat with CONFIG_DEBUG_OBJECTS=y

[Lukas Wunner]

On Tue, Mar 21, 2023 at 02:42:01PM +1100, Alexey Kardashevskiy wrote:
> On 12/3/23 01:40, Lukas Wunner wrote:
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huwei.com>
>                                                   ^^^^^
> 
> huwei? :)

Thanks for spotting this Alexey.

Dan fixed it up when he applied the patch to cxl/fixes yesterday:
https://git.kernel.org/cxl/cxl/c/92dc899c3b49

[PATCH] cxl/port: Fix port to pci device assumptions in read_cdat_data()

[Dan Williams]

Not all CXL ports are associated with PCI devices. Host-bridge and
cxl_test ports are hosted by platform devices. Teach read_cdat_data() to
be careful about non-pci hosted cxl_memdev instances. Otherwise,
cxl_test crashes with this signature:

 RIP: 0010:xas_start+0x6d/0x290
 [..]
 Call Trace:
  <TASK>
  xas_load+0xa/0x50
  xas_find+0x25b/0x2f0
  xa_find+0x118/0x1d0
  pci_find_doe_mailbox+0x51/0xc0
  read_cdat_data+0x45/0x190 [cxl_core]
  cxl_port_probe+0x10a/0x1e0 [cxl_port]
  cxl_bus_probe+0x17/0x50 [cxl_core]

Some other cleanups are included like removing the single-use @uport
variable, and removing the indirection through 'struct cxl_dev_state' to
lookup the device that registered the memdev and may be a pci device.

Fixes: af0a6c3587dc ("cxl/pci: Use CDAT DOE mailbox created by PCI core")
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/pci.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 25b7e8125d5d..bdbd907884ce 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -536,17 +536,18 @@ static int cxl_cdat_read_table(struct device *dev,
  */
 void read_cdat_data(struct cxl_port *port)
 {
-	struct pci_doe_mb *cdat_doe;
+	struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport);
+	struct device *host = cxlmd->dev.parent;
 	struct device *dev = &port->dev;
-	struct device *uport = port->uport;
-	struct cxl_memdev *cxlmd = to_cxl_memdev(uport);
-	struct cxl_dev_state *cxlds = cxlmd->cxlds;
-	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
+	struct pci_doe_mb *cdat_doe;
 	size_t cdat_length;
 	void *cdat_table;
 	int rc;
 
-	cdat_doe = pci_find_doe_mailbox(pdev, PCI_DVSEC_VENDOR_ID_CXL,
+	if (!dev_is_pci(host))
+		return;
+	cdat_doe = pci_find_doe_mailbox(to_pci_dev(host),
+					PCI_DVSEC_VENDOR_ID_CXL,
 					CXL_DOE_PROTOCOL_TABLE_ACCESS);
 	if (!cdat_doe) {
 		dev_dbg(dev, "No CDAT mailbox\n");

Re: [PATCH] cxl/port: Fix port to pci device assumptions in read_cdat_data()

[Lukas Wunner]

On Fri, Apr 21, 2023 at 07:51:47PM -0700, Dan Williams wrote:
> Not all CXL ports are associated with PCI devices. Host-bridge and
> cxl_test ports are hosted by platform devices. Teach read_cdat_data() to
> be careful about non-pci hosted cxl_memdev instances. Otherwise,
> cxl_test crashes with this signature:
> 
>  RIP: 0010:xas_start+0x6d/0x290
>  [..]
>  Call Trace:
>   <TASK>
>   xas_load+0xa/0x50
>   xas_find+0x25b/0x2f0
>   xa_find+0x118/0x1d0
>   pci_find_doe_mailbox+0x51/0xc0
>   read_cdat_data+0x45/0x190 [cxl_core]
>   cxl_port_probe+0x10a/0x1e0 [cxl_port]
>   cxl_bus_probe+0x17/0x50 [cxl_core]
> 
> Some other cleanups are included like removing the single-use @uport
> variable, and removing the indirection through 'struct cxl_dev_state' to
> lookup the device that registered the memdev and may be a pci device.
> 
> Fixes: af0a6c3587dc ("cxl/pci: Use CDAT DOE mailbox created by PCI core")
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Lukas Wunner <lukas@wunner.de>

Take my Reviewed-by with a grain of salt as I'm absolutely not an
expert on the cxl struct hierarchy, nevertheless this looks sane to me.

I note however that before af0a6c3587dc, xa_for_each() was run on an
xarray which was not initialized with xa_init() on non-pci cxl ports.
(xa_init() was run from cxl_pci_probe() -> devm_cxl_pci_create_doe()
but xa_for_each() was run from read_cdat_data() -> find_cdat_doe()
for non-pci cxl ports as well.)

Hence can't this crash prior to af0a6c3587dc as well?  If it can,
the Fixes tag would rather have to point to c97006046c79 ("cxl/port:
Read CDAT table"), though this patch wouldn't apply cleanly to
pre-6.4 kernels.  c97006046c79 went into v6.0 and there's one stable
kernel between it and v6.4 (for which af0a6c3587dc is queued).
So if the missing xa_init() can indeed cause crashes, you may want to
base your fix on v6.3 and fold it in at the front of cxl/next,
then rebase af0a6c3587dc on top of it.  Let me know if you need help
or want me to look into it.

Thanks for fixing this and sorry for not spotting it myself!

Lukas

> ---
>  drivers/cxl/core/pci.c |   13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 25b7e8125d5d..bdbd907884ce 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -536,17 +536,18 @@ static int cxl_cdat_read_table(struct device *dev,
>   */
>  void read_cdat_data(struct cxl_port *port)
>  {
> -	struct pci_doe_mb *cdat_doe;
> +	struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport);
> +	struct device *host = cxlmd->dev.parent;
>  	struct device *dev = &port->dev;
> -	struct device *uport = port->uport;
> -	struct cxl_memdev *cxlmd = to_cxl_memdev(uport);
> -	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> -	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> +	struct pci_doe_mb *cdat_doe;
>  	size_t cdat_length;
>  	void *cdat_table;
>  	int rc;
>  
> -	cdat_doe = pci_find_doe_mailbox(pdev, PCI_DVSEC_VENDOR_ID_CXL,
> +	if (!dev_is_pci(host))
> +		return;
> +	cdat_doe = pci_find_doe_mailbox(to_pci_dev(host),
> +					PCI_DVSEC_VENDOR_ID_CXL,
>  					CXL_DOE_PROTOCOL_TABLE_ACCESS);
>  	if (!cdat_doe) {
>  		dev_dbg(dev, "No CDAT mailbox\n");
> 

Re: [PATCH] cxl/port: Fix port to pci device assumptions in read_cdat_data()

[Lukas Wunner]

On Sat, Apr 22, 2023 at 10:35:02AM +0200, Lukas Wunner wrote:
> I note however that before af0a6c3587dc, xa_for_each() was run on an
> xarray which was not initialized with xa_init() on non-pci cxl ports.
> (xa_init() was run from cxl_pci_probe() -> devm_cxl_pci_create_doe()
> but xa_for_each() was run from read_cdat_data() -> find_cdat_doe()
> for non-pci cxl ports as well.)
> 
> Hence can't this crash prior to af0a6c3587dc as well?

After taking another look with a fresh pair of eyeballs I think
you may see splats with CONFIG_DEBUG_LOCK_ALLOC=y (even prior to
af0a6c3587dc on non-pci cxl devices) due to the missing xa_init().

struct cxl_dev_state is kzalloc'ed and with CONFIG_DEBUG_LOCK_ALLOC=n,
the members of struct xarray all seem to be just zeroed by xa_init().
So no harm no foul.  But that's not the case with CONFIG_DEBUG_LOCK_ALLOC=y
because spinlock_t contains non-zero data.

Thanks,

Lukas

Re: [PATCH] cxl/port: Fix port to pci device assumptions in read_cdat_data()

[Dan Williams]

Lukas Wunner wrote:
> On Sat, Apr 22, 2023 at 10:35:02AM +0200, Lukas Wunner wrote:
> > I note however that before af0a6c3587dc, xa_for_each() was run on an
> > xarray which was not initialized with xa_init() on non-pci cxl ports.
> > (xa_init() was run from cxl_pci_probe() -> devm_cxl_pci_create_doe()
> > but xa_for_each() was run from read_cdat_data() -> find_cdat_doe()
> > for non-pci cxl ports as well.)
> > 
> > Hence can't this crash prior to af0a6c3587dc as well?
> 
> After taking another look with a fresh pair of eyeballs I think
> you may see splats with CONFIG_DEBUG_LOCK_ALLOC=y (even prior to
> af0a6c3587dc on non-pci cxl devices) due to the missing xa_init().

The xa_init() was included before in the removed call to
devm_cxl_pci_create_doe().

---
-static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds)
-{
-       struct device *dev = cxlds->dev;
-       struct pci_dev *pdev = to_pci_dev(dev);
-       u16 off = 0;
-
-       xa_init(&cxlds->doe_mbs);
---

Re: [PATCH] cxl/port: Fix port to pci device assumptions in read_cdat_data()

[Dan Williams]

Lukas Wunner wrote:
> On Fri, Apr 21, 2023 at 07:51:47PM -0700, Dan Williams wrote:
> > Not all CXL ports are associated with PCI devices. Host-bridge and
> > cxl_test ports are hosted by platform devices. Teach read_cdat_data() to
> > be careful about non-pci hosted cxl_memdev instances. Otherwise,
> > cxl_test crashes with this signature:
> > 
> >  RIP: 0010:xas_start+0x6d/0x290
> >  [..]
> >  Call Trace:
> >   <TASK>
> >   xas_load+0xa/0x50
> >   xas_find+0x25b/0x2f0
> >   xa_find+0x118/0x1d0
> >   pci_find_doe_mailbox+0x51/0xc0
> >   read_cdat_data+0x45/0x190 [cxl_core]
> >   cxl_port_probe+0x10a/0x1e0 [cxl_port]
> >   cxl_bus_probe+0x17/0x50 [cxl_core]
> > 
> > Some other cleanups are included like removing the single-use @uport
> > variable, and removing the indirection through 'struct cxl_dev_state' to
> > lookup the device that registered the memdev and may be a pci device.
> > 
> > Fixes: af0a6c3587dc ("cxl/pci: Use CDAT DOE mailbox created by PCI core")
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> Reviewed-by: Lukas Wunner <lukas@wunner.de>
> 
> Take my Reviewed-by with a grain of salt as I'm absolutely not an
> expert on the cxl struct hierarchy, nevertheless this looks sane to me.

Yeah, I think we need a data structure relationship diagram to explain
endpoint devices, memory devices, endpoint ports, and switch ports all
interconnect.

> I note however that before af0a6c3587dc, xa_for_each() was run on an
> xarray which was not initialized with xa_init() on non-pci cxl ports.
> (xa_init() was run from cxl_pci_probe() -> devm_cxl_pci_create_doe()
> but xa_for_each() was run from read_cdat_data() -> find_cdat_doe()
> for non-pci cxl ports as well.)
> 
> Hence can't this crash prior to af0a6c3587dc as well?  If it can,
> the Fixes tag would rather have to point to c97006046c79 ("cxl/port:
> Read CDAT table"), though this patch wouldn't apply cleanly to
> pre-6.4 kernels.  c97006046c79 went into v6.0 and there's one stable
> kernel between it and v6.4 (for which af0a6c3587dc is queued).
> So if the missing xa_init() can indeed cause crashes, you may want to
> base your fix on v6.3 and fold it in at the front of cxl/next,
> then rebase af0a6c3587dc on top of it.  Let me know if you need help
> or want me to look into it.

As I replied on the other note, I think older kernels are ok.

> Thanks for fixing this and sorry for not spotting it myself!

No worries, I had missed that the unit test were not run yet.

Re: [PATCH] cxl/port: Fix port to pci device assumptions in read_cdat_data()

[Lukas Wunner]

On Sat, Apr 22, 2023 at 01:54:00PM -0700, Dan Williams wrote:
> Lukas Wunner wrote:
> > On Sat, Apr 22, 2023 at 10:35:02AM +0200, Lukas Wunner wrote:
> > > I note however that before af0a6c3587dc, xa_for_each() was run on an
> > > xarray which was not initialized with xa_init() on non-pci cxl ports.
> > > (xa_init() was run from cxl_pci_probe() -> devm_cxl_pci_create_doe()
> > > but xa_for_each() was run from read_cdat_data() -> find_cdat_doe()
> > > for non-pci cxl ports as well.)
> > > 
> > > Hence can't this crash prior to af0a6c3587dc as well?
> > 
> > After taking another look with a fresh pair of eyeballs I think
> > you may see splats with CONFIG_DEBUG_LOCK_ALLOC=y (even prior to
> > af0a6c3587dc on non-pci cxl devices) due to the missing xa_init().
> 
> The xa_init() was included before in the removed call to
> devm_cxl_pci_create_doe().

... which was called from cxl_pci_probe(), so only if a pci_dev exists?

I guess I must be missing something...

Thanks,

Lukas

Re: [PATCH] cxl/port: Fix port to pci device assumptions in read_cdat_data()

[Dan Williams]

Lukas Wunner wrote:
> On Sat, Apr 22, 2023 at 01:54:00PM -0700, Dan Williams wrote:
> > Lukas Wunner wrote:
> > > On Sat, Apr 22, 2023 at 10:35:02AM +0200, Lukas Wunner wrote:
> > > > I note however that before af0a6c3587dc, xa_for_each() was run on an
> > > > xarray which was not initialized with xa_init() on non-pci cxl ports.
> > > > (xa_init() was run from cxl_pci_probe() -> devm_cxl_pci_create_doe()
> > > > but xa_for_each() was run from read_cdat_data() -> find_cdat_doe()
> > > > for non-pci cxl ports as well.)
> > > > 
> > > > Hence can't this crash prior to af0a6c3587dc as well?
> > > 
> > > After taking another look with a fresh pair of eyeballs I think
> > > you may see splats with CONFIG_DEBUG_LOCK_ALLOC=y (even prior to
> > > af0a6c3587dc on non-pci cxl devices) due to the missing xa_init().
> > 
> > The xa_init() was included before in the removed call to
> > devm_cxl_pci_create_doe().
> 
> ... which was called from cxl_pci_probe(), so only if a pci_dev exists?
> 
> I guess I must be missing something...

Oh, no you're right in the sense that the code will use an
only-zero-initialized xarray before the move to the PCI core, but a zero
initialized xarray is sufficient. Recall that xarray walks are only RCU
protected, so the xarray spinlock is not used for xa_for_each().

The change that makes this a failure is now the doe_mbs xarray is not
even allocated in the !pci_dev case.

Re: [PATCH] cxl/port: Fix port to pci device assumptions in read_cdat_data()

[Lukas Wunner]

On Sat, Apr 22, 2023 at 04:22:18PM -0700, Dan Williams wrote:
> Lukas Wunner wrote:
> > On Sat, Apr 22, 2023 at 01:54:00PM -0700, Dan Williams wrote:
> > > Lukas Wunner wrote:
> > > > On Sat, Apr 22, 2023 at 10:35:02AM +0200, Lukas Wunner wrote:
> > > > > I note however that before af0a6c3587dc, xa_for_each() was run on an
> > > > > xarray which was not initialized with xa_init() on non-pci cxl ports.
> > > > > (xa_init() was run from cxl_pci_probe() -> devm_cxl_pci_create_doe()
> > > > > but xa_for_each() was run from read_cdat_data() -> find_cdat_doe()
> > > > > for non-pci cxl ports as well.)
> > > > > 
> > > > > Hence can't this crash prior to af0a6c3587dc as well?
> > > > 
> > > > After taking another look with a fresh pair of eyeballs I think
> > > > you may see splats with CONFIG_DEBUG_LOCK_ALLOC=y (even prior to
> > > > af0a6c3587dc on non-pci cxl devices) due to the missing xa_init().
> > > 
> > > The xa_init() was included before in the removed call to
> > > devm_cxl_pci_create_doe().
> > 
> > ... which was called from cxl_pci_probe(), so only if a pci_dev exists?
> > 
> > I guess I must be missing something...
> 
> Oh, no you're right in the sense that the code will use an
> only-zero-initialized xarray before the move to the PCI core, but a zero
> initialized xarray is sufficient. Recall that xarray walks are only RCU
> protected, so the xarray spinlock is not used for xa_for_each().
> 
> The change that makes this a failure is now the doe_mbs xarray is not
> even allocated in the !pci_dev case.

Makes sense, thanks for the explanation.

Re: [PATCH] cxl/port: Fix port to pci device assumptions in read_cdat_data()

[Jonathan Cameron]

On Sat, 22 Apr 2023 13:56:20 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Lukas Wunner wrote:
> > On Fri, Apr 21, 2023 at 07:51:47PM -0700, Dan Williams wrote:  
> > > Not all CXL ports are associated with PCI devices. Host-bridge and
> > > cxl_test ports are hosted by platform devices. Teach read_cdat_data() to
> > > be careful about non-pci hosted cxl_memdev instances. Otherwise,
> > > cxl_test crashes with this signature:
> > > 
> > >  RIP: 0010:xas_start+0x6d/0x290
> > >  [..]
> > >  Call Trace:
> > >   <TASK>
> > >   xas_load+0xa/0x50
> > >   xas_find+0x25b/0x2f0
> > >   xa_find+0x118/0x1d0
> > >   pci_find_doe_mailbox+0x51/0xc0
> > >   read_cdat_data+0x45/0x190 [cxl_core]
> > >   cxl_port_probe+0x10a/0x1e0 [cxl_port]
> > >   cxl_bus_probe+0x17/0x50 [cxl_core]
> > > 
> > > Some other cleanups are included like removing the single-use @uport
> > > variable, and removing the indirection through 'struct cxl_dev_state' to
> > > lookup the device that registered the memdev and may be a pci device.
> > > 
> > > Fixes: af0a6c3587dc ("cxl/pci: Use CDAT DOE mailbox created by PCI core")
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>  
> > 
> > Reviewed-by: Lukas Wunner <lukas@wunner.de>
> > 
> > Take my Reviewed-by with a grain of salt as I'm absolutely not an
> > expert on the cxl struct hierarchy, nevertheless this looks sane to me.  
> 
> Yeah, I think we need a data structure relationship diagram to explain
> endpoint devices, memory devices, endpoint ports, and switch ports all
> interconnect.

Agreed. I keep forgetting how this all works as well and end up scattering
prints everywhere to find out.

A diagram to refer to would be very useful.

Jonathan

> 
> > I note however that before af0a6c3587dc, xa_for_each() was run on an
> > xarray which was not initialized with xa_init() on non-pci cxl ports.
> > (xa_init() was run from cxl_pci_probe() -> devm_cxl_pci_create_doe()
> > but xa_for_each() was run from read_cdat_data() -> find_cdat_doe()
> > for non-pci cxl ports as well.)
> > 
> > Hence can't this crash prior to af0a6c3587dc as well?  If it can,
> > the Fixes tag would rather have to point to c97006046c79 ("cxl/port:
> > Read CDAT table"), though this patch wouldn't apply cleanly to
> > pre-6.4 kernels.  c97006046c79 went into v6.0 and there's one stable
> > kernel between it and v6.4 (for which af0a6c3587dc is queued).
> > So if the missing xa_init() can indeed cause crashes, you may want to
> > base your fix on v6.3 and fold it in at the front of cxl/next,
> > then rebase af0a6c3587dc on top of it.  Let me know if you need help
> > or want me to look into it.  
> 
> As I replied on the other note, I think older kernels are ok.
> 
> > Thanks for fixing this and sorry for not spotting it myself!  
> 
> No worries, I had missed that the unit test were not run yet.

Re: [PATCH] cxl/port: Fix port to pci device assumptions in read_cdat_data()

[Jonathan Cameron]

On Fri, 21 Apr 2023 19:51:47 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Not all CXL ports are associated with PCI devices. Host-bridge and
> cxl_test ports are hosted by platform devices. Teach read_cdat_data() to
> be careful about non-pci hosted cxl_memdev instances. Otherwise,
> cxl_test crashes with this signature:

This is crossing two paths.  One memdevs that aren't hosted by
PCI devices, two the other port cases.  Right now this code assumes
a memdev, so calling out the other case is problematic.

I'd just drop the mention of host bridges or mention that we only
call this for memdev ports today. 

> 
>  RIP: 0010:xas_start+0x6d/0x290
>  [..]
>  Call Trace:
>   <TASK>
>   xas_load+0xa/0x50
>   xas_find+0x25b/0x2f0
>   xa_find+0x118/0x1d0
>   pci_find_doe_mailbox+0x51/0xc0
>   read_cdat_data+0x45/0x190 [cxl_core]
>   cxl_port_probe+0x10a/0x1e0 [cxl_port]
>   cxl_bus_probe+0x17/0x50 [cxl_core]
> 
> Some other cleanups are included like removing the single-use @uport
> variable, and removing the indirection through 'struct cxl_dev_state' to
> lookup the device that registered the memdev and may be a pci device.
> 
> Fixes: af0a6c3587dc ("cxl/pci: Use CDAT DOE mailbox created by PCI core")
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Makes sense. If it's not a PCI device no config space so no DOE instance
(+ the wrong device types etc).

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/core/pci.c |   13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 25b7e8125d5d..bdbd907884ce 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -536,17 +536,18 @@ static int cxl_cdat_read_table(struct device *dev,
>   */
>  void read_cdat_data(struct cxl_port *port)
>  {
> -	struct pci_doe_mb *cdat_doe;
> +	struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport);

Given you talk about host bridges above, this would not be safe

I don't think we call this on host bridges though so perhaps the
patch description needs to not mention them to avoid confusion.

> +	struct device *host = cxlmd->dev.parent;

>  	struct device *dev = &port->dev;
> -	struct device *uport = port->uport;
> -	struct cxl_memdev *cxlmd = to_cxl_memdev(uport);
> -	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> -	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> +	struct pci_doe_mb *cdat_doe;
>  	size_t cdat_length;
>  	void *cdat_table;
>  	int rc;
>  
> -	cdat_doe = pci_find_doe_mailbox(pdev, PCI_DVSEC_VENDOR_ID_CXL,
> +	if (!dev_is_pci(host))
> +		return;
> +	cdat_doe = pci_find_doe_mailbox(to_pci_dev(host),
> +					PCI_DVSEC_VENDOR_ID_CXL,
>  					CXL_DOE_PROTOCOL_TABLE_ACCESS);
>  	if (!cdat_doe) {
>  		dev_dbg(dev, "No CDAT mailbox\n");
> 

Re: [PATCH] cxl/port: Fix port to pci device assumptions in read_cdat_data()

[Dan Williams]

Jonathan Cameron wrote:
> On Fri, 21 Apr 2023 19:51:47 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Not all CXL ports are associated with PCI devices. Host-bridge and
> > cxl_test ports are hosted by platform devices. Teach read_cdat_data() to
> > be careful about non-pci hosted cxl_memdev instances. Otherwise,
> > cxl_test crashes with this signature:
> 
> This is crossing two paths.  One memdevs that aren't hosted by
> PCI devices, two the other port cases.  Right now this code assumes
> a memdev, so calling out the other case is problematic.
> 
> I'd just drop the mention of host bridges or mention that we only
> call this for memdev ports today. 

Yeah, the reason for mentioning host-bridges was more for 
educational purposes of highlighting that not all 'struct cxl_port'
instances are associated with PCI device objects.

I'll drop it so folks do not come away with the implication that CDAT
data is retrieved from host-bridge ports.