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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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