[PATCH v2 00/10] Collection of DOE material

[PATCH v2 00/10] Collection of DOE material

[Lukas Wunner]

Collection of DOE material, v2:

* Fix WARN splat reported by Gregory Price
* Migrate to synchronous API
* Create DOE mailboxes in PCI core instead of in drivers
* No longer require request and response size to be multiple of 4 bytes

This is in preparation for CMA device authentication (PCIe r6.0, sec 6.31),
which performs DOE exchanges of irregular size and is going to be handled
in the PCI core.  The synchronous API reduces code size for DOE users.

Link to CMA development branch:
https://github.com/l1k/linux/commits/doe


Changes v1 -> v2:
* [PATCH v2 01/10] PCI/DOE: Silence WARN splat with CONFIG_DEBUG_OBJECTS=y 
  * Add note in kernel-doc of pci_doe_submit_task() that pci_doe_task must
    be allocated on the stack (Jonathan)
* [PATCH v2 05/10] PCI/DOE: Make asynchronous API private
  * Deduplicate note in kernel-doc of struct pci_doe_task that caller need
    not initialize certain fields (Jonathan)

Link to v1:
https://lore.kernel.org/linux-pci/cover.1669608950.git.lukas@wunner.de/


Lukas Wunner (10):
  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: 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

 .clang-format           |   1 -
 drivers/cxl/core/pci.c  |  89 ++++--------
 drivers/cxl/cxlmem.h    |   3 -
 drivers/cxl/pci.c       |  49 -------
 drivers/pci/doe.c       | 315 ++++++++++++++++++++++++++++++----------
 drivers/pci/pci.h       |  10 ++
 drivers/pci/probe.c     |   1 +
 drivers/pci/remove.c    |   2 +
 include/linux/pci-doe.h |  62 +-------
 include/linux/pci.h     |   3 +
 10 files changed, 283 insertions(+), 252 deletions(-)

-- 
2.39.1

[PATCH v2 01/10] 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.

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>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: stable@vger.kernel.org # v6.0+
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 Changes v1 -> v2:
  * Add note in kernel-doc of pci_doe_submit_task() that pci_doe_task must
    be allocated on the stack (Jonathan)

 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 66d9ab288646..12a6752351bf 100644
--- a/drivers/pci/doe.c
+++ b/drivers/pci/doe.c
@@ -520,6 +520,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
@@ -541,7 +543,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

Re: [PATCH v2 01/10] PCI/DOE: Silence WARN splat with CONFIG_DEBUG_OBJECTS=y

[Ira Weiny]

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.
> 
> 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>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: stable@vger.kernel.org # v6.0+
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  Changes v1 -> v2:
>   * Add note in kernel-doc of pci_doe_submit_task() that pci_doe_task must
>     be allocated on the stack (Jonathan)
> 
>  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 66d9ab288646..12a6752351bf 100644
> --- a/drivers/pci/doe.c
> +++ b/drivers/pci/doe.c
> @@ -520,6 +520,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
> @@ -541,7 +543,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
> 

Re: [PATCH v2 01/10] PCI/DOE: Silence WARN splat with CONFIG_DEBUG_OBJECTS=y

[Jonathan Cameron]

On Mon, 23 Jan 2023 16:33:36 -0800
Ira Weiny <ira.weiny@intel.com> wrote:

> 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.
> > 
> > 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>  
> 
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>

It's an unusual requirement, but this is indeed the minimal fix
given current users.  Obviously becomes more sensible later in the
series once you make the API synchronous only.

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

> 
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > Cc: stable@vger.kernel.org # v6.0+
> > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> >  Changes v1 -> v2:
> >   * Add note in kernel-doc of pci_doe_submit_task() that pci_doe_task must
> >     be allocated on the stack (Jonathan)
> > 
> >  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 66d9ab288646..12a6752351bf 100644
> > --- a/drivers/pci/doe.c
> > +++ b/drivers/pci/doe.c
> > @@ -520,6 +520,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
> > @@ -541,7 +543,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
> >   
> 
> 

Re: [PATCH v2 01/10] PCI/DOE: Silence WARN splat with CONFIG_DEBUG_OBJECTS=y

[Lukas Wunner]

On Tue, Jan 24, 2023 at 10:32:08AM +0000, Jonathan Cameron wrote:
> On Mon, 23 Jan 2023 16:33:36 -0800 Ira Weiny <ira.weiny@intel.com> wrote:
> > 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.
[...]
> It's an unusual requirement, but this is indeed the minimal fix
> given current users.  Obviously becomes more sensible later in the
> series once you make the API synchronous only.

Okay, I'll amend the commit message as follows when respinning
to make more obvious what's being done here:

    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.

Thanks,

Lukas

Re: [PATCH v2 01/10] PCI/DOE: Silence WARN splat with CONFIG_DEBUG_OBJECTS=y

[Gregory Price]

On Mon, Jan 23, 2023 at 11:11:00AM +0100, 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.
>
> ... snip ...
> Reported-by: Gregory Price <gregory.price@memverge.com>

Tested-by: Gregory Price <gregory.price@memverge.com>
Reviewed-by: Gregory Price <gregory.price@memverge.com>

RE: [PATCH v2 01/10] PCI/DOE: Silence WARN splat with CONFIG_DEBUG_OBJECTS=y

[Dan Williams]

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.
> 
> 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>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: stable@vger.kernel.org # v6.0+
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

[PATCH v2 02/10] 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>
Cc: stable@vger.kernel.org # v6.0+
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/pci/doe.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
index 12a6752351bf..7451b5732044 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

Re: [PATCH v2 02/10] PCI/DOE: Fix memory leak with CONFIG_DEBUG_OBJECTS=y

[Ira Weiny]

Lukas Wunner wrote:
> 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>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: stable@vger.kernel.org # v6.0+
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/pci/doe.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> index 12a6752351bf..7451b5732044 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
> 

Re: [PATCH v2 02/10] PCI/DOE: Fix memory leak with CONFIG_DEBUG_OBJECTS=y

[Jonathan Cameron]

On Mon, 23 Jan 2023 16:35:54 -0800
Ira Weiny <ira.weiny@intel.com> wrote:

> Lukas Wunner wrote:
> > 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>  
> 
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Good find.

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

> 
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > Cc: stable@vger.kernel.org # v6.0+
> > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> >  drivers/pci/doe.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> > index 12a6752351bf..7451b5732044 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
> >   
> 
> 

RE: [PATCH v2 02/10] PCI/DOE: Fix memory leak with CONFIG_DEBUG_OBJECTS=y

[Dan Williams]

Lukas Wunner wrote:
> 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>
> Cc: stable@vger.kernel.org # v6.0+
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

While this could be squashed with the previous patch they really are
independent fixes.

[PATCH v2 03/10] 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>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
---
 drivers/pci/doe.c       | 65 +++++++++++++++++++++++++++++++----------
 include/linux/pci-doe.h |  6 +++-
 2 files changed, 55 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
index 7451b5732044..dce6af2ab574 100644
--- a/drivers/pci/doe.c
+++ b/drivers/pci/doe.c
@@ -319,26 +319,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);
@@ -549,3 +538,49 @@ 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.
+ *
+ * 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 ed9b4df792b8..1608e1536284 100644
--- a/include/linux/pci-doe.h
+++ b/include/linux/pci-doe.h
@@ -45,7 +45,7 @@ struct pci_doe_mb;
  */
 struct pci_doe_task {
 	struct pci_doe_protocol prot;
-	u32 *request_pl;
+	const u32 *request_pl;
 	size_t request_pl_sz;
 	u32 *response_pl;
 	size_t response_pl_sz;
@@ -74,4 +74,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

Re: [PATCH v2 03/10] PCI/DOE: Provide synchronous API and use it internally

[Ira Weiny]

Lukas Wunner 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 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>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> ---
>  drivers/pci/doe.c       | 65 +++++++++++++++++++++++++++++++----------
>  include/linux/pci-doe.h |  6 +++-
>  2 files changed, 55 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> index 7451b5732044..dce6af2ab574 100644
> --- a/drivers/pci/doe.c
> +++ b/drivers/pci/doe.c
> @@ -319,26 +319,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);
> @@ -549,3 +538,49 @@ 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.
> + *
> + * 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 ed9b4df792b8..1608e1536284 100644
> --- a/include/linux/pci-doe.h
> +++ b/include/linux/pci-doe.h
> @@ -45,7 +45,7 @@ struct pci_doe_mb;
>   */
>  struct pci_doe_task {
>  	struct pci_doe_protocol prot;
> -	u32 *request_pl;
> +	const u32 *request_pl;
>  	size_t request_pl_sz;
>  	u32 *response_pl;
>  	size_t response_pl_sz;
> @@ -74,4 +74,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
> 

Re: [PATCH v2 03/10] PCI/DOE: Provide synchronous API and use it internally

[Jonathan Cameron]

On Mon, 23 Jan 2023 11:13:00 +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 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>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>

Pushing the struct down is fine by me, though I'll note we had it
pretty much like this in one of the earlier versions and got a
request to use a struct instead to wrap up all the parameters.

Let's see if experience convinces people this is the right
approach this time :) It is perhaps easier to argue
now the completion is moved down as well as we'd end up with
a messy case of some elements of the structure being set in the
caller and others inside where it is called (or some messy
wrapper structures).  Been a while, but I don't think we had
such a strong argument in favour of this approach back then.

The const changes makes sense independent of the rest.

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


> ---
>  drivers/pci/doe.c       | 65 +++++++++++++++++++++++++++++++----------
>  include/linux/pci-doe.h |  6 +++-
>  2 files changed, 55 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> index 7451b5732044..dce6af2ab574 100644
> --- a/drivers/pci/doe.c
> +++ b/drivers/pci/doe.c
> @@ -319,26 +319,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);
> @@ -549,3 +538,49 @@ 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.
> + *
> + * 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 ed9b4df792b8..1608e1536284 100644
> --- a/include/linux/pci-doe.h
> +++ b/include/linux/pci-doe.h
> @@ -45,7 +45,7 @@ struct pci_doe_mb;
>   */
>  struct pci_doe_task {
>  	struct pci_doe_protocol prot;
> -	u32 *request_pl;
> +	const u32 *request_pl;
>  	size_t request_pl_sz;
>  	u32 *response_pl;
>  	size_t response_pl_sz;
> @@ -74,4 +74,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

Re: [PATCH v2 03/10] PCI/DOE: Provide synchronous API and use it internally

[Ira Weiny]

Jonathan Cameron wrote:
> On Mon, 23 Jan 2023 11:13:00 +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 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>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> 
> Pushing the struct down is fine by me, though I'll note we had it
> pretty much like this in one of the earlier versions and got a
> request to use a struct instead to wrap up all the parameters.

I had the same thought but decided not to pick at that old wound...  ;-)

> 
> Let's see if experience convinces people this is the right
> approach this time :) It is perhaps easier to argue
> now the completion is moved down as well as we'd end up with
> a messy case of some elements of the structure being set in the
> caller and others inside where it is called (or some messy
> wrapper structures).  Been a while, but I don't think we had
> such a strong argument in favour of this approach back then.

I think this makes a lot of sense and is clean enough.  I don't think the
number of parameters is outrageous.

Ira

> 
> The const changes makes sense independent of the rest.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> 
> > ---
> >  drivers/pci/doe.c       | 65 +++++++++++++++++++++++++++++++----------
> >  include/linux/pci-doe.h |  6 +++-
> >  2 files changed, 55 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> > index 7451b5732044..dce6af2ab574 100644
> > --- a/drivers/pci/doe.c
> > +++ b/drivers/pci/doe.c
> > @@ -319,26 +319,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);
> > @@ -549,3 +538,49 @@ 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.
> > + *
> > + * 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 ed9b4df792b8..1608e1536284 100644
> > --- a/include/linux/pci-doe.h
> > +++ b/include/linux/pci-doe.h
> > @@ -45,7 +45,7 @@ struct pci_doe_mb;
> >   */
> >  struct pci_doe_task {
> >  	struct pci_doe_protocol prot;
> > -	u32 *request_pl;
> > +	const u32 *request_pl;
> >  	size_t request_pl_sz;
> >  	u32 *response_pl;
> >  	size_t response_pl_sz;
> > @@ -74,4 +74,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
> 

RE: [PATCH v2 03/10] PCI/DOE: Provide synchronous API and use it internally

[Dan Williams]

Lukas Wunner 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 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>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>

While I do wish there was a way to avoid a 7-argument function I can't
think of a better way to do this that does not run afoul of the previous
problems with this interface.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

[PATCH v2 04/10] 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>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
---
 drivers/cxl/core/pci.c | 62 ++++++++++++++----------------------------
 1 file changed, 20 insertions(+), 42 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 57764e9cd19d..a02a2b005e6a 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -487,51 +487,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);
+		dev_err(dev, "DOE 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;
@@ -546,26 +521,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);
+			dev_err(dev, "DOE 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);
-- 
2.39.1

Re: [PATCH v2 04/10] cxl/pci: Use synchronous API for DOE

[Ira Weiny]

Lukas Wunner wrote:
> A synchronous API for DOE has just been introduced.  Convert CXL CDAT
> retrieval over to it.
> 
> Tested-by: Ira Weiny <ira.weiny@intel.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> ---
>  drivers/cxl/core/pci.c | 62 ++++++++++++++----------------------------
>  1 file changed, 20 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 57764e9cd19d..a02a2b005e6a 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -487,51 +487,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);
> +		dev_err(dev, "DOE 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;
> @@ -546,26 +521,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);
> +			dev_err(dev, "DOE 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);
> -- 
> 2.39.1
> 

Re: [PATCH v2 04/10] cxl/pci: Use synchronous API for DOE

[Li, Ming]

On 1/24/2023 8:52 AM, Ira Weiny wrote:
> Lukas Wunner wrote:
>> A synchronous API for DOE has just been introduced.  Convert CXL CDAT
>> retrieval over to it.
>>
>> Tested-by: Ira Weiny <ira.weiny@intel.com>
> 
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> 
>> Signed-off-by: Lukas Wunner <lukas@wunner.de>
>> Cc: Dan Williams <dan.j.williams@intel.com>
>> Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
>> ---
>>  drivers/cxl/core/pci.c | 62 ++++++++++++++----------------------------
>>  1 file changed, 20 insertions(+), 42 deletions(-)
>>

......

>>  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);
>> +		dev_err(dev, "DOE failed: %d", rc);
>>  		return rc;
>>  	}
>> -	wait_for_completion(&t.c);
>> -	if (t.task.rv < sizeof(u32))
>> +	if (rc < sizeof(u32))
>>  		return -EIO;
>>  

Sorry, I didn't find the original patchset email, only can reply here.
Should this "if (rc < sizeof(u32))" be "if (rc < 2 * sizeof(u32))"?
Because below code used response[1] directly, that means we need unless two u32 in response payload.

Thanks
Ming 

>> -	*length = t.response_pl[1];
>> +	*length = response[1]>>  	dev_dbg(dev, "CDAT length %zu\n", *length);
>>  
>>  	return 0;
>> @@ -546,26 +521,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);
>> +			dev_err(dev, "DOE 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);
>> -- 
>> 2.39.1
>>
> 
> 

Re: [PATCH v2 04/10] cxl/pci: Use synchronous API for DOE

[Li, Ming]

On 2/3/2023 4:53 PM, Li, Ming wrote:
> On 1/24/2023 8:52 AM, Ira Weiny wrote:
>> Lukas Wunner wrote:
>>> A synchronous API for DOE has just been introduced.  Convert CXL CDAT
>>> retrieval over to it.
>>>
>>> Tested-by: Ira Weiny <ira.weiny@intel.com>
>>
>> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
>>
>>> Signed-off-by: Lukas Wunner <lukas@wunner.de>
>>> Cc: Dan Williams <dan.j.williams@intel.com>
>>> Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
>>> ---
>>>  drivers/cxl/core/pci.c | 62 ++++++++++++++----------------------------
>>>  1 file changed, 20 insertions(+), 42 deletions(-)
>>>
> 
> ......
> 
>>>  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);
>>> +		dev_err(dev, "DOE failed: %d", rc);
>>>  		return rc;
>>>  	}
>>> -	wait_for_completion(&t.c);
>>> -	if (t.task.rv < sizeof(u32))
>>> +	if (rc < sizeof(u32))
>>>  		return -EIO;
>>>  
> 
> Sorry, I didn't find the original patchset email, only can reply here.
> Should this "if (rc < sizeof(u32))" be "if (rc < 2 * sizeof(u32))"?
> Because below code used response[1] directly, that means we need unless two u32 in response payload.

Sorry, at least(not unless) two u32 in response payload.

> Thanks
> Ming 
> 
>>> -	*length = t.response_pl[1];
>>> +	*length = response[1]>>  	dev_dbg(dev, "CDAT length %zu\n", *length);
>>>  
>>>  	return 0;
>>> @@ -546,26 +521,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);
>>> +			dev_err(dev, "DOE 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);
>>> -- 
>>> 2.39.1
>>>
>>
>>
> 

Re: [PATCH v2 04/10] cxl/pci: Use synchronous API for DOE

[Lukas Wunner]

On Fri, Feb 03, 2023 at 04:53:34PM +0800, Li, Ming wrote:
> On 1/24/2023 8:52 AM, Ira Weiny wrote:
> > Lukas Wunner wrote:
> > >  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);
> > > +		dev_err(dev, "DOE failed: %d", rc);
> > >  		return rc;
> > >  	}
> > > -	wait_for_completion(&t.c);
> > > -	if (t.task.rv < sizeof(u32))
> > > +	if (rc < sizeof(u32))
> > >  		return -EIO;
> > >  
> 
> Sorry, I didn't find the original patchset email, only can reply here.
> Should this "if (rc < sizeof(u32))" be "if (rc < 2 * sizeof(u32))"?
> Because below code used response[1] directly, that means we need unless
> two u32 in response payload.

Yes I spotted that as well, there's already a fixup on my
development branch:

  https://github.com/l1k/linux/commits/doe

It's in commit "cxl/pci: Handle truncated CDAT header" which is:

  https://github.com/l1k/linux/commit/208f256b319b

...but that URL may stop working as soon as I rebase the next time.

Actually there's a lot more broken here, there are 3 other new fixup
patches at the beginning of that development branch.

Thanks,

Lukas

Re: [PATCH v2 04/10] cxl/pci: Use synchronous API for DOE

[Jonathan Cameron]

On Mon, 23 Jan 2023 11:14:00 +0100
Lukas Wunner <lukas@wunner.de> wrote:

> 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>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>

The clean up here gives opportunities for 'right sizing' at least
the response to reading the header. The others are harder was we
don't know what each one is going to be.

May make more sense as a follow on patch though. 

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


> ---
>  drivers/cxl/core/pci.c | 62 ++++++++++++++----------------------------
>  1 file changed, 20 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 57764e9cd19d..a02a2b005e6a 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -487,51 +487,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];

As we aren't now using a single structure for multiple purposes
we should take the opportunity to cleanup the magic sizes (32 dword
is just intended to be 'big enough' for anything we expect to read.
Perhaps even declare a structure for the header case.

struct cdat_header_resp {
	u8 resp_code;
	u8 table_type; /* 0 - CDAT */
	u16 entry_handle; /* 0 - Header */
	u32 cdat_length;
	u8 rev;
	u8 checksum;
	u8 resvd[6];
	u32 sequence;
};

A lot less than 32 dword.



>  	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 < 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;
> @@ -546,26 +521,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];
As above, this is still a bit random.
Things we might be reading.
DSMAS: 6 dword
DSLBIS: 6 dword
DSIS: 2 dword
DSEMTS: 6 dword
SSLBIS: 4 dword + 2 * entires dwords.  This can get huge - as
can include p2p as well as the smaller usp / dsp set.

Right now we aren't reading from switches though so we can fix
that later (I posted an RFC for switches ages ago, but haven't
gotten back to it since then)

So for now probably leave this one at the 32 dwords.



>  		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);
> +			dev_err(dev, "DOE 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);

Re: [PATCH v2 04/10] cxl/pci: Use synchronous API for DOE

[Lukas Wunner]

On Tue, Jan 24, 2023 at 11:01:27AM +0000, Jonathan Cameron wrote:
> On Mon, 23 Jan 2023 11:14:00 +0100 Lukas Wunner <lukas@wunner.de> wrote:
> > A synchronous API for DOE has just been introduced.  Convert CXL CDAT
> > retrieval over to it.
> 
> The clean up here gives opportunities for 'right sizing' at least
> the response to reading the header. The others are harder was we
> don't know what each one is going to be.
> May make more sense as a follow on patch though. 

Thy will be done:

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


> > -		DECLARE_CDAT_DOE_TASK(CDAT_DOE_REQ(entry_handle), t);
> > +		u32 request = CDAT_DOE_REQ(entry_handle);
> > +		u32 response[32];
> 
> As above, this is still a bit random.
> Things we might be reading.
> DSMAS: 6 dword
> DSLBIS: 6 dword
> DSIS: 2 dword
> DSEMTS: 6 dword
> SSLBIS: 4 dword + 2 * entires dwords.  This can get huge - as
> can include p2p as well as the smaller usp / dsp set.
> 
> Right now we aren't reading from switches though so we can fix
> that later (I posted an RFC for switches ages ago, but haven't
> gotten back to it since then)
> 
> So for now probably leave this one at the 32 dwords.

I found a way to avoid a response buffer altogether.

Thanks,

Lukas

[PATCH v2 05/10] 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>
Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
---
 Changes v1 -> v2:
 * Deduplicate note in kernel-doc of struct pci_doe_task that caller need
   not initialize certain fields (Jonathan)

 drivers/pci/doe.c       | 45 +++++++++++++++++++++++++++++++++++++++--
 include/linux/pci-doe.h | 44 ----------------------------------------
 2 files changed, 43 insertions(+), 46 deletions(-)

diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
index dce6af2ab574..066400531d09 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 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,
@@ -516,7 +557,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;
@@ -537,7 +579,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 1608e1536284..7f16749c6aa3 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;
-	const 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,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

Re: [PATCH v2 05/10] PCI/DOE: Make asynchronous API private

[Ira Weiny]

Lukas Wunner wrote:
> 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>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> ---
>  Changes v1 -> v2:
>  * Deduplicate note in kernel-doc of struct pci_doe_task that caller need
>    not initialize certain fields (Jonathan)
> 
>  drivers/pci/doe.c       | 45 +++++++++++++++++++++++++++++++++++++++--
>  include/linux/pci-doe.h | 44 ----------------------------------------
>  2 files changed, 43 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> index dce6af2ab574..066400531d09 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 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,
> @@ -516,7 +557,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;
> @@ -537,7 +579,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 1608e1536284..7f16749c6aa3 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;
> -	const 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,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
> 

Re: [PATCH v2 05/10] PCI/DOE: Make asynchronous API private

[Jonathan Cameron]

On Mon, 23 Jan 2023 11:15:00 +0100
Lukas Wunner <lukas@wunner.de> wrote:

> 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>
> Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  Changes v1 -> v2:
>  * Deduplicate note in kernel-doc of struct pci_doe_task that caller need
>    not initialize certain fields (Jonathan)
> 
>  drivers/pci/doe.c       | 45 +++++++++++++++++++++++++++++++++++++++--
>  include/linux/pci-doe.h | 44 ----------------------------------------
>  2 files changed, 43 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> index dce6af2ab574..066400531d09 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 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,
> @@ -516,7 +557,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;
> @@ -537,7 +579,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 1608e1536284..7f16749c6aa3 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;
> -	const 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,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,

[PATCH v2 06/10] 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.

Tested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/doe.c | 103 +++++++++++++++++++++++++++++++---------------
 1 file changed, 70 insertions(+), 33 deletions(-)

diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
index 066400531d09..cc1fdd75ad2a 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
@@ -412,20 +412,6 @@ static int pci_doe_cache_protocols(struct pci_doe_mb *doe_mb)
 	return 0;
 }
 
-static void pci_doe_xa_destroy(void *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_flush_mb(void *mb)
 {
 	struct pci_doe_mb *doe_mb = mb;
@@ -442,7 +428,7 @@ static void pci_doe_flush_mb(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 +439,20 @@ static void pci_doe_flush_mb(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,35 +461,90 @@ 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 mailbox flush prior to using the mailbox to query protocols.
+	 * Use the mailbox to query protocols.
 	 */
-	rc = devm_add_action_or_reset(dev, pci_doe_flush_mb, 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);
+		goto err_flush;
+	}
+
+	return doe_mb;
+
+err_flush:
+	pci_doe_flush_mb(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;
+
+	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(&pdev->dev, pci_doe_destroy_mb, doe_mb);
+	if (rc) {
+		pci_doe_flush_mb(doe_mb);
+		pci_doe_destroy_mb(doe_mb);
 		return ERR_PTR(rc);
 	}
 
+	rc = devm_add_action_or_reset(&pdev->dev, pci_doe_flush_mb, doe_mb);
+	if (rc)
+		return ERR_PTR(rc);
+
 	return doe_mb;
 }
 EXPORT_SYMBOL_GPL(pcim_doe_create_mb);
-- 
2.39.1

Re: [PATCH v2 06/10] PCI/DOE: Allow mailbox creation without devres management

[Jonathan Cameron]

On Mon, 23 Jan 2023 11:16:00 +0100
Lukas Wunner <lukas@wunner.de> wrote:

> 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.
> 
> Tested-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
Hi Lukas,

A few comments inline.

In particular I'd like to understand why flushing in the tear down
can't always be done as that makes the code more complex.

Might become clear in later patches though as I've not read ahead yet!

Jonathan

> ---
>  drivers/pci/doe.c | 103 +++++++++++++++++++++++++++++++---------------
>  1 file changed, 70 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> index 066400531d09..cc1fdd75ad2a 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
> @@ -412,20 +412,6 @@ static int pci_doe_cache_protocols(struct pci_doe_mb *doe_mb)
>  	return 0;
>  }
>  
> -static void pci_doe_xa_destroy(void *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_flush_mb(void *mb)
>  {
>  	struct pci_doe_mb *doe_mb = mb;
> @@ -442,7 +428,7 @@ static void pci_doe_flush_mb(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 +439,20 @@ static void pci_doe_flush_mb(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);
See below - I'd move xa_init() down to just above the pci_doe_cache_protocols()
call.

> -	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,35 +461,90 @@ 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 mailbox flush prior to using the mailbox to query protocols.
> +	 * Use the mailbox to query protocols.
>  	 */
> -	rc = devm_add_action_or_reset(dev, pci_doe_flush_mb, 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);
> +		goto err_flush;
> +	}
> +
> +	return doe_mb;
> +
> +err_flush:
> +	pci_doe_flush_mb(doe_mb);
> +	xa_destroy(&doe_mb->prots);

Why the reorder wrt to the original devm managed cleanup?
I'd expect this to happen on any error path after the xa_init.

It doesn't matter in practice because there isn't anything to
do until after pci_doe_cache_protocols though.  Maybe
simplest option would be move xa_init() down to just above
the call to pci_doe_cache_protocols()?  That way the order
you have here would meet the 'obviously correct' test.


> +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.

Could you comment on why it doesn't make sense to flush the
mb on this path?  Perhaps add a comment here to say what state
we should be in before calling this?

Not flushing here means you need more complex handling in
error paths.

> + */
> +static void pci_doe_destroy_mb(void *ptr)
> +{
> +	struct pci_doe_mb *doe_mb = ptr;
> +
> +	xa_destroy(&doe_mb->prots);

If making the change above, also push the xa_destroy() below
the destroy_workqueue() here.

> +	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(&pdev->dev, pci_doe_destroy_mb, doe_mb);
> +	if (rc) {
> +		pci_doe_flush_mb(doe_mb);
> +		pci_doe_destroy_mb(doe_mb);
>  		return ERR_PTR(rc);
>  	}
>  
> +	rc = devm_add_action_or_reset(&pdev->dev, pci_doe_flush_mb, doe_mb);
> +	if (rc)
> +		return ERR_PTR(rc);
> +
>  	return doe_mb;
>  }
>  EXPORT_SYMBOL_GPL(pcim_doe_create_mb);

Re: [PATCH v2 06/10] PCI/DOE: Allow mailbox creation without devres management

[Jonathan Cameron]

On Tue, 24 Jan 2023 12:15:43 +0000
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Mon, 23 Jan 2023 11:16:00 +0100
> Lukas Wunner <lukas@wunner.de> wrote:
> 
> > 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.
> > 
> > Tested-by: Ira Weiny <ira.weiny@intel.com>
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>  
> Hi Lukas,
> 
> A few comments inline.
> 
> In particular I'd like to understand why flushing in the tear down
> can't always be done as that makes the code more complex.
> 
> Might become clear in later patches though as I've not read ahead yet!
Ah.. It's in the patch description of the next patch. So ignore this question.

Thanks,

Jonathan

> 
> Jonathan
> 
> > ---
> >  drivers/pci/doe.c | 103 +++++++++++++++++++++++++++++++---------------
> >  1 file changed, 70 insertions(+), 33 deletions(-)
> > 
> > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> > index 066400531d09..cc1fdd75ad2a 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
> > @@ -412,20 +412,6 @@ static int pci_doe_cache_protocols(struct pci_doe_mb *doe_mb)
> >  	return 0;
> >  }
> >  
> > -static void pci_doe_xa_destroy(void *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_flush_mb(void *mb)
> >  {
> >  	struct pci_doe_mb *doe_mb = mb;
> > @@ -442,7 +428,7 @@ static void pci_doe_flush_mb(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 +439,20 @@ static void pci_doe_flush_mb(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);  
> See below - I'd move xa_init() down to just above the pci_doe_cache_protocols()
> call.
> 
> > -	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,35 +461,90 @@ 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 mailbox flush prior to using the mailbox to query protocols.
> > +	 * Use the mailbox to query protocols.
> >  	 */
> > -	rc = devm_add_action_or_reset(dev, pci_doe_flush_mb, 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);
> > +		goto err_flush;
> > +	}
> > +
> > +	return doe_mb;
> > +
> > +err_flush:
> > +	pci_doe_flush_mb(doe_mb);
> > +	xa_destroy(&doe_mb->prots);  
> 
> Why the reorder wrt to the original devm managed cleanup?
> I'd expect this to happen on any error path after the xa_init.
> 
> It doesn't matter in practice because there isn't anything to
> do until after pci_doe_cache_protocols though.  Maybe
> simplest option would be move xa_init() down to just above
> the call to pci_doe_cache_protocols()?  That way the order
> you have here would meet the 'obviously correct' test.
> 
> 
> > +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.  
> 
> Could you comment on why it doesn't make sense to flush the
> mb on this path?  Perhaps add a comment here to say what state
> we should be in before calling this?
> 
> Not flushing here means you need more complex handling in
> error paths.
> 
> > + */
> > +static void pci_doe_destroy_mb(void *ptr)
> > +{
> > +	struct pci_doe_mb *doe_mb = ptr;
> > +
> > +	xa_destroy(&doe_mb->prots);  
> 
> If making the change above, also push the xa_destroy() below
> the destroy_workqueue() here.
> 
> > +	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(&pdev->dev, pci_doe_destroy_mb, doe_mb);
> > +	if (rc) {
> > +		pci_doe_flush_mb(doe_mb);
> > +		pci_doe_destroy_mb(doe_mb);
> >  		return ERR_PTR(rc);
> >  	}
> >  
> > +	rc = devm_add_action_or_reset(&pdev->dev, pci_doe_flush_mb, doe_mb);
> > +	if (rc)
> > +		return ERR_PTR(rc);
> > +
> >  	return doe_mb;
> >  }
> >  EXPORT_SYMBOL_GPL(pcim_doe_create_mb);  
> 

Re: [PATCH v2 06/10] PCI/DOE: Allow mailbox creation without devres management

[Li, Ming]

On 1/24/2023 8:15 PM, Jonathan Cameron wrote:
> On Mon, 23 Jan 2023 11:16:00 +0100
> Lukas Wunner <lukas@wunner.de> wrote:
> 
>> 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.
>>
>> Tested-by: Ira Weiny <ira.weiny@intel.com>
>> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Hi Lukas,
> 
> A few comments inline.
> 
> In particular I'd like to understand why flushing in the tear down
> can't always be done as that makes the code more complex.
> 
> Might become clear in later patches though as I've not read ahead yet!
> 
> Jonathan
> 
>> ---
>>  drivers/pci/doe.c | 103 +++++++++++++++++++++++++++++++---------------
>>  1 file changed, 70 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
>> index 066400531d09..cc1fdd75ad2a 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
>> @@ -412,20 +412,6 @@ static int pci_doe_cache_protocols(struct pci_doe_mb *doe_mb)
>>  	return 0;
>>  }
>>  

......

>> +/**
>> + * 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)

Sorry, didn't find the original patch, reply on here.
I don't get why uses "void *ptr" as the parameter of this function, maybe I miss something. I guess we can use "struct pci_doe_mb *doe_mb" as the parameter.

Thanks
Ming


>> +{
>> +	struct pci_doe_mb *doe_mb = ptr;
>> +
>> +	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(&pdev->dev, pci_doe_destroy_mb, doe_mb);
>> +	if (rc) {
>> +		pci_doe_flush_mb(doe_mb);
>> +		pci_doe_destroy_mb(doe_mb);
>>  		return ERR_PTR(rc);
>>  	}
>>  
>> +	rc = devm_add_action_or_reset(&pdev->dev, pci_doe_flush_mb, doe_mb);
>> +	if (rc)
>> +		return ERR_PTR(rc);
>> +
>>  	return doe_mb;
>>  }
>>  EXPORT_SYMBOL_GPL(pcim_doe_create_mb);
> 

Re: [PATCH v2 06/10] PCI/DOE: Allow mailbox creation without devres management

[Li, Ming]

On 2/3/2023 5:06 PM, Li, Ming wrote:
> On 1/24/2023 8:15 PM, Jonathan Cameron wrote:
>> On Mon, 23 Jan 2023 11:16:00 +0100
>> Lukas Wunner <lukas@wunner.de> wrote:
>>
>>> 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.
>>>
>>> Tested-by: Ira Weiny <ira.weiny@intel.com>
>>> Signed-off-by: Lukas Wunner <lukas@wunner.de>
>> Hi Lukas,
>>
>> A few comments inline.
>>
>> In particular I'd like to understand why flushing in the tear down
>> can't always be done as that makes the code more complex.
>>
>> Might become clear in later patches though as I've not read ahead yet!
>>
>> Jonathan
>>
>>> ---
>>>  drivers/pci/doe.c | 103 +++++++++++++++++++++++++++++++---------------
>>>  1 file changed, 70 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
>>> index 066400531d09..cc1fdd75ad2a 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
>>> @@ -412,20 +412,6 @@ static int pci_doe_cache_protocols(struct pci_doe_mb *doe_mb)
>>>  	return 0;
>>>  }
>>>  
> 
> ......
> 
>>> +/**
>>> + * 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)
> 
> Sorry, didn't find the original patch, reply on here.
> I don't get why uses "void *ptr" as the parameter of this function, maybe I miss something. I guess we can use "struct pci_doe_mb *doe_mb" as the parameter.
> 
> Thanks
> Ming
> 

Please ignore my comment, I saw it has been changed by PATCH #9

Thanks
Ming

> 
>>> +{
>>> +	struct pci_doe_mb *doe_mb = ptr;
>>> +
>>> +	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(&pdev->dev, pci_doe_destroy_mb, doe_mb);
>>> +	if (rc) {
>>> +		pci_doe_flush_mb(doe_mb);
>>> +		pci_doe_destroy_mb(doe_mb);
>>>  		return ERR_PTR(rc);
>>>  	}
>>>  
>>> +	rc = devm_add_action_or_reset(&pdev->dev, pci_doe_flush_mb, doe_mb);
>>> +	if (rc)
>>> +		return ERR_PTR(rc);
>>> +
>>>  	return doe_mb;
>>>  }
>>>  EXPORT_SYMBOL_GPL(pcim_doe_create_mb);
>>
> 

Re: [PATCH v2 06/10] PCI/DOE: Allow mailbox creation without devres management

[Lukas Wunner]

On Fri, Feb 03, 2023 at 05:06:21PM +0800, Li, Ming wrote:
> On 1/24/2023 8:15 PM, Jonathan Cameron wrote:
> > On Mon, 23 Jan 2023 11:16:00 +0100 Lukas Wunner <lukas@wunner.de> wrote:
> > > +/**
> > > + * 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)
> 
> I don't get why uses "void *ptr" as the parameter of this function,
> maybe I miss something. I guess we can use "struct pci_doe_mb *doe_mb"
> as the parameter.

It's a "void *ptr" argument so that pci_doe_destroy_mb() can serve
as a callback for devm_add_action().  But as you've correctly noted,
devm_add_action() is removed in a subsequent commit and then the
argument is adjusted to "struct pci_doe_mb *doe_mb".

Thanks,

Lukas

Re: [PATCH v2 06/10] PCI/DOE: Allow mailbox creation without devres management

[Lukas Wunner]

On Tue, Jan 24, 2023 at 12:15:43PM +0000, Jonathan Cameron wrote:
> On Mon, 23 Jan 2023 11:16:00 +0100 Lukas Wunner <lukas@wunner.de> wrote:
> > 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.
[...]
> I'd like to understand why flushing in the tear down
> can't always be done as that makes the code more complex.

After sending out v2, I realized I had made a mistake:

In v2, on device removal I canceled any ongoing DOE exchanges
and declared the DOE mailbox dead before unbinding the driver
from a device.  That's the right thing to do for surprise removal
because you don't want to wait for an ongoing exchange to time out.

However we also have a code path for orderly device removal,
either via sysfs or by pressing the Attention Button (if present).
In that case, it should be legal for the driver to still perform
DOE exchanges in its ->remove() hook.

So in v3 I've changed the behavior to only cancel requests on
surprise removal.  By doing so, I was able to always flush on
mailbox destruction, as you've requested, and thereby simplify
the error paths.


> > +err_flush:
> > +	pci_doe_flush_mb(doe_mb);
> > +	xa_destroy(&doe_mb->prots);
> 
> Why the reorder wrt to the original devm managed cleanup?
> I'd expect this to happen on any error path after the xa_init.
> 
> It doesn't matter in practice because there isn't anything to
> do until after pci_doe_cache_protocols though.

Right, it's unnecessary to call xa_destroy() if
alloc_ordered_workqueue() failed 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().

I've amended the commit message to explain that, but otherwise
did not change the code in v3.  Let me know if you have any
objections or feel strongly about moving xa_init().

Thanks,

Lukas

[PATCH v2 07/10] 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.

Overcome this limitation 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.

On device removal, tear down mailboxes in two steps:

In pci_stop_dev(), before the driver is unbound, stop ongoing DOE
exchanges and prevent new ones from being scheduled.  This ensures that
a hot-removed device doesn't needlessly wait for a running exchange to
time out.

In pci_destroy_dev(), after the driver is unbound, free the mailboxes
and their internal data structures.

Tested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/doe.c       | 71 +++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h       | 10 ++++++
 drivers/pci/probe.c     |  1 +
 drivers/pci/remove.c    |  2 ++
 include/linux/pci-doe.h |  2 ++
 include/linux/pci.h     |  3 ++
 6 files changed, 89 insertions(+)

diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
index cc1fdd75ad2a..06c57af05570 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 */
@@ -662,3 +664,72 @@ 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))
+			continue;
+
+		rc = xa_insert(&pdev->doe_mbs, offset, doe_mb, GFP_KERNEL);
+		if (rc) {
+			pci_doe_flush_mb(doe_mb);
+			pci_doe_destroy_mb(doe_mb);
+			pci_err(pdev, "[%x] failed to insert mailbox: %d\n",
+				offset, rc);
+		}
+	}
+}
+
+void pci_doe_stop(struct pci_dev *pdev)
+{
+	struct pci_doe_mb *doe_mb;
+	unsigned long index;
+
+	xa_for_each(&pdev->doe_mbs, index, doe_mb)
+		pci_doe_flush_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);
+}
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 9ed3b5550043..94656c1a01c0 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -777,6 +777,16 @@ static inline pci_power_t mid_pci_get_power_state(struct pci_dev *pdev)
 }
 #endif
 
+#ifdef CONFIG_PCI_DOE
+void pci_doe_init(struct pci_dev *pdev);
+void pci_doe_stop(struct pci_dev *pdev);
+void pci_doe_destroy(struct pci_dev *pdev);
+#else
+static inline void pci_doe_init(struct pci_dev *pdev) { }
+static inline void pci_doe_stop(struct pci_dev *pdev) { }
+static inline void pci_doe_destroy(struct pci_dev *pdev) { }
+#endif
+
 /*
  * Config Address for PCI Configuration Mechanism #1
  *
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 1779582fb500..65e60ee50489 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2476,6 +2476,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..739c7b0f5b91 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -16,6 +16,7 @@ static void pci_free_resources(struct pci_dev *dev)
 
 static void pci_stop_dev(struct pci_dev *dev)
 {
+	pci_doe_stop(dev);
 	pci_pme_active(dev, false);
 
 	if (pci_dev_is_added(dev)) {
@@ -39,6 +40,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 adffd65e84b4..254c79f9013a 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

Re: [PATCH v2 07/10] PCI/DOE: Create mailboxes on device enumeration

[Ira Weiny]

Lukas Wunner wrote:
> 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.
> 
> Overcome this limitation 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.
> 
> On device removal, tear down mailboxes in two steps:
> 
> In pci_stop_dev(), before the driver is unbound, stop ongoing DOE
> exchanges and prevent new ones from being scheduled.  This ensures that
> a hot-removed device doesn't needlessly wait for a running exchange to
> time out.
> 
> In pci_destroy_dev(), after the driver is unbound, free the mailboxes
> and their internal data structures.
> 
> Tested-by: Ira Weiny <ira.weiny@intel.com>

I rather like this.  Much cleaner, thanks!

Minor comment below.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/pci/doe.c       | 71 +++++++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci.h       | 10 ++++++
>  drivers/pci/probe.c     |  1 +
>  drivers/pci/remove.c    |  2 ++
>  include/linux/pci-doe.h |  2 ++
>  include/linux/pci.h     |  3 ++
>  6 files changed, 89 insertions(+)
> 
> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> index cc1fdd75ad2a..06c57af05570 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 */
> @@ -662,3 +664,72 @@ 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))
> +			continue;

I feel like a pci_dbg() would be nice here.  But not needed.

Ira

> +
> +		rc = xa_insert(&pdev->doe_mbs, offset, doe_mb, GFP_KERNEL);
> +		if (rc) {
> +			pci_doe_flush_mb(doe_mb);
> +			pci_doe_destroy_mb(doe_mb);
> +			pci_err(pdev, "[%x] failed to insert mailbox: %d\n",
> +				offset, rc);
> +		}
> +	}
> +}
> +
> +void pci_doe_stop(struct pci_dev *pdev)
> +{
> +	struct pci_doe_mb *doe_mb;
> +	unsigned long index;
> +
> +	xa_for_each(&pdev->doe_mbs, index, doe_mb)
> +		pci_doe_flush_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);
> +}
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 9ed3b5550043..94656c1a01c0 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -777,6 +777,16 @@ static inline pci_power_t mid_pci_get_power_state(struct pci_dev *pdev)
>  }
>  #endif
>  
> +#ifdef CONFIG_PCI_DOE
> +void pci_doe_init(struct pci_dev *pdev);
> +void pci_doe_stop(struct pci_dev *pdev);
> +void pci_doe_destroy(struct pci_dev *pdev);
> +#else
> +static inline void pci_doe_init(struct pci_dev *pdev) { }
> +static inline void pci_doe_stop(struct pci_dev *pdev) { }
> +static inline void pci_doe_destroy(struct pci_dev *pdev) { }
> +#endif
> +
>  /*
>   * Config Address for PCI Configuration Mechanism #1
>   *
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 1779582fb500..65e60ee50489 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2476,6 +2476,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..739c7b0f5b91 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -16,6 +16,7 @@ static void pci_free_resources(struct pci_dev *dev)
>  
>  static void pci_stop_dev(struct pci_dev *dev)
>  {
> +	pci_doe_stop(dev);
>  	pci_pme_active(dev, false);
>  
>  	if (pci_dev_is_added(dev)) {
> @@ -39,6 +40,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 adffd65e84b4..254c79f9013a 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
> 

Re: [PATCH v2 07/10] PCI/DOE: Create mailboxes on device enumeration

[Jonathan Cameron]

On Mon, 23 Jan 2023 11:17:00 +0100
Lukas Wunner <lukas@wunner.de> wrote:

> 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.
> 
> Overcome this limitation 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.
> 
> On device removal, tear down mailboxes in two steps:
> 
> In pci_stop_dev(), before the driver is unbound, stop ongoing DOE
> exchanges and prevent new ones from being scheduled.  This ensures that
> a hot-removed device doesn't needlessly wait for a running exchange to
> time out.

Ah. I didn't have to go far to find answer to my earlier query!  This
needs to be two step - hence the split in the previous patch.

> 
> In pci_destroy_dev(), after the driver is unbound, free the mailboxes
> and their internal data structures.
> 
> Tested-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Very nice.  One comment on a possible future need inline.

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


> +/**
> + * 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)

This is good for now.  We may want eventually to be slightly
more flexible and allow for a 'find instance X of a DOE that supports Y'.
Can solve that when we need it though.

> +{
> +	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);

[PATCH v2 08/10] 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>
---
 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 a02a2b005e6a..5cb6ffa8df0e 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -459,27 +459,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)					\
 	(FIELD_PREP(CXL_DOE_TABLE_ACCESS_REQ_CODE,			\
 		    CXL_DOE_TABLE_ACCESS_REQ_CODE_READ) |		\
@@ -569,10 +548,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 ab138004f644..e1a1b23cf56c 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -227,7 +227,6 @@ struct cxl_endpoint_dvsec_info {
  * @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
  * @mbox_send: @dev specific transport for transmitting mailbox commands
  *
  * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
@@ -264,8 +263,6 @@ struct cxl_dev_state {
 	resource_size_t component_reg_phys;
 	u64 serial;
 
-	struct xarray doe_mbs;
-
 	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 33083a522fd1..f8b8e514a3c6 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"
@@ -359,52 +358,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
@@ -469,8 +422,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

Re: [PATCH v2 08/10] cxl/pci: Use CDAT DOE mailbox created by PCI core

[Ira Weiny]

Lukas Wunner wrote:
> 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>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  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 a02a2b005e6a..5cb6ffa8df0e 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -459,27 +459,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)					\
>  	(FIELD_PREP(CXL_DOE_TABLE_ACCESS_REQ_CODE,			\
>  		    CXL_DOE_TABLE_ACCESS_REQ_CODE_READ) |		\
> @@ -569,10 +548,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 ab138004f644..e1a1b23cf56c 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -227,7 +227,6 @@ struct cxl_endpoint_dvsec_info {
>   * @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
>   * @mbox_send: @dev specific transport for transmitting mailbox commands
>   *
>   * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
> @@ -264,8 +263,6 @@ struct cxl_dev_state {
>  	resource_size_t component_reg_phys;
>  	u64 serial;
>  
> -	struct xarray doe_mbs;
> -
>  	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 33083a522fd1..f8b8e514a3c6 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"
> @@ -359,52 +358,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
> @@ -469,8 +422,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
> 

Re: [PATCH v2 08/10] cxl/pci: Use CDAT DOE mailbox created by PCI core

[Jonathan Cameron]

On Mon, 23 Jan 2023 11:18:00 +0100
Lukas Wunner <lukas@wunner.de> wrote:

> 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: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[PATCH v2 09/10] 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_flush_mb() and pci_doe_destroy_mb() are no longer used as
callbacks for devm_add_action(), so refactor them 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>
---
 .clang-format           |  1 -
 drivers/pci/doe.c       | 52 ++++-------------------------------------
 include/linux/pci-doe.h | 14 -----------
 3 files changed, 5 insertions(+), 62 deletions(-)

diff --git a/.clang-format b/.clang-format
index b62836419ea3..cb1c17c7fcc9 100644
--- a/.clang-format
+++ b/.clang-format
@@ -520,7 +520,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 06c57af05570..0263bcfdddd8 100644
--- a/drivers/pci/doe.c
+++ b/drivers/pci/doe.c
@@ -414,10 +414,8 @@ static int pci_doe_cache_protocols(struct pci_doe_mb *doe_mb)
 	return 0;
 }
 
-static void pci_doe_flush_mb(void *mb)
+static void pci_doe_flush_mb(struct pci_doe_mb *doe_mb)
 {
-	struct pci_doe_mb *doe_mb = mb;
-
 	/* Stop all pending work items from starting */
 	set_bit(PCI_DOE_FLAG_DEAD, &doe_mb->flags);
 
@@ -457,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) {
@@ -501,56 +499,17 @@ 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;
-
 	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(&pdev->dev, pci_doe_destroy_mb, doe_mb);
-	if (rc) {
-		pci_doe_flush_mb(doe_mb);
-		pci_doe_destroy_mb(doe_mb);
-		return ERR_PTR(rc);
-	}
-
-	rc = devm_add_action_or_reset(&pdev->dev, pci_doe_flush_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
@@ -560,7 +519,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;
@@ -575,7 +534,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

Re: [PATCH v2 09/10] PCI/DOE: Make mailbox creation API private

[Ira Weiny]

Lukas Wunner wrote:
> 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_flush_mb() and pci_doe_destroy_mb() are no longer used as
> callbacks for devm_add_action(), so refactor them 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>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  .clang-format           |  1 -
>  drivers/pci/doe.c       | 52 ++++-------------------------------------
>  include/linux/pci-doe.h | 14 -----------
>  3 files changed, 5 insertions(+), 62 deletions(-)
> 
> diff --git a/.clang-format b/.clang-format
> index b62836419ea3..cb1c17c7fcc9 100644
> --- a/.clang-format
> +++ b/.clang-format
> @@ -520,7 +520,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 06c57af05570..0263bcfdddd8 100644
> --- a/drivers/pci/doe.c
> +++ b/drivers/pci/doe.c
> @@ -414,10 +414,8 @@ static int pci_doe_cache_protocols(struct pci_doe_mb *doe_mb)
>  	return 0;
>  }
>  
> -static void pci_doe_flush_mb(void *mb)
> +static void pci_doe_flush_mb(struct pci_doe_mb *doe_mb)
>  {
> -	struct pci_doe_mb *doe_mb = mb;
> -
>  	/* Stop all pending work items from starting */
>  	set_bit(PCI_DOE_FLAG_DEAD, &doe_mb->flags);
>  
> @@ -457,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) {
> @@ -501,56 +499,17 @@ 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;
> -
>  	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(&pdev->dev, pci_doe_destroy_mb, doe_mb);
> -	if (rc) {
> -		pci_doe_flush_mb(doe_mb);
> -		pci_doe_destroy_mb(doe_mb);
> -		return ERR_PTR(rc);
> -	}
> -
> -	rc = devm_add_action_or_reset(&pdev->dev, pci_doe_flush_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
> @@ -560,7 +519,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;
> @@ -575,7 +534,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
> 

Re: [PATCH v2 09/10] PCI/DOE: Make mailbox creation API private

[Jonathan Cameron]

On Mon, 23 Jan 2023 11:19:00 +0100
Lukas Wunner <lukas@wunner.de> wrote:

> 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_flush_mb() and pci_doe_destroy_mb() are no longer used as
> callbacks for devm_add_action(), so refactor them 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: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[PATCH v2 10/10] 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.

Tested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
---
 drivers/pci/doe.c | 66 ++++++++++++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 26 deletions(-)

diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
index 0263bcfdddd8..2113ec95379f 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(u32);
+	length = 2 + DIV_ROUND_UP(task->request_pl_sz, sizeof(u32));
 	if (length > PCI_DOE_MAX_LENGTH)
 		return -EIO;
 	if (length == PCI_DOE_MAX_LENGTH)
@@ -184,10 +177,20 @@ 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(u32); i++)
 		pci_write_config_dword(pdev, offset + PCI_DOE_WRITE,
 				       task->request_pl[i]);
 
+	/* Write last payload dword */
+	remainder = task->request_pl_sz % sizeof(u32);
+	if (remainder) {
+		val = 0;
+		memcpy(&val, &task->request_pl[i], remainder);
+		pci_write_config_dword(pdev, offset + PCI_DOE_WRITE, val);
+	}
+
 	pci_doe_write_ctrl(doe_mb, PCI_DOE_CTRL_GO);
 
 	return 0;
@@ -207,11 +210,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 +241,34 @@ 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));
-	/* 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]);
+	received = task->response_pl_sz;
+	payload_length = DIV_ROUND_UP(task->response_pl_sz, sizeof(u32));
+	remainder = task->response_pl_sz % sizeof(u32);
+	if (!remainder)
+		remainder = sizeof(u32);
+
+	if (length < payload_length) {
+		received = length * sizeof(u32);
+		payload_length = length;
+		remainder = sizeof(u32);
+	}
+
+	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,
+					      &task->response_pl[i]);
+			pci_write_config_dword(pdev, offset + PCI_DOE_READ, 0);
+		}
+
+		/* Read last payload dword */
+		pci_read_config_dword(pdev, offset + PCI_DOE_READ, &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 +282,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 received;
 }
 
 static void signal_task_complete(struct pci_doe_task *task, int rv)
@@ -560,14 +582,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(u32) ||
-	    task->response_pl_sz < sizeof(u32))
-		return -EINVAL;
-
 	if (test_bit(PCI_DOE_FLAG_DEAD, &doe_mb->flags))
 		return -EIO;
 
-- 
2.39.1

Re: [PATCH v2 10/10] PCI/DOE: Relax restrictions on request and response size

[Bjorn Helgaas]

Hi Lukas,

On Mon, Jan 23, 2023 at 11:20:00AM +0100, Lukas Wunner wrote:
> 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.

Can you add a note about this non-dwordness?  The sec 6.30.1 Data
Object Header 2 "Length" field is in DW and the code in
pci_doe_send_req() and pci_doe_recv_resp() still reads/writes dwords.
I don't see the 6.31 text that requires non-dword sizes.

From a spec point of view, AFAICS, DOE is still specified in dwords,
but I guess you're leaving the actual PCI config-level DOE interface
in dwords and just making it more convenient for callers by having
pci_doe_*() hide the details of any partial dwords at the end by
transferring the entire dword over PCI but only copying the requested
bytes to/from the caller's buffer?

> Tested-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> ---
>  drivers/pci/doe.c | 66 ++++++++++++++++++++++++++++-------------------
>  1 file changed, 40 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> index 0263bcfdddd8..2113ec95379f 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(u32);
> +	length = 2 + DIV_ROUND_UP(task->request_pl_sz, sizeof(u32));
>  	if (length > PCI_DOE_MAX_LENGTH)
>  		return -EIO;
>  	if (length == PCI_DOE_MAX_LENGTH)
> @@ -184,10 +177,20 @@ 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(u32); i++)
>  		pci_write_config_dword(pdev, offset + PCI_DOE_WRITE,
>  				       task->request_pl[i]);
>  
> +	/* Write last payload dword */
> +	remainder = task->request_pl_sz % sizeof(u32);
> +	if (remainder) {
> +		val = 0;
> +		memcpy(&val, &task->request_pl[i], remainder);
> +		pci_write_config_dword(pdev, offset + PCI_DOE_WRITE, val);
> +	}
> +
>  	pci_doe_write_ctrl(doe_mb, PCI_DOE_CTRL_GO);
>  
>  	return 0;
> @@ -207,11 +210,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 +241,34 @@ 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));
> -	/* 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]);
> +	received = task->response_pl_sz;
> +	payload_length = DIV_ROUND_UP(task->response_pl_sz, sizeof(u32));
> +	remainder = task->response_pl_sz % sizeof(u32);
> +	if (!remainder)
> +		remainder = sizeof(u32);
> +
> +	if (length < payload_length) {
> +		received = length * sizeof(u32);
> +		payload_length = length;
> +		remainder = sizeof(u32);
> +	}
> +
> +	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,
> +					      &task->response_pl[i]);
> +			pci_write_config_dword(pdev, offset + PCI_DOE_READ, 0);
> +		}
> +
> +		/* Read last payload dword */
> +		pci_read_config_dword(pdev, offset + PCI_DOE_READ, &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 +282,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 received;
>  }
>  
>  static void signal_task_complete(struct pci_doe_task *task, int rv)
> @@ -560,14 +582,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(u32) ||
> -	    task->response_pl_sz < sizeof(u32))
> -		return -EINVAL;
> -
>  	if (test_bit(PCI_DOE_FLAG_DEAD, &doe_mb->flags))
>  		return -EIO;
>  
> -- 
> 2.39.1
> 

Re: [PATCH v2 10/10] PCI/DOE: Relax restrictions on request and response size

[Ira Weiny]

Lukas Wunner wrote:
> 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.
> 
> Tested-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> ---
>  drivers/pci/doe.c | 66 ++++++++++++++++++++++++++++-------------------
>  1 file changed, 40 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> index 0263bcfdddd8..2113ec95379f 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(u32);
> +	length = 2 + DIV_ROUND_UP(task->request_pl_sz, sizeof(u32));
>  	if (length > PCI_DOE_MAX_LENGTH)
>  		return -EIO;
>  	if (length == PCI_DOE_MAX_LENGTH)
> @@ -184,10 +177,20 @@ 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(u32); i++)
>  		pci_write_config_dword(pdev, offset + PCI_DOE_WRITE,
>  				       task->request_pl[i]);
>  
> +	/* Write last payload dword */
> +	remainder = task->request_pl_sz % sizeof(u32);
> +	if (remainder) {
> +		val = 0;
> +		memcpy(&val, &task->request_pl[i], remainder);

Are there any issues with endianess here?

> +		pci_write_config_dword(pdev, offset + PCI_DOE_WRITE, val);
> +	}
> +
>  	pci_doe_write_ctrl(doe_mb, PCI_DOE_CTRL_GO);
>  
>  	return 0;
> @@ -207,11 +210,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 +241,34 @@ 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));
> -	/* 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]);
> +	received = task->response_pl_sz;
> +	payload_length = DIV_ROUND_UP(task->response_pl_sz, sizeof(u32));
> +	remainder = task->response_pl_sz % sizeof(u32);
> +	if (!remainder)
> +		remainder = sizeof(u32);
> +
> +	if (length < payload_length) {
> +		received = length * sizeof(u32);
> +		payload_length = length;
> +		remainder = sizeof(u32);

It was a bit confusing why remainder was set to a dword here.  But I got
that it is because length and payload_length are both in dwords.

> +	}
> +
> +	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,
> +					      &task->response_pl[i]);
> +			pci_write_config_dword(pdev, offset + PCI_DOE_READ, 0);
> +		}
> +
> +		/* Read last payload dword */
> +		pci_read_config_dword(pdev, offset + PCI_DOE_READ, &val);
> +		memcpy(&task->response_pl[i], &val, remainder);

Same question on endianess.

Ira

>  		/* 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 +282,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 received;
>  }
>  
>  static void signal_task_complete(struct pci_doe_task *task, int rv)
> @@ -560,14 +582,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(u32) ||
> -	    task->response_pl_sz < sizeof(u32))
> -		return -EINVAL;
> -
>  	if (test_bit(PCI_DOE_FLAG_DEAD, &doe_mb->flags))
>  		return -EIO;
>  
> -- 
> 2.39.1
> 

Re: [PATCH v2 10/10] PCI/DOE: Relax restrictions on request and response size

[Lukas Wunner]

On Mon, Jan 23, 2023 at 05:43:53PM -0800, Ira Weiny wrote:
> Lukas Wunner wrote:
> > 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.
[...]
> > +	/* Write last payload dword */
> > +	remainder = task->request_pl_sz % sizeof(u32);
> > +	if (remainder) {
> > +		val = 0;
> > +		memcpy(&val, &task->request_pl[i], remainder);
> 
> Are there any issues with endianess here?

Indeed there were.  I've fixed that in v3.


> >  	/* First 2 dwords have already been read */
> >  	length -= 2;
> > -	payload_length = min(length, task->response_pl_sz / sizeof(u32));
> > -	/* 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]);
> > +	received = task->response_pl_sz;
> > +	payload_length = DIV_ROUND_UP(task->response_pl_sz, sizeof(u32));
> > +	remainder = task->response_pl_sz % sizeof(u32);
> > +	if (!remainder)
> > +		remainder = sizeof(u32);
> > +
> > +	if (length < payload_length) {
> > +		received = length * sizeof(u32);
> > +		payload_length = length;
> > +		remainder = sizeof(u32);
> 
> It was a bit confusing why remainder was set to a dword here.  But I got
> that it is because length and payload_length are both in dwords.

Here in pci_doe_recv_resp(), "remainder" signifies the number of
data bytes in the last payload dword.

If the response received via DOE is shorter than the buffer it is
written into, then the last payload dword contains 4 data bytes.
(DOE transmits full dwords.)

I've added a code comment in v3 to hopefully avoid any confusion:
/* remainder signifies number of data bytes in last payload dword */

Thanks,

Lukas

Re: [PATCH v2 10/10] PCI/DOE: Relax restrictions on request and response size

[Jonathan Cameron]

On Mon, 23 Jan 2023 11:20:00 +0100
Lukas Wunner <lukas@wunner.de> wrote:

> 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.
> 
> Tested-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Ah. This...

I can't immediately find the original discussion thread, but I'm fairly
sure we had a version of the DOE code that did this (maybe we just
discussed doing it and never had code...)

IIRC, at the time feedback was strongly in favour of making
the handling of non dword payloads a problem for the caller (e.g. PCI/CMA)
not the DOE core, mainly so that we could keep the layering simple.
DOE part of PCI spec says DWORD multiples only, CMA has non dword
entries.

Personally I'm fully in favour of making our lives easier and handling
this at the DOE layer!  The CMA padding code is nasty as we have to deal
with caching just the right bits of the payload for the running hashes.
With it at this layer I'd imagine that code gets much simpler

Assuming resolution to Ira's question on endianness is resolved.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/pci/doe.c | 66 ++++++++++++++++++++++++++++-------------------
>  1 file changed, 40 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> index 0263bcfdddd8..2113ec95379f 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(u32);
> +	length = 2 + DIV_ROUND_UP(task->request_pl_sz, sizeof(u32));
>  	if (length > PCI_DOE_MAX_LENGTH)
>  		return -EIO;
>  	if (length == PCI_DOE_MAX_LENGTH)
> @@ -184,10 +177,20 @@ 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(u32); i++)
>  		pci_write_config_dword(pdev, offset + PCI_DOE_WRITE,
>  				       task->request_pl[i]);
>  
> +	/* Write last payload dword */
> +	remainder = task->request_pl_sz % sizeof(u32);
> +	if (remainder) {
> +		val = 0;
> +		memcpy(&val, &task->request_pl[i], remainder);
> +		pci_write_config_dword(pdev, offset + PCI_DOE_WRITE, val);
> +	}
> +
>  	pci_doe_write_ctrl(doe_mb, PCI_DOE_CTRL_GO);
>  
>  	return 0;
> @@ -207,11 +210,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 +241,34 @@ 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));
> -	/* 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]);
> +	received = task->response_pl_sz;
> +	payload_length = DIV_ROUND_UP(task->response_pl_sz, sizeof(u32));
> +	remainder = task->response_pl_sz % sizeof(u32);
> +	if (!remainder)
> +		remainder = sizeof(u32);
> +
> +	if (length < payload_length) {
> +		received = length * sizeof(u32);
> +		payload_length = length;
> +		remainder = sizeof(u32);
> +	}
> +
> +	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,
> +					      &task->response_pl[i]);
> +			pci_write_config_dword(pdev, offset + PCI_DOE_READ, 0);
> +		}
> +
> +		/* Read last payload dword */
> +		pci_read_config_dword(pdev, offset + PCI_DOE_READ, &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 +282,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 received;
>  }
>  
>  static void signal_task_complete(struct pci_doe_task *task, int rv)
> @@ -560,14 +582,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(u32) ||
> -	    task->response_pl_sz < sizeof(u32))
> -		return -EINVAL;
> -
>  	if (test_bit(PCI_DOE_FLAG_DEAD, &doe_mb->flags))
>  		return -EIO;
>  

Re: [PATCH v2 10/10] PCI/DOE: Relax restrictions on request and response size

[Bjorn Helgaas]

On Tue, Jan 24, 2023 at 12:43:15PM +0000, Jonathan Cameron wrote:
> On Mon, 23 Jan 2023 11:20:00 +0100
> Lukas Wunner <lukas@wunner.de> wrote:
> 
> > 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.
> > 
> > Tested-by: Ira Weiny <ira.weiny@intel.com>
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Ah. This...
> 
> I can't immediately find the original discussion thread, but I'm fairly
> sure we had a version of the DOE code that did this (maybe we just
> discussed doing it and never had code...)
> 
> IIRC, at the time feedback was strongly in favour of making
> the handling of non dword payloads a problem for the caller (e.g. PCI/CMA)
> not the DOE core, mainly so that we could keep the layering simple.
> DOE part of PCI spec says DWORD multiples only, CMA has non dword
> entries.

I can't remember, but I might have been the voice in favor of making
it the caller's problem.  Your argument about dealing with it here
makes a lot of sense, and I'm OK with it, but I *would* like to add
some text to the commit log and comments in the code about what is
happening here.  Otherwise there's an unexplained disconnect between
the DWORD spec language and the byte-oriented code.

> Personally I'm fully in favour of making our lives easier and handling
> this at the DOE layer!  The CMA padding code is nasty as we have to deal
> with caching just the right bits of the payload for the running hashes.
> With it at this layer I'd imagine that code gets much simpler
> 
> Assuming resolution to Ira's question on endianness is resolved.
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH v2 10/10] PCI/DOE: Relax restrictions on request and response size

[Jonathan Cameron]

On Tue, 24 Jan 2023 17:51:55 -0600
Bjorn Helgaas <helgaas@kernel.org> wrote:

> On Tue, Jan 24, 2023 at 12:43:15PM +0000, Jonathan Cameron wrote:
> > On Mon, 23 Jan 2023 11:20:00 +0100
> > Lukas Wunner <lukas@wunner.de> wrote:
> >   
> > > 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.
> > > 
> > > Tested-by: Ira Weiny <ira.weiny@intel.com>
> > > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > > Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>  
> > Ah. This...
> > 
> > I can't immediately find the original discussion thread, but I'm fairly
> > sure we had a version of the DOE code that did this (maybe we just
> > discussed doing it and never had code...)
> > 
> > IIRC, at the time feedback was strongly in favour of making
> > the handling of non dword payloads a problem for the caller (e.g. PCI/CMA)
> > not the DOE core, mainly so that we could keep the layering simple.
> > DOE part of PCI spec says DWORD multiples only, CMA has non dword
> > entries.  
> 
> I can't remember, but I might have been the voice in favor of making
> it the caller's problem.  Your argument about dealing with it here
> makes a lot of sense, and I'm OK with it, but I *would* like to add
> some text to the commit log and comments in the code about what is
> happening here.  Otherwise there's an unexplained disconnect between
> the DWORD spec language and the byte-oriented code.

Absolutely agree. Calling out why we have a mismatch with the specification
will avoid a bunch of head scratching in the future!

> 
> > Personally I'm fully in favour of making our lives easier and handling
> > this at the DOE layer!  The CMA padding code is nasty as we have to deal
> > with caching just the right bits of the payload for the running hashes.
> > With it at this layer I'd imagine that code gets much simpler
> > 
> > Assuming resolution to Ira's question on endianness is resolved.
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  

Re: [PATCH v2 10/10] PCI/DOE: Relax restrictions on request and response size

[Lukas Wunner]

On Tue, Jan 24, 2023 at 05:51:55PM -0600, Bjorn Helgaas wrote:
> On Tue, Jan 24, 2023 at 12:43:15PM +0000, Jonathan Cameron wrote:
> > On Mon, 23 Jan 2023 11:20:00 +0100 Lukas Wunner <lukas@wunner.de> wrote:
> > > 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.
[...]
> > IIRC, at the time feedback was strongly in favour of making
> > the handling of non dword payloads a problem for the caller (e.g. PCI/CMA)
> > not the DOE core, mainly so that we could keep the layering simple.
> > DOE part of PCI spec says DWORD multiples only, CMA has non dword
> > entries.
> 
> I can't remember, but I might have been the voice in favor of making
> it the caller's problem.  Your argument about dealing with it here
> makes a lot of sense, and I'm OK with it, but I *would* like to add
> some text to the commit log and comments in the code about what is
> happening here.  Otherwise there's an unexplained disconnect between
> the DWORD spec language and the byte-oriented code.

In v3 I amended both the commit message and the kerneldoc for pci_doe()
to make it clear that support for arbitrary-sized request and response
buffers is not stipulated by the spec, but merely for the convenience
of the caller.

Thanks,

Lukas

Re: [PATCH v2 00/10] Collection of DOE material

[Bjorn Helgaas]

On Mon, Jan 23, 2023 at 11:10:00AM +0100, Lukas Wunner wrote:
> Collection of DOE material, v2:
> 
> * Fix WARN splat reported by Gregory Price
> * Migrate to synchronous API
> * Create DOE mailboxes in PCI core instead of in drivers
> * No longer require request and response size to be multiple of 4 bytes
> 
> This is in preparation for CMA device authentication (PCIe r6.0, sec 6.31),
> which performs DOE exchanges of irregular size and is going to be handled
> in the PCI core.  The synchronous API reduces code size for DOE users.
> 
> Link to CMA development branch:
> https://github.com/l1k/linux/commits/doe
> 
> 
> Changes v1 -> v2:
> * [PATCH v2 01/10] PCI/DOE: Silence WARN splat with CONFIG_DEBUG_OBJECTS=y 
>   * Add note in kernel-doc of pci_doe_submit_task() that pci_doe_task must
>     be allocated on the stack (Jonathan)
> * [PATCH v2 05/10] PCI/DOE: Make asynchronous API private
>   * Deduplicate note in kernel-doc of struct pci_doe_task that caller need
>     not initialize certain fields (Jonathan)
> 
> Link to v1:
> https://lore.kernel.org/linux-pci/cover.1669608950.git.lukas@wunner.de/
> 
> 
> Lukas Wunner (10):
>   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: 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

Looks nice.  Do you envision getting acks from the CXL folks and
merging via the PCI tree, or the reverse?

Bjorn

Re: [PATCH v2 00/10] Collection of DOE material

[Lukas Wunner]

On Mon, Jan 23, 2023 at 04:30:53PM -0600, Bjorn Helgaas wrote:
> On Mon, Jan 23, 2023 at 11:10:00AM +0100, Lukas Wunner wrote:
> > Collection of DOE material, v2:
> 
> Do you envision getting acks from the CXL folks and
> merging via the PCI tree, or the reverse?

I do not have a strong preference myself.

I've just submitted v3 and based that on pci/next.

The patches apply equally well to cxl/next, save for a trivial conflict
in patch [13/16] due to a context change in drivers/cxl/cxlmem.h.
(I'm removing "struct xarray doe_mbs" and an adjacent new line was
added in cxl/next for "struct cxl_event_state event".)

I recognize that it might be too late to squeeze the full series into 6.3.
However, the first 6 patches are fixes tagged for stable, so at least
those should still be fine for 6.3.

I believe Dave Jiang is basing his in-kernel CDAT parsing on this series,
hence needs it merged during the next cycle.  So if you do not want to
apply the full series for 6.3, then the last 10 patches should probably
be picked up by Dan for 6.4 early during the next cycle in support of
Dave.

All of those ruminations are moot of couse if there are objections
requiring another respin.

Dan, what do you think?

Thanks,

Lukas

Re: [PATCH v2 00/10] Collection of DOE material

[Dan Williams]

Lukas Wunner wrote:
> On Mon, Jan 23, 2023 at 04:30:53PM -0600, Bjorn Helgaas wrote:
> > On Mon, Jan 23, 2023 at 11:10:00AM +0100, Lukas Wunner wrote:
> > > Collection of DOE material, v2:
> > 
> > Do you envision getting acks from the CXL folks and
> > merging via the PCI tree, or the reverse?
> 
> I do not have a strong preference myself.
> 
> I've just submitted v3 and based that on pci/next.
> 
> The patches apply equally well to cxl/next, save for a trivial conflict
> in patch [13/16] due to a context change in drivers/cxl/cxlmem.h.
> (I'm removing "struct xarray doe_mbs" and an adjacent new line was
> added in cxl/next for "struct cxl_event_state event".)
> 
> I recognize that it might be too late to squeeze the full series into 6.3.
> However, the first 6 patches are fixes tagged for stable, so at least
> those should still be fine for 6.3.
> 
> I believe Dave Jiang is basing his in-kernel CDAT parsing on this series,
> hence needs it merged during the next cycle.  So if you do not want to
> apply the full series for 6.3, then the last 10 patches should probably
> be picked up by Dan for 6.4 early during the next cycle in support of
> Dave.
> 
> All of those ruminations are moot of couse if there are objections
> requiring another respin.
> 
> Dan, what do you think?

I am ok for this to go through PCI. I will go review v3.