Message-ID: <99dba1da-4fc6-4e35-a6fc-40233144f7dd@linux.intel.com>
Date: Fri, 16 May 2025 15:58:59 -0700
From: Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@...ux.intel.com>
To: Philipp Stanner <phasta@...nel.org>, Jonathan Corbet <corbet@....net>,
 Bjorn Helgaas <bhelgaas@...gle.com>, Mark Brown <broonie@...nel.org>,
 David Lechner <dlechner@...libre.com>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Zijun Hu <quic_zijuhu@...cinc.com>, Yang Yingliang
 <yangyingliang@...wei.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Krzysztof Wilczyński <kw@...ux.com>
Cc: linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-pci@...r.kernel.org
Subject: Re: [PATCH v2 1/6] PCI: Remove hybrid devres nature from request
 functions

Hi,

On 5/16/25 10:41 AM, Philipp Stanner wrote:
> All functions based on __pci_request_region() and its release counter
> part support "hybrid mode", where the functions become managed if the
> PCI device was enabled with pcim_enable_device().
>
> Removing this undesirable feature requires to remove all users who
> activated their device with that function and use one of the affected
> request functions.
>
> These users were:
> 	ASoC
> 	alsa
> 	cardreader
> 	cirrus
> 	i2c
> 	mmc
> 	mtd
> 	mtd
> 	mxser
> 	net
> 	spi
> 	vdpa
> 	vmwgfx
>
> all of which have been ported to always-managed pcim_ functions by now.
>
> The hybrid nature can, thus, be removed from the aforementioned PCI
> functions.
>
> Remove all function guards and documentation in pci.c related to the
> hybrid redirection. Adjust the visibility of pcim_release_region().
>
> Signed-off-by: Philipp Stanner <phasta@...nel.org>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> ---

Reviewed-by: Kuppuswamy Sathyanarayanan 
<sathyanarayanan.kuppuswamy@...ux.intel.com>

>   drivers/pci/devres.c | 39 ++++++++++++---------------------------
>   drivers/pci/pci.c    | 42 ------------------------------------------
>   drivers/pci/pci.h    |  1 -
>   3 files changed, 12 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
> index 73047316889e..5480d537f400 100644
> --- a/drivers/pci/devres.c
> +++ b/drivers/pci/devres.c
> @@ -6,30 +6,13 @@
>   /*
>    * On the state of PCI's devres implementation:
>    *
> - * The older devres API for PCI has two significant problems:
> + * The older PCI devres API has one significant problem:
>    *
> - * 1. It is very strongly tied to the statically allocated mapping table in
> - *    struct pcim_iomap_devres below. This is mostly solved in the sense of the
> - *    pcim_ functions in this file providing things like ranged mapping by
> - *    bypassing this table, whereas the functions that were present in the old
> - *    API still enter the mapping addresses into the table for users of the old
> - *    API.
> - *
> - * 2. The region-request-functions in pci.c do become managed IF the device has
> - *    been enabled with pcim_enable_device() instead of pci_enable_device().
> - *    This resulted in the API becoming inconsistent: Some functions have an
> - *    obviously managed counter-part (e.g., pci_iomap() <-> pcim_iomap()),
> - *    whereas some don't and are never managed, while others don't and are
> - *    _sometimes_ managed (e.g. pci_request_region()).
> - *
> - *    Consequently, in the new API, region requests performed by the pcim_
> - *    functions are automatically cleaned up through the devres callback
> - *    pcim_addr_resource_release().
> - *
> - *    Users of pcim_enable_device() + pci_*region*() are redirected in
> - *    pci.c to the managed functions here in this file. This isn't exactly
> - *    perfect, but the only alternative way would be to port ALL drivers
> - *    using said combination to pcim_ functions.
> + * It is very strongly tied to the statically allocated mapping table in struct
> + * pcim_iomap_devres below. This is mostly solved in the sense of the pcim_
> + * functions in this file providing things like ranged mapping by bypassing
> + * this table, whereas the functions that were present in the old API still
> + * enter the mapping addresses into the table for users of the old API.
>    *
>    * TODO:
>    * Remove the legacy table entirely once all calls to pcim_iomap_table() in
> @@ -89,10 +72,12 @@ static inline void pcim_addr_devres_clear(struct pcim_addr_devres *res)
>   
>   /*
>    * The following functions, __pcim_*_region*, exist as counterparts to the
> - * versions from pci.c - which, unfortunately, can be in "hybrid mode", i.e.,
> - * sometimes managed, sometimes not.
> + * versions from pci.c - which, unfortunately, were in the past in "hybrid
> + * mode", i.e., sometimes managed, sometimes not.

Why not remove "hybrid mode"  reference like other places?

>    *
> - * To separate the APIs cleanly, we define our own, simplified versions here.
> + * To separate the APIs cleanly, we defined our own, simplified versions here.
> + *
> + * TODO: unify those functions with the counterparts in pci.c
>    */
>   
>   /**
> @@ -893,7 +878,7 @@ int pcim_request_region_exclusive(struct pci_dev *pdev, int bar, const char *nam
>    * Release a region manually that was previously requested by
>    * pcim_request_region().
>    */
> -void pcim_release_region(struct pci_dev *pdev, int bar)
> +static void pcim_release_region(struct pci_dev *pdev, int bar)
>   {
>   	struct pcim_addr_devres res_searched;
>   
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e77d5b53c0ce..4acc23823637 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3937,16 +3937,6 @@ void pci_release_region(struct pci_dev *pdev, int bar)
>   	if (!pci_bar_index_is_valid(bar))
>   		return;
>   
> -	/*
> -	 * This is done for backwards compatibility, because the old PCI devres
> -	 * API had a mode in which the function became managed if it had been
> -	 * enabled with pcim_enable_device() instead of pci_enable_device().
> -	 */
> -	if (pci_is_managed(pdev)) {
> -		pcim_release_region(pdev, bar);
> -		return;
> -	}
> -
>   	if (pci_resource_len(pdev, bar) == 0)
>   		return;
>   	if (pci_resource_flags(pdev, bar) & IORESOURCE_IO)
> @@ -3984,13 +3974,6 @@ static int __pci_request_region(struct pci_dev *pdev, int bar,
>   	if (!pci_bar_index_is_valid(bar))
>   		return -EINVAL;
>   
> -	if (pci_is_managed(pdev)) {
> -		if (exclusive == IORESOURCE_EXCLUSIVE)
> -			return pcim_request_region_exclusive(pdev, bar, name);
> -
> -		return pcim_request_region(pdev, bar, name);
> -	}
> -
>   	if (pci_resource_len(pdev, bar) == 0)
>   		return 0;
>   
> @@ -4027,11 +4010,6 @@ static int __pci_request_region(struct pci_dev *pdev, int bar,
>    *
>    * Returns 0 on success, or %EBUSY on error.  A warning
>    * message is also printed on failure.
> - *
> - * NOTE:
> - * This is a "hybrid" function: It's normally unmanaged, but becomes managed
> - * when pcim_enable_device() has been called in advance. This hybrid feature is
> - * DEPRECATED! If you want managed cleanup, use the pcim_* functions instead.
>    */
>   int pci_request_region(struct pci_dev *pdev, int bar, const char *name)
>   {
> @@ -4084,11 +4062,6 @@ static int __pci_request_selected_regions(struct pci_dev *pdev, int bars,
>    * @name: Name of the driver requesting the resources
>    *
>    * Returns: 0 on success, negative error code on failure.
> - *
> - * NOTE:
> - * This is a "hybrid" function: It's normally unmanaged, but becomes managed
> - * when pcim_enable_device() has been called in advance. This hybrid feature is
> - * DEPRECATED! If you want managed cleanup, use the pcim_* functions instead.
>    */
>   int pci_request_selected_regions(struct pci_dev *pdev, int bars,
>   				 const char *name)
> @@ -4104,11 +4077,6 @@ EXPORT_SYMBOL(pci_request_selected_regions);
>    * @name: name of the driver requesting the resources
>    *
>    * Returns: 0 on success, negative error code on failure.
> - *
> - * NOTE:
> - * This is a "hybrid" function: It's normally unmanaged, but becomes managed
> - * when pcim_enable_device() has been called in advance. This hybrid feature is
> - * DEPRECATED! If you want managed cleanup, use the pcim_* functions instead.
>    */
>   int pci_request_selected_regions_exclusive(struct pci_dev *pdev, int bars,
>   					   const char *name)
> @@ -4144,11 +4112,6 @@ EXPORT_SYMBOL(pci_release_regions);
>    *
>    * Returns 0 on success, or %EBUSY on error.  A warning
>    * message is also printed on failure.
> - *
> - * NOTE:
> - * This is a "hybrid" function: It's normally unmanaged, but becomes managed
> - * when pcim_enable_device() has been called in advance. This hybrid feature is
> - * DEPRECATED! If you want managed cleanup, use the pcim_* functions instead.
>    */
>   int pci_request_regions(struct pci_dev *pdev, const char *name)
>   {
> @@ -4173,11 +4136,6 @@ EXPORT_SYMBOL(pci_request_regions);
>    *
>    * Returns 0 on success, or %EBUSY on error.  A warning message is also
>    * printed on failure.
> - *
> - * NOTE:
> - * This is a "hybrid" function: It's normally unmanaged, but becomes managed
> - * when pcim_enable_device() has been called in advance. This hybrid feature is
> - * DEPRECATED! If you want managed cleanup, use the pcim_* functions instead.
>    */
>   int pci_request_regions_exclusive(struct pci_dev *pdev, const char *name)
>   {
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index b81e99cd4b62..8c3e5fb2443a 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -1062,7 +1062,6 @@ static inline pci_power_t mid_pci_get_power_state(struct pci_dev *pdev)
>   int pcim_intx(struct pci_dev *dev, int enable);
>   int pcim_request_region_exclusive(struct pci_dev *pdev, int bar,
>   				  const char *name);
> -void pcim_release_region(struct pci_dev *pdev, int bar);
>   

Since you removed the only use of pcim_request_region_exclusive(), why 
not remove the definition in the same patch?
>   /*
>    * Config Address for PCI Configuration Mechanism #1

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Powered by blists - more mailing lists