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

From: Ira Weiny <ira.weiny@intel.com>
To: Lukas Wunner <lukas@wunner.de>,
	Bjorn Helgaas <helgaas@kernel.org>, <linux-pci@vger.kernel.org>
Cc: Gregory Price <gregory.price@memverge.com>,
	Ira Weiny <ira.weiny@intel.com>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	"Dan Williams" <dan.j.williams@intel.com>,
	Alison Schofield <alison.schofield@intel.com>,
	Vishal Verma <vishal.l.verma@intel.com>,
	"Dave Jiang" <dave.jiang@intel.com>,
	"Li, Ming" <ming4.li@intel.com>, Hillf Danton <hdanton@sina.com>,
	Ben Widawsky <bwidawsk@kernel.org>, <linuxarm@huawei.com>,
	<linux-cxl@vger.kernel.org>
Subject: Re: [PATCH v2 09/10] PCI/DOE: Make mailbox creation API private
Date: Mon, 23 Jan 2023 17:25:07 -0800	[thread overview]
Message-ID: <63cf3373e213c_5cca29410@iweiny-mobl.notmuch> (raw)
In-Reply-To: <3dca6f956342707fb69cba94a771f3d4d2f5f3b4.1674468099.git.lukas@wunner.de>

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
>