[PATCH V3 0/2] PCI/DOE: Remove asynchronous task support

[PATCH V3 0/2] PCI/DOE: Remove asynchronous task support

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

Changes from V2
	From Jonathan
		Add comment about change in PCI_DOE_POLL_INTERVAL value
		Remove additional code

Changes from V1
	Address comments around locking
	Add patch to remove unneeded pci_doe_flush_mb() call

The asynchronous task support in the PCI DOE mailboxes has become a
maintenance burden.

Remove the code associated with that support.

Ira Weiny (2):
  PCI/DOE: Remove the pci_doe_flush_mb() call
  PCI/DOE: Remove asynchronous task support

 drivers/cxl/core/pci.c  |  16 +----
 drivers/pci/doe.c       | 135 ++++++++++------------------------------
 include/linux/pci-doe.h |  11 +---
 3 files changed, 36 insertions(+), 126 deletions(-)


base-commit: b6e7fdfd6f6a8bf88fcdb4a45da52c42ba238c25
-- 
2.37.2

[PATCH V3 1/2] PCI/DOE: Remove the pci_doe_flush_mb() call

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

pci_doe_flush_mb() does not work and is currently unused.

It does not work because each struct doe_mb is managed as part of the
PCI device.  They can't go away as long as the PCI device exists.
pci_doe_flush_mb() was set up to flush the workqueue and prevent any
further submissions to the mailboxes when the PCI device goes away.
Unfortunately, this was fundamentally flawed.  There was no guarantee
that a struct doe_mb remained after pci_doe_flush_mb() returned.
Therefore, the doe_mb state could be invalid when those threads waiting
on the workqueue were flushed.

Fortunately the current code is safe because all callers make a
synchronous call to pci_doe_submit_task() and maintain a reference on
the PCI device.  Therefore pci_doe_flush_mb() is effectively unused.

Rather than attempt to fix pci_doe_flush_mb() just remove the dead code
around pci_doe_flush_mb().

Cc: Lukas Wunner <lukas@wunner.de>
Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Changes from V2:
	Lukas
		Clarify commit message.
	Jonathan
		Add comment for changed poll interval.
---
 drivers/pci/doe.c | 49 +++++------------------------------------------
 1 file changed, 5 insertions(+), 44 deletions(-)

diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
index e402f05068a5..685e7d26c7eb 100644
--- a/drivers/pci/doe.c
+++ b/drivers/pci/doe.c
@@ -24,10 +24,10 @@
 
 /* Timeout of 1 second from 6.30.2 Operation, PCI Spec r6.0 */
 #define PCI_DOE_TIMEOUT HZ
-#define PCI_DOE_POLL_INTERVAL	(PCI_DOE_TIMEOUT / 128)
+/* Interval to poll mailbox status */
+#define PCI_DOE_POLL_INTERVAL_MSECS	8
 
-#define PCI_DOE_FLAG_CANCEL	0
-#define PCI_DOE_FLAG_DEAD	1
+#define PCI_DOE_FLAG_DEAD	0
 
 /**
  * struct pci_doe_mb - State for a single DOE mailbox
@@ -53,15 +53,6 @@ struct pci_doe_mb {
 	unsigned long flags;
 };
 
-static int pci_doe_wait(struct pci_doe_mb *doe_mb, unsigned long timeout)
-{
-	if (wait_event_timeout(doe_mb->wq,
-			       test_bit(PCI_DOE_FLAG_CANCEL, &doe_mb->flags),
-			       timeout))
-		return -EIO;
-	return 0;
-}
-
 static void pci_doe_write_ctrl(struct pci_doe_mb *doe_mb, u32 val)
 {
 	struct pci_dev *pdev = doe_mb->pdev;
@@ -82,12 +73,9 @@ static int pci_doe_abort(struct pci_doe_mb *doe_mb)
 	pci_doe_write_ctrl(doe_mb, PCI_DOE_CTRL_ABORT);
 
 	do {
-		int rc;
 		u32 val;
 
-		rc = pci_doe_wait(doe_mb, PCI_DOE_POLL_INTERVAL);
-		if (rc)
-			return rc;
+		msleep_interruptible(PCI_DOE_POLL_INTERVAL_MSECS);
 		pci_read_config_dword(pdev, offset + PCI_DOE_STATUS, &val);
 
 		/* Abort success! */
@@ -278,11 +266,7 @@ static void doe_statemachine_work(struct work_struct *work)
 			signal_task_abort(task, -EIO);
 			return;
 		}
-		rc = pci_doe_wait(doe_mb, PCI_DOE_POLL_INTERVAL);
-		if (rc) {
-			signal_task_abort(task, rc);
-			return;
-		}
+		msleep_interruptible(PCI_DOE_POLL_INTERVAL_MSECS);
 		goto retry_resp;
 	}
 
@@ -383,21 +367,6 @@ static void pci_doe_destroy_workqueue(void *mb)
 	destroy_workqueue(doe_mb->work_queue);
 }
 
-static void pci_doe_flush_mb(void *mb)
-{
-	struct pci_doe_mb *doe_mb = mb;
-
-	/* Stop all pending work items from starting */
-	set_bit(PCI_DOE_FLAG_DEAD, &doe_mb->flags);
-
-	/* Cancel an in progress work item, if necessary */
-	set_bit(PCI_DOE_FLAG_CANCEL, &doe_mb->flags);
-	wake_up(&doe_mb->wq);
-
-	/* Flush all work items */
-	flush_workqueue(doe_mb->work_queue);
-}
-
 /**
  * pcim_doe_create_mb() - Create a DOE mailbox object
  *
@@ -450,14 +419,6 @@ struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset)
 		return ERR_PTR(rc);
 	}
 
-	/*
-	 * The state machine and the mailbox should be in sync now;
-	 * Set up mailbox flush prior to using 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",
-- 
2.37.2

Re: [PATCH V3 1/2] PCI/DOE: Remove the pci_doe_flush_mb() call

[Li, Ming]

On 11/28/2022 12:03 PM, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> pci_doe_flush_mb() does not work and is currently unused.
> 
> It does not work because each struct doe_mb is managed as part of the
> PCI device.  They can't go away as long as the PCI device exists.
> pci_doe_flush_mb() was set up to flush the workqueue and prevent any
> further submissions to the mailboxes when the PCI device goes away.
> Unfortunately, this was fundamentally flawed.  There was no guarantee
> that a struct doe_mb remained after pci_doe_flush_mb() returned.
> Therefore, the doe_mb state could be invalid when those threads waiting
> on the workqueue were flushed.
> 
> Fortunately the current code is safe because all callers make a
> synchronous call to pci_doe_submit_task() and maintain a reference on
> the PCI device.  Therefore pci_doe_flush_mb() is effectively unused.
> 
> Rather than attempt to fix pci_doe_flush_mb() just remove the dead code
> around pci_doe_flush_mb().
> 
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

Some comments inline.

> 
> ---
> Changes from V2:
> 	Lukas
> 		Clarify commit message.
> 	Jonathan
> 		Add comment for changed poll interval.

...

>  
> -static int pci_doe_wait(struct pci_doe_mb *doe_mb, unsigned long timeout)
> -{
> -	if (wait_event_timeout(doe_mb->wq,
> -			       test_bit(PCI_DOE_FLAG_CANCEL, &doe_mb->flags),
> -			       timeout))
> -		return -EIO;
> -	return 0;
> -}
> -
>  static void pci_doe_write_ctrl(struct pci_doe_mb *doe_mb, u32 val)
>  {
>  	struct pci_dev *pdev = doe_mb->pdev;
> @@ -82,12 +73,9 @@ static int pci_doe_abort(struct pci_doe_mb *doe_mb)
>  	pci_doe_write_ctrl(doe_mb, PCI_DOE_CTRL_ABORT);
>  
>  	do {
> -		int rc;
>  		u32 val;
>  
> -		rc = pci_doe_wait(doe_mb, PCI_DOE_POLL_INTERVAL);
> -		if (rc)
> -			return rc;
> +		msleep_interruptible(PCI_DOE_POLL_INTERVAL_MSECS);
>  		pci_read_config_dword(pdev, offset + PCI_DOE_STATUS, &val);

Looks like we don't have to use msleep_interruptible() here, can use msleep() directly?

>  
>  		/* Abort success! */
> @@ -278,11 +266,7 @@ static void doe_statemachine_work(struct work_struct *work)
>  			signal_task_abort(task, -EIO);
>  			return;
>  		}
> -		rc = pci_doe_wait(doe_mb, PCI_DOE_POLL_INTERVAL);
> -		if (rc) {
> -			signal_task_abort(task, rc);
> -			return;
> -		}
> +		msleep_interruptible(PCI_DOE_POLL_INTERVAL_MSECS);
>  		goto retry_resp;
>  	}

I guess that you use msleep_interruptible() here for aborting current task when signals come.
So there should be signal_task_abort() and return when msleep_interruptible() receives a signal.

Thanks
Ming

>  
> @@ -383,21 +367,6 @@ static void pci_doe_destroy_workqueue(void *mb)
>  	destroy_workqueue(doe_mb->work_queue);
>  }
>  
> -static void pci_doe_flush_mb(void *mb)
> -{
> -	struct pci_doe_mb *doe_mb = mb;
> -
> -	/* Stop all pending work items from starting */
> -	set_bit(PCI_DOE_FLAG_DEAD, &doe_mb->flags);
> -
> -	/* Cancel an in progress work item, if necessary */
> -	set_bit(PCI_DOE_FLAG_CANCEL, &doe_mb->flags);
> -	wake_up(&doe_mb->wq);
> -
> -	/* Flush all work items */
> -	flush_workqueue(doe_mb->work_queue);
> -}
> -
>  /**
>   * pcim_doe_create_mb() - Create a DOE mailbox object
>   *
> @@ -450,14 +419,6 @@ struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset)
>  		return ERR_PTR(rc);
>  	}
>  
> -	/*
> -	 * The state machine and the mailbox should be in sync now;
> -	 * Set up mailbox flush prior to using 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",

Re: [PATCH V3 1/2] PCI/DOE: Remove the pci_doe_flush_mb() call

[Ira Weiny]

On Mon, Nov 28, 2022 at 01:51:24PM +0800, Li, Ming wrote:
> 
> On 11/28/2022 12:03 PM, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > pci_doe_flush_mb() does not work and is currently unused.
> > 
> > It does not work because each struct doe_mb is managed as part of the
> > PCI device.  They can't go away as long as the PCI device exists.
> > pci_doe_flush_mb() was set up to flush the workqueue and prevent any
> > further submissions to the mailboxes when the PCI device goes away.
> > Unfortunately, this was fundamentally flawed.  There was no guarantee
> > that a struct doe_mb remained after pci_doe_flush_mb() returned.
> > Therefore, the doe_mb state could be invalid when those threads waiting
> > on the workqueue were flushed.
> > 
> > Fortunately the current code is safe because all callers make a
> > synchronous call to pci_doe_submit_task() and maintain a reference on
> > the PCI device.  Therefore pci_doe_flush_mb() is effectively unused.
> > 
> > Rather than attempt to fix pci_doe_flush_mb() just remove the dead code
> > around pci_doe_flush_mb().
> > 
> > Cc: Lukas Wunner <lukas@wunner.de>
> > Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> Some comments inline.
> 
> > 
> > ---
> > Changes from V2:
> > 	Lukas
> > 		Clarify commit message.
> > 	Jonathan
> > 		Add comment for changed poll interval.
> 
> ...
> 
> >  
> > -static int pci_doe_wait(struct pci_doe_mb *doe_mb, unsigned long timeout)
> > -{
> > -	if (wait_event_timeout(doe_mb->wq,
> > -			       test_bit(PCI_DOE_FLAG_CANCEL, &doe_mb->flags),
> > -			       timeout))
> > -		return -EIO;
> > -	return 0;
> > -}
> > -
> >  static void pci_doe_write_ctrl(struct pci_doe_mb *doe_mb, u32 val)
> >  {
> >  	struct pci_dev *pdev = doe_mb->pdev;
> > @@ -82,12 +73,9 @@ static int pci_doe_abort(struct pci_doe_mb *doe_mb)
> >  	pci_doe_write_ctrl(doe_mb, PCI_DOE_CTRL_ABORT);
> >  
> >  	do {
> > -		int rc;
> >  		u32 val;
> >  
> > -		rc = pci_doe_wait(doe_mb, PCI_DOE_POLL_INTERVAL);
> > -		if (rc)
> > -			return rc;
> > +		msleep_interruptible(PCI_DOE_POLL_INTERVAL_MSECS);
> >  		pci_read_config_dword(pdev, offset + PCI_DOE_STATUS, &val);
> 
> Looks like we don't have to use msleep_interruptible() here, can use msleep() directly?

I don't know.  I think your suggestion below holds here too.  Unfortunately
that could mask the signal received in the case below.  But I think that is
going to be rare enough we could ignore it.

> 
> >  
> >  		/* Abort success! */
> > @@ -278,11 +266,7 @@ static void doe_statemachine_work(struct work_struct *work)
> >  			signal_task_abort(task, -EIO);
> >  			return;
> >  		}
> > -		rc = pci_doe_wait(doe_mb, PCI_DOE_POLL_INTERVAL);
> > -		if (rc) {
> > -			signal_task_abort(task, rc);
> > -			return;
> > -		}
> > +		msleep_interruptible(PCI_DOE_POLL_INTERVAL_MSECS);
> >  		goto retry_resp;
> >  	}
> 
> I guess that you use msleep_interruptible() here for aborting current task when signals come.
> So there should be signal_task_abort() and return when msleep_interruptible() receives a signal.

Yes this makes much more sense.  Thanks for looking!
Ira

> 
> Thanks
> Ming
> 
> >  
> > @@ -383,21 +367,6 @@ static void pci_doe_destroy_workqueue(void *mb)
> >  	destroy_workqueue(doe_mb->work_queue);
> >  }
> >  
> > -static void pci_doe_flush_mb(void *mb)
> > -{
> > -	struct pci_doe_mb *doe_mb = mb;
> > -
> > -	/* Stop all pending work items from starting */
> > -	set_bit(PCI_DOE_FLAG_DEAD, &doe_mb->flags);
> > -
> > -	/* Cancel an in progress work item, if necessary */
> > -	set_bit(PCI_DOE_FLAG_CANCEL, &doe_mb->flags);
> > -	wake_up(&doe_mb->wq);
> > -
> > -	/* Flush all work items */
> > -	flush_workqueue(doe_mb->work_queue);
> > -}
> > -
> >  /**
> >   * pcim_doe_create_mb() - Create a DOE mailbox object
> >   *
> > @@ -450,14 +419,6 @@ struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset)
> >  		return ERR_PTR(rc);
> >  	}
> >  
> > -	/*
> > -	 * The state machine and the mailbox should be in sync now;
> > -	 * Set up mailbox flush prior to using 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",
> 

Re: [PATCH V3 1/2] PCI/DOE: Remove the pci_doe_flush_mb() call

[Alison Schofield]

On Sun, Nov 27, 2022 at 08:03:37PM -0800, Ira Weiny wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> pci_doe_flush_mb() does not work and is currently unused.
> 
> It does not work because each struct doe_mb is managed as part of the
> PCI device.  They can't go away as long as the PCI device exists.
> pci_doe_flush_mb() was set up to flush the workqueue and prevent any
> further submissions to the mailboxes when the PCI device goes away.
> Unfortunately, this was fundamentally flawed.  There was no guarantee
> that a struct doe_mb remained after pci_doe_flush_mb() returned.
> Therefore, the doe_mb state could be invalid when those threads waiting
> on the workqueue were flushed.
> 
> Fortunately the current code is safe because all callers make a
> synchronous call to pci_doe_submit_task() and maintain a reference on
> the PCI device.  Therefore pci_doe_flush_mb() is effectively unused.
> 
> Rather than attempt to fix pci_doe_flush_mb() just remove the dead code
> around pci_doe_flush_mb().

The commit message says "Remove ...." and the commit log only
talks about removing code, yet an msleep() is added.
Can those be clearer?

> 
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> ---
> Changes from V2:
> 	Lukas
> 		Clarify commit message.
> 	Jonathan
> 		Add comment for changed poll interval.
> ---
>  drivers/pci/doe.c | 49 +++++------------------------------------------
>  1 file changed, 5 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> index e402f05068a5..685e7d26c7eb 100644
> --- a/drivers/pci/doe.c
> +++ b/drivers/pci/doe.c
> @@ -24,10 +24,10 @@
>  
>  /* Timeout of 1 second from 6.30.2 Operation, PCI Spec r6.0 */
>  #define PCI_DOE_TIMEOUT HZ
> -#define PCI_DOE_POLL_INTERVAL	(PCI_DOE_TIMEOUT / 128)
> +/* Interval to poll mailbox status */
> +#define PCI_DOE_POLL_INTERVAL_MSECS	8
>  
> -#define PCI_DOE_FLAG_CANCEL	0
> -#define PCI_DOE_FLAG_DEAD	1
> +#define PCI_DOE_FLAG_DEAD	0
>  
>  /**
>   * struct pci_doe_mb - State for a single DOE mailbox
> @@ -53,15 +53,6 @@ struct pci_doe_mb {
>  	unsigned long flags;
>  };
>  
> -static int pci_doe_wait(struct pci_doe_mb *doe_mb, unsigned long timeout)
> -{
> -	if (wait_event_timeout(doe_mb->wq,
> -			       test_bit(PCI_DOE_FLAG_CANCEL, &doe_mb->flags),
> -			       timeout))
> -		return -EIO;
> -	return 0;
> -}
> -
>  static void pci_doe_write_ctrl(struct pci_doe_mb *doe_mb, u32 val)
>  {
>  	struct pci_dev *pdev = doe_mb->pdev;
> @@ -82,12 +73,9 @@ static int pci_doe_abort(struct pci_doe_mb *doe_mb)
>  	pci_doe_write_ctrl(doe_mb, PCI_DOE_CTRL_ABORT);
>  
>  	do {
> -		int rc;
>  		u32 val;
>  
> -		rc = pci_doe_wait(doe_mb, PCI_DOE_POLL_INTERVAL);
> -		if (rc)
> -			return rc;
> +		msleep_interruptible(PCI_DOE_POLL_INTERVAL_MSECS);
>  		pci_read_config_dword(pdev, offset + PCI_DOE_STATUS, &val);
>  
>  		/* Abort success! */
> @@ -278,11 +266,7 @@ static void doe_statemachine_work(struct work_struct *work)
>  			signal_task_abort(task, -EIO);
>  			return;
>  		}
> -		rc = pci_doe_wait(doe_mb, PCI_DOE_POLL_INTERVAL);
> -		if (rc) {
> -			signal_task_abort(task, rc);
> -			return;
> -		}
> +		msleep_interruptible(PCI_DOE_POLL_INTERVAL_MSECS);
>  		goto retry_resp;
>  	}
>  
> @@ -383,21 +367,6 @@ static void pci_doe_destroy_workqueue(void *mb)
>  	destroy_workqueue(doe_mb->work_queue);
>  }
>  
> -static void pci_doe_flush_mb(void *mb)
> -{
> -	struct pci_doe_mb *doe_mb = mb;
> -
> -	/* Stop all pending work items from starting */
> -	set_bit(PCI_DOE_FLAG_DEAD, &doe_mb->flags);
> -
> -	/* Cancel an in progress work item, if necessary */
> -	set_bit(PCI_DOE_FLAG_CANCEL, &doe_mb->flags);
> -	wake_up(&doe_mb->wq);
> -
> -	/* Flush all work items */
> -	flush_workqueue(doe_mb->work_queue);
> -}
> -
>  /**
>   * pcim_doe_create_mb() - Create a DOE mailbox object
>   *
> @@ -450,14 +419,6 @@ struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset)
>  		return ERR_PTR(rc);
>  	}
>  
> -	/*
> -	 * The state machine and the mailbox should be in sync now;
> -	 * Set up mailbox flush prior to using 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",
> -- 
> 2.37.2
> 

Re: [PATCH V3 1/2] PCI/DOE: Remove the pci_doe_flush_mb() call

[Ira Weiny]

On Mon, Nov 28, 2022 at 09:51:47AM -0800, Alison Schofield wrote:
> On Sun, Nov 27, 2022 at 08:03:37PM -0800, Ira Weiny wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > pci_doe_flush_mb() does not work and is currently unused.
> > 
> > It does not work because each struct doe_mb is managed as part of the
> > PCI device.  They can't go away as long as the PCI device exists.
> > pci_doe_flush_mb() was set up to flush the workqueue and prevent any
> > further submissions to the mailboxes when the PCI device goes away.
> > Unfortunately, this was fundamentally flawed.  There was no guarantee
> > that a struct doe_mb remained after pci_doe_flush_mb() returned.
> > Therefore, the doe_mb state could be invalid when those threads waiting
> > on the workqueue were flushed.
> > 
> > Fortunately the current code is safe because all callers make a
> > synchronous call to pci_doe_submit_task() and maintain a reference on
> > the PCI device.  Therefore pci_doe_flush_mb() is effectively unused.
> > 
> > Rather than attempt to fix pci_doe_flush_mb() just remove the dead code
> > around pci_doe_flush_mb().
> 
> The commit message says "Remove ...." and the commit log only
> talks about removing code, yet an msleep() is added.
> Can those be clearer?

:-/

I'm struggling a bit with this comment.  The patch focus is on removing an
unneeded and effectively unused function.  (It is called but not in a way that
would cause any running task to be aborted.)  The replacement of pci_doe_wait()
with msleep_interruptible() is a side effect and I don't think belongs in the
one liner.

I suppose something like this might work?

PCI/DOE: Replace broken task cancellation with msleep

I guess that makes some sense.  Combined with Ming's suggestion we are still
allowing task cancellation but from a signal rather than PCI device removal.

Ira

> 
> > 
> > Cc: Lukas Wunner <lukas@wunner.de>
> > Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > 
> > ---
> > Changes from V2:
> > 	Lukas
> > 		Clarify commit message.
> > 	Jonathan
> > 		Add comment for changed poll interval.
> > ---
> >  drivers/pci/doe.c | 49 +++++------------------------------------------
> >  1 file changed, 5 insertions(+), 44 deletions(-)
> > 
> > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> > index e402f05068a5..685e7d26c7eb 100644
> > --- a/drivers/pci/doe.c
> > +++ b/drivers/pci/doe.c
> > @@ -24,10 +24,10 @@
> >  
> >  /* Timeout of 1 second from 6.30.2 Operation, PCI Spec r6.0 */
> >  #define PCI_DOE_TIMEOUT HZ
> > -#define PCI_DOE_POLL_INTERVAL	(PCI_DOE_TIMEOUT / 128)
> > +/* Interval to poll mailbox status */
> > +#define PCI_DOE_POLL_INTERVAL_MSECS	8
> >  
> > -#define PCI_DOE_FLAG_CANCEL	0
> > -#define PCI_DOE_FLAG_DEAD	1
> > +#define PCI_DOE_FLAG_DEAD	0
> >  
> >  /**
> >   * struct pci_doe_mb - State for a single DOE mailbox
> > @@ -53,15 +53,6 @@ struct pci_doe_mb {
> >  	unsigned long flags;
> >  };
> >  
> > -static int pci_doe_wait(struct pci_doe_mb *doe_mb, unsigned long timeout)
> > -{
> > -	if (wait_event_timeout(doe_mb->wq,
> > -			       test_bit(PCI_DOE_FLAG_CANCEL, &doe_mb->flags),
> > -			       timeout))
> > -		return -EIO;
> > -	return 0;
> > -}
> > -
> >  static void pci_doe_write_ctrl(struct pci_doe_mb *doe_mb, u32 val)
> >  {
> >  	struct pci_dev *pdev = doe_mb->pdev;
> > @@ -82,12 +73,9 @@ static int pci_doe_abort(struct pci_doe_mb *doe_mb)
> >  	pci_doe_write_ctrl(doe_mb, PCI_DOE_CTRL_ABORT);
> >  
> >  	do {
> > -		int rc;
> >  		u32 val;
> >  
> > -		rc = pci_doe_wait(doe_mb, PCI_DOE_POLL_INTERVAL);
> > -		if (rc)
> > -			return rc;
> > +		msleep_interruptible(PCI_DOE_POLL_INTERVAL_MSECS);
> >  		pci_read_config_dword(pdev, offset + PCI_DOE_STATUS, &val);
> >  
> >  		/* Abort success! */
> > @@ -278,11 +266,7 @@ static void doe_statemachine_work(struct work_struct *work)
> >  			signal_task_abort(task, -EIO);
> >  			return;
> >  		}
> > -		rc = pci_doe_wait(doe_mb, PCI_DOE_POLL_INTERVAL);
> > -		if (rc) {
> > -			signal_task_abort(task, rc);
> > -			return;
> > -		}
> > +		msleep_interruptible(PCI_DOE_POLL_INTERVAL_MSECS);
> >  		goto retry_resp;
> >  	}
> >  
> > @@ -383,21 +367,6 @@ static void pci_doe_destroy_workqueue(void *mb)
> >  	destroy_workqueue(doe_mb->work_queue);
> >  }
> >  
> > -static void pci_doe_flush_mb(void *mb)
> > -{
> > -	struct pci_doe_mb *doe_mb = mb;
> > -
> > -	/* Stop all pending work items from starting */
> > -	set_bit(PCI_DOE_FLAG_DEAD, &doe_mb->flags);
> > -
> > -	/* Cancel an in progress work item, if necessary */
> > -	set_bit(PCI_DOE_FLAG_CANCEL, &doe_mb->flags);
> > -	wake_up(&doe_mb->wq);
> > -
> > -	/* Flush all work items */
> > -	flush_workqueue(doe_mb->work_queue);
> > -}
> > -
> >  /**
> >   * pcim_doe_create_mb() - Create a DOE mailbox object
> >   *
> > @@ -450,14 +419,6 @@ struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset)
> >  		return ERR_PTR(rc);
> >  	}
> >  
> > -	/*
> > -	 * The state machine and the mailbox should be in sync now;
> > -	 * Set up mailbox flush prior to using 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",
> > -- 
> > 2.37.2
> > 

[PATCH V3 2/2] PCI/DOE: Remove asynchronous task support

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

Gregory Price and Jonathan Cameron reported a bug within
pci_doe_submit_task().[1]  The issue was that work item initialization
needs to be done with either INIT_WORK_ONSTACK() or INIT_WORK()
depending on how the work item is allocated.

Initially, it was anticipated that DOE tasks were going to need to be
submitted asynchronously and the code was designed thusly.  Many
alternatives were discussed to fix the work initialization issue.[2]

However, all current users submit tasks synchronously and this has
therefore become an unneeded maintenance burden.  Remove the extra
maintenance burden by replacing asynchronous task submission with
a synchronous wait function.[3]

[1] https://lore.kernel.org/linux-cxl/20221014151045.24781-1-Jonathan.Cameron@huawei.com/T/#m88a7f50dcce52f30c8bf5c3dcc06fa9843b54a2d
[2] https://lore.kernel.org/linux-cxl/Y3kSDQDur+IUDs2O@iweiny-mobl/T/#m0f057773d9c75432fcfcc54a2604483fe82abe92
[3] https://lore.kernel.org/linux-cxl/Y3kSDQDur+IUDs2O@iweiny-mobl/T/#m32d3f9b208ef7486bc148d94a326b26b2d3e69ff

Reported-by: Gregory Price <gregory.price@memverge.com>
Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Suggested-by: Dan Williams <dan.j.williams@intel.com>
Suggested-by: "Li, Ming" <ming4.li@intel.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Changes from V2:
	Jonathan
		Fix comment on struct pci_doe_mb
		Remove signal_task_complete()

Changes from V1:
	Dan Williams:
		Use mutex_lock_interruptible()
		Adjust comment to lock the doe_mb structure not the
		code
	Adjust the locking based on the new series which eliminated
	PCI_DOE_FLAG_CANCEL.

Thanks to Dan for the bulk of the patch.
Thanks to Ming for pointing out the need for a lock to prevent more
than 1 task from being processed at a time.
---
 drivers/cxl/core/pci.c  | 16 ++------
 drivers/pci/doe.c       | 86 +++++++++++++----------------------------
 include/linux/pci-doe.h | 11 +-----
 3 files changed, 31 insertions(+), 82 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 9240df53ed87..58977e0712b6 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -490,21 +490,14 @@ 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,        \
@@ -513,8 +506,6 @@ struct cdat_doe_task cdt = {                                  \
 		.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,                            \
 	}                                                     \
 }
 
@@ -525,12 +516,12 @@ static int cxl_cdat_get_length(struct device *dev,
 	DECLARE_CDAT_DOE_TASK(CDAT_DOE_REQ(0), t);
 	int rc;
 
-	rc = pci_doe_submit_task(cdat_doe, &t.task);
+	rc = pci_doe_submit_task_wait(cdat_doe, &t.task);
 	if (rc < 0) {
 		dev_err(dev, "DOE submit failed: %d", rc);
 		return rc;
 	}
-	wait_for_completion(&t.c);
+
 	if (t.task.rv < sizeof(u32))
 		return -EIO;
 
@@ -554,12 +545,11 @@ static int cxl_cdat_read_table(struct device *dev,
 		u32 *entry;
 		int rc;
 
-		rc = pci_doe_submit_task(cdat_doe, &t.task);
+		rc = pci_doe_submit_task_wait(cdat_doe, &t.task);
 		if (rc < 0) {
 			dev_err(dev, "DOE submit failed: %d", rc);
 			return rc;
 		}
-		wait_for_completion(&t.c);
 		/* 1 DW header + 1 DW data min */
 		if (t.task.rv < (2 * sizeof(u32)))
 			return -EIO;
diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
index 685e7d26c7eb..b399f0f4fb5d 100644
--- a/drivers/pci/doe.c
+++ b/drivers/pci/doe.c
@@ -18,7 +18,6 @@
 #include <linux/mutex.h>
 #include <linux/pci.h>
 #include <linux/pci-doe.h>
-#include <linux/workqueue.h>
 
 #define PCI_DOE_PROTOCOL_DISCOVERY 0
 
@@ -40,7 +39,7 @@
  * @cap_offset: Capability offset
  * @prots: Array of protocols supported (encoded as long values)
  * @wq: Wait queue for work item
- * @work_queue: Queue of pci_doe_work items
+ * @lock: Lock state of doe_mb during task processing
  * @flags: Bit array of PCI_DOE_FLAG_* flags
  */
 struct pci_doe_mb {
@@ -49,7 +48,7 @@ struct pci_doe_mb {
 	struct xarray prots;
 
 	wait_queue_head_t wq;
-	struct workqueue_struct *work_queue;
+	struct mutex lock;
 	unsigned long flags;
 };
 
@@ -196,12 +195,6 @@ static int pci_doe_recv_resp(struct pci_doe_mb *doe_mb, struct pci_doe_task *tas
 	return min(length, task->response_pl_sz / sizeof(u32)) * sizeof(u32);
 }
 
-static void signal_task_complete(struct pci_doe_task *task, int rv)
-{
-	task->rv = rv;
-	task->complete(task);
-}
-
 static void signal_task_abort(struct pci_doe_task *task, int rv)
 {
 	struct pci_doe_mb *doe_mb = task->doe_mb;
@@ -216,13 +209,11 @@ static void signal_task_abort(struct pci_doe_task *task, int rv)
 			doe_mb->cap_offset);
 		set_bit(PCI_DOE_FLAG_DEAD, &doe_mb->flags);
 	}
-	signal_task_complete(task, rv);
+	task->rv = rv;
 }
 
-static void doe_statemachine_work(struct work_struct *work)
+static void exec_task(struct pci_doe_task *task)
 {
-	struct pci_doe_task *task = container_of(work, struct pci_doe_task,
-						 work);
 	struct pci_doe_mb *doe_mb = task->doe_mb;
 	struct pci_dev *pdev = doe_mb->pdev;
 	int offset = doe_mb->cap_offset;
@@ -231,7 +222,7 @@ static void doe_statemachine_work(struct work_struct *work)
 	int rc;
 
 	if (test_bit(PCI_DOE_FLAG_DEAD, &doe_mb->flags)) {
-		signal_task_complete(task, -EIO);
+		task->rv = -EIO;
 		return;
 	}
 
@@ -276,12 +267,7 @@ static void doe_statemachine_work(struct work_struct *work)
 		return;
 	}
 
-	signal_task_complete(task, rc);
-}
-
-static void pci_doe_task_complete(struct pci_doe_task *task)
-{
-	complete(task->private);
+	task->rv = rc;
 }
 
 static int pci_doe_discovery(struct pci_doe_mb *doe_mb, u8 *index, u16 *vid,
@@ -290,7 +276,6 @@ 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,
@@ -298,17 +283,13 @@ static int pci_doe_discovery(struct pci_doe_mb *doe_mb, u8 *index, u16 *vid,
 		.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_submit_task_wait(doe_mb, &task);
 	if (rc < 0)
 		return rc;
 
-	wait_for_completion(&c);
-
 	if (task.rv != sizeof(response_pl))
 		return -EIO;
 
@@ -360,13 +341,6 @@ static void pci_doe_xa_destroy(void *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);
-}
-
 /**
  * pcim_doe_create_mb() - Create a DOE mailbox object
  *
@@ -392,25 +366,13 @@ struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset)
 	doe_mb->pdev = pdev;
 	doe_mb->cap_offset = cap_offset;
 	init_waitqueue_head(&doe_mb->wq);
+	mutex_init(&doe_mb->lock);
 
 	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),
-						pci_name(pdev),
-						doe_mb->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 = 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) {
@@ -457,24 +419,25 @@ bool pci_doe_supports_prot(struct pci_doe_mb *doe_mb, u16 vid, u8 type)
 EXPORT_SYMBOL_GPL(pci_doe_supports_prot);
 
 /**
- * pci_doe_submit_task() - Submit a task to be processed by the state machine
+ * pci_doe_submit_task_wait() - Submit and execute a task
  *
  * @doe_mb: DOE mailbox capability to submit to
- * @task: task to be queued
- *
- * Submit a DOE task (request/response) to the DOE mailbox to be processed.
- * Returns upon queueing the task object.  If the queue is full this function
- * will sleep until there is room in the queue.
+ * @task: task to be run
  *
- * task->complete will be called when the state machine is done processing this
- * task.
+ * Submit and run DOE task (request/response) to the DOE mailbox to be
+ * processed.
  *
  * Excess data will be discarded.
  *
- * RETURNS: 0 when task has been successfully queued, -ERRNO on error
+ * Context: non-interrupt
+ *
+ * RETURNS: 0 when task was executed, the @task->rv holds the status
+ * result of the executed opertion, -ERRNO on failure to submit.
  */
-int pci_doe_submit_task(struct pci_doe_mb *doe_mb, struct pci_doe_task *task)
+int pci_doe_submit_task_wait(struct pci_doe_mb *doe_mb, struct pci_doe_task *task)
 {
+	int rc;
+
 	if (!pci_doe_supports_prot(doe_mb, task->prot.vid, task->prot.type))
 		return -EINVAL;
 
@@ -490,8 +453,13 @@ 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);
-	queue_work(doe_mb->work_queue, &task->work);
+
+	rc = mutex_lock_interruptible(&doe_mb->lock);
+	if (rc)
+		return rc;
+	exec_task(task);
+	mutex_unlock(&doe_mb->lock);
+
 	return 0;
 }
-EXPORT_SYMBOL_GPL(pci_doe_submit_task);
+EXPORT_SYMBOL_GPL(pci_doe_submit_task_wait);
diff --git a/include/linux/pci-doe.h b/include/linux/pci-doe.h
index ed9b4df792b8..55e6e45b87f3 100644
--- a/include/linux/pci-doe.h
+++ b/include/linux/pci-doe.h
@@ -29,9 +29,6 @@ struct pci_doe_mb;
  * @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
@@ -50,11 +47,6 @@ struct pci_doe_task {
 	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;
 };
 
@@ -72,6 +64,5 @@ 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_submit_task_wait(struct pci_doe_mb *doe_mb, struct pci_doe_task *task);
 #endif
-- 
2.37.2

Re: [PATCH V3 2/2] PCI/DOE: Remove asynchronous task support

[Alison Schofield]

On Sun, Nov 27, 2022 at 08:03:38PM -0800, Ira Weiny wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> Gregory Price and Jonathan Cameron reported a bug within
> pci_doe_submit_task().[1]  The issue was that work item initialization
> needs to be done with either INIT_WORK_ONSTACK() or INIT_WORK()
> depending on how the work item is allocated.
> 
> Initially, it was anticipated that DOE tasks were going to need to be
> submitted asynchronously and the code was designed thusly.  Many
> alternatives were discussed to fix the work initialization issue.[2]
> 
> However, all current users submit tasks synchronously and this has
> therefore become an unneeded maintenance burden.  Remove the extra
> maintenance burden by replacing asynchronous task submission with
> a synchronous wait function.[3]

Commit msg: Please look at replacing this:
PCI/DOE: Remove asynchronous task support
with something like this:
PCI/DOE: Replace async task submission with sync wait function
to more accurately reflect what this patch does. It is not
only removing code.

Like the first patch, I'm unfamiliar w the details, and am
just expecting that a patch that says 'Remove' doesn't add
anything.

> 
> [1] https://lore.kernel.org/linux-cxl/20221014151045.24781-1-Jonathan.Cameron@huawei.com/T/#m88a7f50dcce52f30c8bf5c3dcc06fa9843b54a2d
> [2] https://lore.kernel.org/linux-cxl/Y3kSDQDur+IUDs2O@iweiny-mobl/T/#m0f057773d9c75432fcfcc54a2604483fe82abe92
> [3] https://lore.kernel.org/linux-cxl/Y3kSDQDur+IUDs2O@iweiny-mobl/T/#m32d3f9b208ef7486bc148d94a326b26b2d3e69ff
> 
> Reported-by: Gregory Price <gregory.price@memverge.com>
> Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Suggested-by: "Li, Ming" <ming4.li@intel.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> ---
> Changes from V2:
> 	Jonathan
> 		Fix comment on struct pci_doe_mb
> 		Remove signal_task_complete()
> 
> Changes from V1:
> 	Dan Williams:
> 		Use mutex_lock_interruptible()
> 		Adjust comment to lock the doe_mb structure not the
> 		code
> 	Adjust the locking based on the new series which eliminated
> 	PCI_DOE_FLAG_CANCEL.
> 
> Thanks to Dan for the bulk of the patch.
> Thanks to Ming for pointing out the need for a lock to prevent more
> than 1 task from being processed at a time.
> ---
>  drivers/cxl/core/pci.c  | 16 ++------
>  drivers/pci/doe.c       | 86 +++++++++++++----------------------------
>  include/linux/pci-doe.h | 11 +-----
>  3 files changed, 31 insertions(+), 82 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 9240df53ed87..58977e0712b6 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -490,21 +490,14 @@ 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,        \
> @@ -513,8 +506,6 @@ struct cdat_doe_task cdt = {                                  \
>  		.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,                            \
>  	}                                                     \
>  }
>  
> @@ -525,12 +516,12 @@ static int cxl_cdat_get_length(struct device *dev,
>  	DECLARE_CDAT_DOE_TASK(CDAT_DOE_REQ(0), t);
>  	int rc;
>  
> -	rc = pci_doe_submit_task(cdat_doe, &t.task);
> +	rc = pci_doe_submit_task_wait(cdat_doe, &t.task);
>  	if (rc < 0) {
>  		dev_err(dev, "DOE submit failed: %d", rc);
>  		return rc;
>  	}
> -	wait_for_completion(&t.c);
> +
>  	if (t.task.rv < sizeof(u32))
>  		return -EIO;
>  
> @@ -554,12 +545,11 @@ static int cxl_cdat_read_table(struct device *dev,
>  		u32 *entry;
>  		int rc;
>  
> -		rc = pci_doe_submit_task(cdat_doe, &t.task);
> +		rc = pci_doe_submit_task_wait(cdat_doe, &t.task);
>  		if (rc < 0) {
>  			dev_err(dev, "DOE submit failed: %d", rc);
>  			return rc;
>  		}
> -		wait_for_completion(&t.c);
>  		/* 1 DW header + 1 DW data min */
>  		if (t.task.rv < (2 * sizeof(u32)))
>  			return -EIO;
> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> index 685e7d26c7eb..b399f0f4fb5d 100644
> --- a/drivers/pci/doe.c
> +++ b/drivers/pci/doe.c
> @@ -18,7 +18,6 @@
>  #include <linux/mutex.h>
>  #include <linux/pci.h>
>  #include <linux/pci-doe.h>
> -#include <linux/workqueue.h>
>  
>  #define PCI_DOE_PROTOCOL_DISCOVERY 0
>  
> @@ -40,7 +39,7 @@
>   * @cap_offset: Capability offset
>   * @prots: Array of protocols supported (encoded as long values)
>   * @wq: Wait queue for work item
> - * @work_queue: Queue of pci_doe_work items
> + * @lock: Lock state of doe_mb during task processing
>   * @flags: Bit array of PCI_DOE_FLAG_* flags
>   */
>  struct pci_doe_mb {
> @@ -49,7 +48,7 @@ struct pci_doe_mb {
>  	struct xarray prots;
>  
>  	wait_queue_head_t wq;
> -	struct workqueue_struct *work_queue;
> +	struct mutex lock;
>  	unsigned long flags;
>  };
>  
> @@ -196,12 +195,6 @@ static int pci_doe_recv_resp(struct pci_doe_mb *doe_mb, struct pci_doe_task *tas
>  	return min(length, task->response_pl_sz / sizeof(u32)) * sizeof(u32);
>  }
>  
> -static void signal_task_complete(struct pci_doe_task *task, int rv)
> -{
> -	task->rv = rv;
> -	task->complete(task);
> -}
> -
>  static void signal_task_abort(struct pci_doe_task *task, int rv)
>  {
>  	struct pci_doe_mb *doe_mb = task->doe_mb;
> @@ -216,13 +209,11 @@ static void signal_task_abort(struct pci_doe_task *task, int rv)
>  			doe_mb->cap_offset);
>  		set_bit(PCI_DOE_FLAG_DEAD, &doe_mb->flags);
>  	}
> -	signal_task_complete(task, rv);
> +	task->rv = rv;
>  }
>  
> -static void doe_statemachine_work(struct work_struct *work)
> +static void exec_task(struct pci_doe_task *task)
>  {
> -	struct pci_doe_task *task = container_of(work, struct pci_doe_task,
> -						 work);
>  	struct pci_doe_mb *doe_mb = task->doe_mb;
>  	struct pci_dev *pdev = doe_mb->pdev;
>  	int offset = doe_mb->cap_offset;
> @@ -231,7 +222,7 @@ static void doe_statemachine_work(struct work_struct *work)
>  	int rc;
>  
>  	if (test_bit(PCI_DOE_FLAG_DEAD, &doe_mb->flags)) {
> -		signal_task_complete(task, -EIO);
> +		task->rv = -EIO;
>  		return;
>  	}
>  
> @@ -276,12 +267,7 @@ static void doe_statemachine_work(struct work_struct *work)
>  		return;
>  	}
>  
> -	signal_task_complete(task, rc);
> -}
> -
> -static void pci_doe_task_complete(struct pci_doe_task *task)
> -{
> -	complete(task->private);
> +	task->rv = rc;
>  }
>  
>  static int pci_doe_discovery(struct pci_doe_mb *doe_mb, u8 *index, u16 *vid,
> @@ -290,7 +276,6 @@ 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,
> @@ -298,17 +283,13 @@ static int pci_doe_discovery(struct pci_doe_mb *doe_mb, u8 *index, u16 *vid,
>  		.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_submit_task_wait(doe_mb, &task);
>  	if (rc < 0)
>  		return rc;
>  
> -	wait_for_completion(&c);
> -
>  	if (task.rv != sizeof(response_pl))
>  		return -EIO;
>  
> @@ -360,13 +341,6 @@ static void pci_doe_xa_destroy(void *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);
> -}
> -
>  /**
>   * pcim_doe_create_mb() - Create a DOE mailbox object
>   *
> @@ -392,25 +366,13 @@ struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset)
>  	doe_mb->pdev = pdev;
>  	doe_mb->cap_offset = cap_offset;
>  	init_waitqueue_head(&doe_mb->wq);
> +	mutex_init(&doe_mb->lock);
>  
>  	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),
> -						pci_name(pdev),
> -						doe_mb->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 = 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) {
> @@ -457,24 +419,25 @@ bool pci_doe_supports_prot(struct pci_doe_mb *doe_mb, u16 vid, u8 type)
>  EXPORT_SYMBOL_GPL(pci_doe_supports_prot);
>  
>  /**
> - * pci_doe_submit_task() - Submit a task to be processed by the state machine
> + * pci_doe_submit_task_wait() - Submit and execute a task
>   *
>   * @doe_mb: DOE mailbox capability to submit to
> - * @task: task to be queued
> - *
> - * Submit a DOE task (request/response) to the DOE mailbox to be processed.
> - * Returns upon queueing the task object.  If the queue is full this function
> - * will sleep until there is room in the queue.
> + * @task: task to be run
>   *
> - * task->complete will be called when the state machine is done processing this
> - * task.
> + * Submit and run DOE task (request/response) to the DOE mailbox to be
> + * processed.
>   *
>   * Excess data will be discarded.
>   *
> - * RETURNS: 0 when task has been successfully queued, -ERRNO on error
> + * Context: non-interrupt
> + *
> + * RETURNS: 0 when task was executed, the @task->rv holds the status
> + * result of the executed opertion, -ERRNO on failure to submit.
>   */
> -int pci_doe_submit_task(struct pci_doe_mb *doe_mb, struct pci_doe_task *task)
> +int pci_doe_submit_task_wait(struct pci_doe_mb *doe_mb, struct pci_doe_task *task)
>  {
> +	int rc;
> +
>  	if (!pci_doe_supports_prot(doe_mb, task->prot.vid, task->prot.type))
>  		return -EINVAL;
>  
> @@ -490,8 +453,13 @@ 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);
> -	queue_work(doe_mb->work_queue, &task->work);
> +
> +	rc = mutex_lock_interruptible(&doe_mb->lock);
> +	if (rc)
> +		return rc;
> +	exec_task(task);
> +	mutex_unlock(&doe_mb->lock);
> +
>  	return 0;
>  }
> -EXPORT_SYMBOL_GPL(pci_doe_submit_task);
> +EXPORT_SYMBOL_GPL(pci_doe_submit_task_wait);
> diff --git a/include/linux/pci-doe.h b/include/linux/pci-doe.h
> index ed9b4df792b8..55e6e45b87f3 100644
> --- a/include/linux/pci-doe.h
> +++ b/include/linux/pci-doe.h
> @@ -29,9 +29,6 @@ struct pci_doe_mb;
>   * @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
> @@ -50,11 +47,6 @@ struct pci_doe_task {
>  	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;
>  };
>  
> @@ -72,6 +64,5 @@ 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_submit_task_wait(struct pci_doe_mb *doe_mb, struct pci_doe_task *task);
>  #endif
> -- 
> 2.37.2
> 

Re: [PATCH V3 2/2] PCI/DOE: Remove asynchronous task support

[Ira Weiny]

On Mon, Nov 28, 2022 at 09:58:11AM -0800, Alison Schofield wrote:
> On Sun, Nov 27, 2022 at 08:03:38PM -0800, Ira Weiny wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > Gregory Price and Jonathan Cameron reported a bug within
> > pci_doe_submit_task().[1]  The issue was that work item initialization
> > needs to be done with either INIT_WORK_ONSTACK() or INIT_WORK()
> > depending on how the work item is allocated.
> > 
> > Initially, it was anticipated that DOE tasks were going to need to be
> > submitted asynchronously and the code was designed thusly.  Many
> > alternatives were discussed to fix the work initialization issue.[2]
> > 
> > However, all current users submit tasks synchronously and this has
> > therefore become an unneeded maintenance burden.  Remove the extra
> > maintenance burden by replacing asynchronous task submission with
> > a synchronous wait function.[3]
> 
> Commit msg: Please look at replacing this:
> PCI/DOE: Remove asynchronous task support
> with something like this:
> PCI/DOE: Replace async task submission with sync wait function

Ok yes this makes sense.

Thanks for looking!
Ira

> to more accurately reflect what this patch does. It is not
> only removing code.
> 
> Like the first patch, I'm unfamiliar w the details, and am
> just expecting that a patch that says 'Remove' doesn't add
> anything.
> 
> > 
> > [1] https://lore.kernel.org/linux-cxl/20221014151045.24781-1-Jonathan.Cameron@huawei.com/T/#m88a7f50dcce52f30c8bf5c3dcc06fa9843b54a2d
> > [2] https://lore.kernel.org/linux-cxl/Y3kSDQDur+IUDs2O@iweiny-mobl/T/#m0f057773d9c75432fcfcc54a2604483fe82abe92
> > [3] https://lore.kernel.org/linux-cxl/Y3kSDQDur+IUDs2O@iweiny-mobl/T/#m32d3f9b208ef7486bc148d94a326b26b2d3e69ff
> > 
> > Reported-by: Gregory Price <gregory.price@memverge.com>
> > Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Suggested-by: Dan Williams <dan.j.williams@intel.com>
> > Suggested-by: "Li, Ming" <ming4.li@intel.com>
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > 
> > ---
> > Changes from V2:
> > 	Jonathan
> > 		Fix comment on struct pci_doe_mb
> > 		Remove signal_task_complete()
> > 
> > Changes from V1:
> > 	Dan Williams:
> > 		Use mutex_lock_interruptible()
> > 		Adjust comment to lock the doe_mb structure not the
> > 		code
> > 	Adjust the locking based on the new series which eliminated
> > 	PCI_DOE_FLAG_CANCEL.
> > 
> > Thanks to Dan for the bulk of the patch.
> > Thanks to Ming for pointing out the need for a lock to prevent more
> > than 1 task from being processed at a time.
> > ---
> >  drivers/cxl/core/pci.c  | 16 ++------
> >  drivers/pci/doe.c       | 86 +++++++++++++----------------------------
> >  include/linux/pci-doe.h | 11 +-----
> >  3 files changed, 31 insertions(+), 82 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> > index 9240df53ed87..58977e0712b6 100644
> > --- a/drivers/cxl/core/pci.c
> > +++ b/drivers/cxl/core/pci.c
> > @@ -490,21 +490,14 @@ 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,        \
> > @@ -513,8 +506,6 @@ struct cdat_doe_task cdt = {                                  \
> >  		.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,                            \
> >  	}                                                     \
> >  }
> >  
> > @@ -525,12 +516,12 @@ static int cxl_cdat_get_length(struct device *dev,
> >  	DECLARE_CDAT_DOE_TASK(CDAT_DOE_REQ(0), t);
> >  	int rc;
> >  
> > -	rc = pci_doe_submit_task(cdat_doe, &t.task);
> > +	rc = pci_doe_submit_task_wait(cdat_doe, &t.task);
> >  	if (rc < 0) {
> >  		dev_err(dev, "DOE submit failed: %d", rc);
> >  		return rc;
> >  	}
> > -	wait_for_completion(&t.c);
> > +
> >  	if (t.task.rv < sizeof(u32))
> >  		return -EIO;
> >  
> > @@ -554,12 +545,11 @@ static int cxl_cdat_read_table(struct device *dev,
> >  		u32 *entry;
> >  		int rc;
> >  
> > -		rc = pci_doe_submit_task(cdat_doe, &t.task);
> > +		rc = pci_doe_submit_task_wait(cdat_doe, &t.task);
> >  		if (rc < 0) {
> >  			dev_err(dev, "DOE submit failed: %d", rc);
> >  			return rc;
> >  		}
> > -		wait_for_completion(&t.c);
> >  		/* 1 DW header + 1 DW data min */
> >  		if (t.task.rv < (2 * sizeof(u32)))
> >  			return -EIO;
> > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> > index 685e7d26c7eb..b399f0f4fb5d 100644
> > --- a/drivers/pci/doe.c
> > +++ b/drivers/pci/doe.c
> > @@ -18,7 +18,6 @@
> >  #include <linux/mutex.h>
> >  #include <linux/pci.h>
> >  #include <linux/pci-doe.h>
> > -#include <linux/workqueue.h>
> >  
> >  #define PCI_DOE_PROTOCOL_DISCOVERY 0
> >  
> > @@ -40,7 +39,7 @@
> >   * @cap_offset: Capability offset
> >   * @prots: Array of protocols supported (encoded as long values)
> >   * @wq: Wait queue for work item
> > - * @work_queue: Queue of pci_doe_work items
> > + * @lock: Lock state of doe_mb during task processing
> >   * @flags: Bit array of PCI_DOE_FLAG_* flags
> >   */
> >  struct pci_doe_mb {
> > @@ -49,7 +48,7 @@ struct pci_doe_mb {
> >  	struct xarray prots;
> >  
> >  	wait_queue_head_t wq;
> > -	struct workqueue_struct *work_queue;
> > +	struct mutex lock;
> >  	unsigned long flags;
> >  };
> >  
> > @@ -196,12 +195,6 @@ static int pci_doe_recv_resp(struct pci_doe_mb *doe_mb, struct pci_doe_task *tas
> >  	return min(length, task->response_pl_sz / sizeof(u32)) * sizeof(u32);
> >  }
> >  
> > -static void signal_task_complete(struct pci_doe_task *task, int rv)
> > -{
> > -	task->rv = rv;
> > -	task->complete(task);
> > -}
> > -
> >  static void signal_task_abort(struct pci_doe_task *task, int rv)
> >  {
> >  	struct pci_doe_mb *doe_mb = task->doe_mb;
> > @@ -216,13 +209,11 @@ static void signal_task_abort(struct pci_doe_task *task, int rv)
> >  			doe_mb->cap_offset);
> >  		set_bit(PCI_DOE_FLAG_DEAD, &doe_mb->flags);
> >  	}
> > -	signal_task_complete(task, rv);
> > +	task->rv = rv;
> >  }
> >  
> > -static void doe_statemachine_work(struct work_struct *work)
> > +static void exec_task(struct pci_doe_task *task)
> >  {
> > -	struct pci_doe_task *task = container_of(work, struct pci_doe_task,
> > -						 work);
> >  	struct pci_doe_mb *doe_mb = task->doe_mb;
> >  	struct pci_dev *pdev = doe_mb->pdev;
> >  	int offset = doe_mb->cap_offset;
> > @@ -231,7 +222,7 @@ static void doe_statemachine_work(struct work_struct *work)
> >  	int rc;
> >  
> >  	if (test_bit(PCI_DOE_FLAG_DEAD, &doe_mb->flags)) {
> > -		signal_task_complete(task, -EIO);
> > +		task->rv = -EIO;
> >  		return;
> >  	}
> >  
> > @@ -276,12 +267,7 @@ static void doe_statemachine_work(struct work_struct *work)
> >  		return;
> >  	}
> >  
> > -	signal_task_complete(task, rc);
> > -}
> > -
> > -static void pci_doe_task_complete(struct pci_doe_task *task)
> > -{
> > -	complete(task->private);
> > +	task->rv = rc;
> >  }
> >  
> >  static int pci_doe_discovery(struct pci_doe_mb *doe_mb, u8 *index, u16 *vid,
> > @@ -290,7 +276,6 @@ 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,
> > @@ -298,17 +283,13 @@ static int pci_doe_discovery(struct pci_doe_mb *doe_mb, u8 *index, u16 *vid,
> >  		.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_submit_task_wait(doe_mb, &task);
> >  	if (rc < 0)
> >  		return rc;
> >  
> > -	wait_for_completion(&c);
> > -
> >  	if (task.rv != sizeof(response_pl))
> >  		return -EIO;
> >  
> > @@ -360,13 +341,6 @@ static void pci_doe_xa_destroy(void *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);
> > -}
> > -
> >  /**
> >   * pcim_doe_create_mb() - Create a DOE mailbox object
> >   *
> > @@ -392,25 +366,13 @@ struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset)
> >  	doe_mb->pdev = pdev;
> >  	doe_mb->cap_offset = cap_offset;
> >  	init_waitqueue_head(&doe_mb->wq);
> > +	mutex_init(&doe_mb->lock);
> >  
> >  	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),
> > -						pci_name(pdev),
> > -						doe_mb->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 = 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) {
> > @@ -457,24 +419,25 @@ bool pci_doe_supports_prot(struct pci_doe_mb *doe_mb, u16 vid, u8 type)
> >  EXPORT_SYMBOL_GPL(pci_doe_supports_prot);
> >  
> >  /**
> > - * pci_doe_submit_task() - Submit a task to be processed by the state machine
> > + * pci_doe_submit_task_wait() - Submit and execute a task
> >   *
> >   * @doe_mb: DOE mailbox capability to submit to
> > - * @task: task to be queued
> > - *
> > - * Submit a DOE task (request/response) to the DOE mailbox to be processed.
> > - * Returns upon queueing the task object.  If the queue is full this function
> > - * will sleep until there is room in the queue.
> > + * @task: task to be run
> >   *
> > - * task->complete will be called when the state machine is done processing this
> > - * task.
> > + * Submit and run DOE task (request/response) to the DOE mailbox to be
> > + * processed.
> >   *
> >   * Excess data will be discarded.
> >   *
> > - * RETURNS: 0 when task has been successfully queued, -ERRNO on error
> > + * Context: non-interrupt
> > + *
> > + * RETURNS: 0 when task was executed, the @task->rv holds the status
> > + * result of the executed opertion, -ERRNO on failure to submit.
> >   */
> > -int pci_doe_submit_task(struct pci_doe_mb *doe_mb, struct pci_doe_task *task)
> > +int pci_doe_submit_task_wait(struct pci_doe_mb *doe_mb, struct pci_doe_task *task)
> >  {
> > +	int rc;
> > +
> >  	if (!pci_doe_supports_prot(doe_mb, task->prot.vid, task->prot.type))
> >  		return -EINVAL;
> >  
> > @@ -490,8 +453,13 @@ 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);
> > -	queue_work(doe_mb->work_queue, &task->work);
> > +
> > +	rc = mutex_lock_interruptible(&doe_mb->lock);
> > +	if (rc)
> > +		return rc;
> > +	exec_task(task);
> > +	mutex_unlock(&doe_mb->lock);
> > +
> >  	return 0;
> >  }
> > -EXPORT_SYMBOL_GPL(pci_doe_submit_task);
> > +EXPORT_SYMBOL_GPL(pci_doe_submit_task_wait);
> > diff --git a/include/linux/pci-doe.h b/include/linux/pci-doe.h
> > index ed9b4df792b8..55e6e45b87f3 100644
> > --- a/include/linux/pci-doe.h
> > +++ b/include/linux/pci-doe.h
> > @@ -29,9 +29,6 @@ struct pci_doe_mb;
> >   * @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
> > @@ -50,11 +47,6 @@ struct pci_doe_task {
> >  	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;
> >  };
> >  
> > @@ -72,6 +64,5 @@ 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_submit_task_wait(struct pci_doe_mb *doe_mb, struct pci_doe_task *task);
> >  #endif
> > -- 
> > 2.37.2
> > 

Re: [PATCH V3 2/2] PCI/DOE: Remove asynchronous task support

[Ira Weiny]

On Mon, Nov 28, 2022 at 05:51:12PM +0800, Hillf Danton wrote:
> On 27 Nov 2022 20:03:38 -0800 Ira Weiny <ira.weiny@intel.com>
> > @@ -49,7 +48,7 @@ struct pci_doe_mb {
> >  	struct xarray prots;
> >  
> >  	wait_queue_head_t wq;
> > -	struct workqueue_struct *work_queue;
> > +	struct mutex lock;
> >  	unsigned long flags;
> >  };
> 
> Add a followup cleanup patch to cut wait queue off as the wakeup in
> pci_doe_flush_mb() is gone.

Thanks.  That should have been part of patch 1/2 as it is gone then.

Ira