Message-ID: <0d87dc70-67d5-4de0-8f12-9895640fd904@linux.intel.com>
Date: Fri, 16 May 2025 16:14:08 -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


On 5/16/25 3:58 PM, Sathyanarayanan Kuppuswamy wrote:
> 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?

Please ignore my comment. It looks like you have cleaned up everything
int the end.

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