* [PATCH 0/2] DOE WARN splat be gone @ 2022-11-28 4:15 Lukas Wunner 2022-11-28 4:25 ` [PATCH 1/2] PCI/DOE: Silence WARN splat upon task submission Lukas Wunner 2022-11-28 4:25 ` [PATCH 2/2] PCI/DOE: Provide synchronous API Lukas Wunner 0 siblings, 2 replies; 8+ messages in thread From: Lukas Wunner @ 2022-11-28 4:15 UTC (permalink / raw) To: Bjorn Helgaas, linux-pci Cc: Gregory Price, Ira Weiny, Jonathan Cameron, Dan Williams, Alison Schofield, Vishal Verma, Li, Ming, linux-cxl Here's my proposal to fix the DOE WARN splat reported by Gregory Price: One backportable oneliner to replace INIT_WORK() with INIT_WORK_ONSTACK() and one patch to change the API and thus fix the problem for good. So that's an alternative approach to Ira's. Please review and test. Thanks! Lukas Wunner (2): PCI/DOE: Silence WARN splat upon task submission PCI/DOE: Provide synchronous API drivers/cxl/core/pci.c | 58 +++++++------------- drivers/pci/doe.c | 114 +++++++++++++++++++++++++++++++++------- include/linux/pci-doe.h | 47 ++--------------- 3 files changed, 117 insertions(+), 102 deletions(-) -- 2.36.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] PCI/DOE: Silence WARN splat upon task submission 2022-11-28 4:15 [PATCH 0/2] DOE WARN splat be gone Lukas Wunner @ 2022-11-28 4:25 ` Lukas Wunner 2022-11-30 15:36 ` Jonathan Cameron 2022-11-28 4:25 ` [PATCH 2/2] PCI/DOE: Provide synchronous API Lukas Wunner 1 sibling, 1 reply; 8+ messages in thread From: Lukas Wunner @ 2022-11-28 4:25 UTC (permalink / raw) To: Bjorn Helgaas, linux-pci Cc: Gregory Price, Ira Weiny, Jonathan Cameron, Dan Williams, Alison Schofield, Vishal Verma, Li, Ming, linux-cxl 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. 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> Signed-off-by: Lukas Wunner <lukas@wunner.de> Cc: stable@vger.kernel.org # v6.0+ Cc: Ira Weiny <ira.weiny@intel.com> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com> --- drivers/pci/doe.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c index 66d9ab288646..52541eac17f1 100644 --- a/drivers/pci/doe.c +++ b/drivers/pci/doe.c @@ -541,7 +541,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.36.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] PCI/DOE: Silence WARN splat upon task submission 2022-11-28 4:25 ` [PATCH 1/2] PCI/DOE: Silence WARN splat upon task submission Lukas Wunner @ 2022-11-30 15:36 ` Jonathan Cameron 2022-11-30 18:52 ` Ira Weiny 0 siblings, 1 reply; 8+ messages in thread From: Jonathan Cameron @ 2022-11-30 15:36 UTC (permalink / raw) To: Lukas Wunner Cc: Bjorn Helgaas, linux-pci, Gregory Price, Ira Weiny, Dan Williams, Alison Schofield, Vishal Verma, Li, Ming, linux-cxl On Mon, 28 Nov 2022 05:25:51 +0100 Lukas Wunner <lukas@wunner.de> 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. > > 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> > Signed-off-by: Lukas Wunner <lukas@wunner.de> > Cc: stable@vger.kernel.org # v6.0+ > Cc: Ira Weiny <ira.weiny@intel.com> > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/pci/doe.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c > index 66d9ab288646..52541eac17f1 100644 > --- a/drivers/pci/doe.c > +++ b/drivers/pci/doe.c > @@ -541,7 +541,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); If we go this way, add a comment to say 'why' it is ONSTACK or add to the function description to say it 'must be on stack'. > queue_work(doe_mb->work_queue, &task->work); > return 0; > } ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] PCI/DOE: Silence WARN splat upon task submission 2022-11-30 15:36 ` Jonathan Cameron @ 2022-11-30 18:52 ` Ira Weiny 0 siblings, 0 replies; 8+ messages in thread From: Ira Weiny @ 2022-11-30 18:52 UTC (permalink / raw) To: Jonathan Cameron Cc: Lukas Wunner, Bjorn Helgaas, linux-pci, Gregory Price, Dan Williams, Alison Schofield, Vishal Verma, Li, Ming, linux-cxl On Wed, Nov 30, 2022 at 03:36:58PM +0000, Jonathan Cameron wrote: > On Mon, 28 Nov 2022 05:25:51 +0100 > Lukas Wunner <lukas@wunner.de> 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. > > > > 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> > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > > Cc: stable@vger.kernel.org # v6.0+ > > Cc: Ira Weiny <ira.weiny@intel.com> > > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > --- > > drivers/pci/doe.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c > > index 66d9ab288646..52541eac17f1 100644 > > --- a/drivers/pci/doe.c > > +++ b/drivers/pci/doe.c > > @@ -541,7 +541,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); > > If we go this way, add a comment to say 'why' it is ONSTACK > or add to the function description to say it 'must be on stack'. My apologies, I somehow missed this series with the chaos of my personal life. I do like this as a backportable fix and I'm not opposed to it. I think Lukas will need to determine how much he needs the async support. So I'm going to back off and let him deal with this. Ira > > > queue_work(doe_mb->work_queue, &task->work); > > return 0; > > } > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] PCI/DOE: Provide synchronous API 2022-11-28 4:15 [PATCH 0/2] DOE WARN splat be gone Lukas Wunner 2022-11-28 4:25 ` [PATCH 1/2] PCI/DOE: Silence WARN splat upon task submission Lukas Wunner @ 2022-11-28 4:25 ` Lukas Wunner 2022-11-30 15:33 ` Jonathan Cameron 1 sibling, 1 reply; 8+ messages in thread From: Lukas Wunner @ 2022-11-28 4:25 UTC (permalink / raw) To: Bjorn Helgaas, linux-pci Cc: Gregory Price, Ira Weiny, Jonathan Cameron, Dan Williams, Alison Schofield, Vishal Verma, Li, Ming, linux-cxl The DOE API only allows asynchronous exchanges and forces callers to provide a completion callback. Yet all existing callers only perform synchronous exchanges. Upcoming patches for CMA (Component Measurement and Authentication, PCIe r6.0.1 sec 6.31) likewise require only synchronous DOE exchanges. Asynchronous users are currently not foreseeable. Provide a synchronous pci_doe() API call which builds on the internal asynchronous machinery. Should asynchronous users appear, reintroducing a pci_doe_async() API call will be trivial. Convert all users to the new synchronous API and make the asynchronous pci_doe_submit_task() as well as the pci_doe_task struct private. Signed-off-by: Lukas Wunner <lukas@wunner.de> --- drivers/cxl/core/pci.c | 58 +++++++-------------- drivers/pci/doe.c | 112 ++++++++++++++++++++++++++++++++++------ include/linux/pci-doe.h | 47 ++--------------- 3 files changed, 116 insertions(+), 101 deletions(-) diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c index 9240df53ed87..743105feb646 100644 --- a/drivers/cxl/core/pci.c +++ b/drivers/cxl/core/pci.c @@ -490,51 +490,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 { - u32 request_pl; - u32 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); + u32 request = CDAT_DOE_REQ(0); + u32 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); return rc; } - wait_for_completion(&t.c); - if (t.task.rv < sizeof(u32)) + if (rc < sizeof(u32)) return -EIO; - *length = t.response_pl[1]; + *length = response[1]; dev_dbg(dev, "CDAT length %zu\n", *length); return 0; @@ -549,26 +524,29 @@ static int cxl_cdat_read_table(struct device *dev, int entry_handle = 0; do { - DECLARE_CDAT_DOE_TASK(CDAT_DOE_REQ(entry_handle), t); + u32 request = CDAT_DOE_REQ(entry_handle); + u32 response[32]; size_t entry_dw; u32 *entry; 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); return rc; } - wait_for_completion(&t.c); /* 1 DW header + 1 DW data min */ - if (t.task.rv < (2 * sizeof(u32))) + if (rc < (2 * sizeof(u32))) return -EIO; /* Get the CXL table access header entry handle */ entry_handle = FIELD_GET(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE, - t.response_pl[0]); - entry = t.response_pl + 1; - entry_dw = t.task.rv / sizeof(u32); + response[0]); + entry = response + 1; + entry_dw = rc / sizeof(u32); /* Skip Header */ entry_dw -= 1; entry_dw = min(length / sizeof(u32), entry_dw); diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c index 52541eac17f1..7d1eb5bef4b5 100644 --- a/drivers/pci/doe.c +++ b/drivers/pci/doe.c @@ -56,6 +56,49 @@ 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 + * + * NOTE there is no need for the caller to initialize work or doe_mb. + */ +struct pci_doe_task { + struct pci_doe_protocol prot; + u32 *request_pl; + size_t request_pl_sz; + u32 *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, @@ -318,26 +361,15 @@ 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); 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_sz = sizeof(request_pl), - .response_pl = &response_pl, - .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, sizeof(request_pl), + &response_pl, sizeof(response_pl)); if (rc < 0) return rc; - wait_for_completion(&c); - - if (task.rv != sizeof(response_pl)) + if (rc != sizeof(response_pl)) return -EIO; *vid = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_VID, response_pl); @@ -524,7 +556,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; @@ -545,4 +578,49 @@ 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 + * + * @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. + * + * 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, + 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 ed9b4df792b8..d6a07c8fe0b2 100644 --- a/include/linux/pci-doe.h +++ b/include/linux/pci-doe.h @@ -13,51 +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 - * - * 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; - u32 *request_pl; - size_t request_pl_sz; - u32 *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 @@ -72,6 +29,8 @@ 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, + void *request, size_t request_sz, + void *response, size_t response_sz); #endif -- 2.36.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] PCI/DOE: Provide synchronous API 2022-11-28 4:25 ` [PATCH 2/2] PCI/DOE: Provide synchronous API Lukas Wunner @ 2022-11-30 15:33 ` Jonathan Cameron 2022-11-30 18:50 ` Ira Weiny 2022-12-03 13:51 ` Lukas Wunner 0 siblings, 2 replies; 8+ messages in thread From: Jonathan Cameron @ 2022-11-30 15:33 UTC (permalink / raw) To: Lukas Wunner Cc: Bjorn Helgaas, linux-pci, Gregory Price, Ira Weiny, Dan Williams, Alison Schofield, Vishal Verma, Li, Ming, linux-cxl On Mon, 28 Nov 2022 05:25:52 +0100 Lukas Wunner <lukas@wunner.de> wrote: > The DOE API only allows asynchronous exchanges and forces callers to > provide a completion callback. Yet all existing callers only perform > synchronous exchanges. Upcoming patches for CMA (Component Measurement > and Authentication, PCIe r6.0.1 sec 6.31) likewise require only > synchronous DOE exchanges. Asynchronous users are currently not > foreseeable. > > Provide a synchronous pci_doe() API call which builds on the internal > asynchronous machinery. Should asynchronous users appear, reintroducing > a pci_doe_async() API call will be trivial. > > Convert all users to the new synchronous API and make the asynchronous > pci_doe_submit_task() as well as the pci_doe_task struct private. > > Signed-off-by: Lukas Wunner <lukas@wunner.de> Hi Lukas, Thanks for looking at this. A few trivial comments line. This covers the existing question around async vs sync but doesn't have the potential advantages that Ira's series has in terms of ripping out a bunch of complexity. I'm too tied up in the various implementations to offer a clear view on which way was should go on this - I'll end up spending all day arguing with myself! It's a bit of crystal ball gazing for how useful keeping the async stuff around will be. Might be a case of taking your first patch then sitting on the current implementation for a cycle or two to see if it get users... Or take approach Ira proposed and only put the infrastructure back in when we have a user for async. Jonathan > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c > index 52541eac17f1..7d1eb5bef4b5 100644 > --- a/drivers/pci/doe.c > +++ b/drivers/pci/doe.c ... > +/** > + * 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 > + * > + * NOTE there is no need for the caller to initialize work or doe_mb. Cut and paste from original, but what's the "caller" of a struct? I'd just drop this NOTE as it's better explained below. > + */ > +struct pci_doe_task { > + struct pci_doe_protocol prot; > + u32 *request_pl; > + size_t request_pl_sz; > + u32 *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; > +}; > + ... > /** > * pci_doe_for_each_off - Iterate each DOE capability > * @pdev: struct pci_dev to iterate > @@ -72,6 +29,8 @@ 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, Whilst there is clearly a verb hidden in that doe, the fact that the whole spec section is called the same is confusing. pci_doe_query_response() maybe or pci_doe_do() perhaps? > + void *request, size_t request_sz, > + void *response, size_t response_sz); > > #endif ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] PCI/DOE: Provide synchronous API 2022-11-30 15:33 ` Jonathan Cameron @ 2022-11-30 18:50 ` Ira Weiny 2022-12-03 13:51 ` Lukas Wunner 1 sibling, 0 replies; 8+ messages in thread From: Ira Weiny @ 2022-11-30 18:50 UTC (permalink / raw) To: Jonathan Cameron Cc: Lukas Wunner, Bjorn Helgaas, linux-pci, Gregory Price, Dan Williams, Alison Schofield, Vishal Verma, Li, Ming, linux-cxl On Wed, Nov 30, 2022 at 03:33:30PM +0000, Jonathan Cameron wrote: > On Mon, 28 Nov 2022 05:25:52 +0100 > Lukas Wunner <lukas@wunner.de> wrote: > > > The DOE API only allows asynchronous exchanges and forces callers to > > provide a completion callback. Yet all existing callers only perform > > synchronous exchanges. Upcoming patches for CMA (Component Measurement > > and Authentication, PCIe r6.0.1 sec 6.31) likewise require only > > synchronous DOE exchanges. Asynchronous users are currently not > > foreseeable. > > > > Provide a synchronous pci_doe() API call which builds on the internal > > asynchronous machinery. Should asynchronous users appear, reintroducing > > a pci_doe_async() API call will be trivial. > > > > Convert all users to the new synchronous API and make the asynchronous > > pci_doe_submit_task() as well as the pci_doe_task struct private. > > > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > > Hi Lukas, > > Thanks for looking at this. A few trivial comments line. > > This covers the existing question around async vs sync > but doesn't have the potential advantages that Ira's series > has in terms of ripping out a bunch of complexity. > > I'm too tied up in the various implementations to offer a clear > view on which way was should go on this - I'll end up spending > all day arguing with myself! > > It's a bit of crystal ball gazing for how useful keeping the async stuff I agree that this is much too 'crystal ball gazing' for me as well. See below for more. > around will be. Might be a case of taking your first patch then > sitting on the current implementation for a cycle or two to see > if it get users... Or take approach Ira proposed and only put the > infrastructure back in when we have a user for async. > > Jonathan > > > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c > > index 52541eac17f1..7d1eb5bef4b5 100644 > > --- a/drivers/pci/doe.c > > +++ b/drivers/pci/doe.c > > ... > > > +/** > > + * 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 > > + * > > + * NOTE there is no need for the caller to initialize work or doe_mb. > > Cut and paste from original, but what's the "caller" of a struct? I'd just > drop this NOTE as it's better explained below. > > > + */ > > +struct pci_doe_task { > > + struct pci_doe_protocol prot; > > + u32 *request_pl; > > + size_t request_pl_sz; > > + u32 *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; > > +}; > > + > > ... > > > /** > > * pci_doe_for_each_off - Iterate each DOE capability > > * @pdev: struct pci_dev to iterate > > @@ -72,6 +29,8 @@ 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, > Whilst there is clearly a verb hidden in that doe, the fact that the > whole spec section is called the same is confusing. > > pci_doe_query_response() maybe or pci_doe_do() perhaps? Or just pci_doe_submit()? Lukas and I discussed this off-line. Because he is going to need this stuff going forward. I'm going to back off fixing this and let him handle it. I agree with him that eventually something like a 'flush' operation will be needed but right now that mechanism is broken. I'll let him determine if it should be removed or fixed depending on his future needs. Ira > > > > + void *request, size_t request_sz, > > + void *response, size_t response_sz); > > > > #endif > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] PCI/DOE: Provide synchronous API 2022-11-30 15:33 ` Jonathan Cameron 2022-11-30 18:50 ` Ira Weiny @ 2022-12-03 13:51 ` Lukas Wunner 1 sibling, 0 replies; 8+ messages in thread From: Lukas Wunner @ 2022-12-03 13:51 UTC (permalink / raw) To: Jonathan Cameron Cc: Bjorn Helgaas, linux-pci, Gregory Price, Ira Weiny, Dan Williams, Alison Schofield, Vishal Verma, Li, Ming, linux-cxl On Wed, Nov 30, 2022 at 03:33:30PM +0000, Jonathan Cameron wrote: > On Mon, 28 Nov 2022 05:25:52 +0100 Lukas Wunner <lukas@wunner.de> wrote: > > + * NOTE there is no need for the caller to initialize work or doe_mb. > > Cut and paste from original, but what's the "caller" of a struct? I'd just > drop this NOTE as it's better explained below. Okay, will drop this note when respinning, it just duplicates the code comment inside the pci_doe_task declaration. > > 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, > > Whilst there is clearly a verb hidden in that doe, the fact that the > whole spec section is called the same is confusing. > > pci_doe_query_response() maybe or pci_doe_do() perhaps? Going forward the PCI core will allocate DOE mailboxes on device enumeration. And the API will then consist of just two calls: One call to get a pointer to a pci_doe_mb for a (pci_dev, vendor, protocol) triple which I intend to call pci_find_doe_mailbox(). And one call to perform a synchronous query/response over a pci_doe_mb. That's the pci_doe() I'm introducing here. I initially went for pci_doe_exchange(), then realized that the "e" in "doe" already stands for "exchange", so that would be redundant. We generally try to use the same terms as the PCIe Base Spec to make it easy for an uninitiated reader to connect the spec to the implementation. The spec talks about "performing data object exchanges", but pci_doe_perform() didn't seem very appealing to me. I ended up just using pci_doe() for simplicity and because it makes it easy to stay within 80 chars if the function name is short. Passing in the request and response pointers directly instead of populating a struct is likewise an attempt to make things as simple as possible for driver authors. It's modeled after the SPI and I2C APIs which offer similar primitives to perform read/write exchanges with devices. (Side note: pci_find_doe_mailbox() will return the *first* mailbox found for a given (pci_dev, vendor, protocol) triple. Conceivably, there may be devices with multiple mailboxes supporting the same protocols. We may have to add a pci_find_next_doe_mailbox(), should such devices appear. Those names match what we already have for capabilities, i.e. pci_find_ext_capability() and pci_find_next_ext_capability().) Thanks, Lukas ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-12-03 13:51 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-11-28 4:15 [PATCH 0/2] DOE WARN splat be gone Lukas Wunner 2022-11-28 4:25 ` [PATCH 1/2] PCI/DOE: Silence WARN splat upon task submission Lukas Wunner 2022-11-30 15:36 ` Jonathan Cameron 2022-11-30 18:52 ` Ira Weiny 2022-11-28 4:25 ` [PATCH 2/2] PCI/DOE: Provide synchronous API Lukas Wunner 2022-11-30 15:33 ` Jonathan Cameron 2022-11-30 18:50 ` Ira Weiny 2022-12-03 13:51 ` Lukas Wunner
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.