Re: [PATCH v2 1/6] PCI: Remove hybrid devres nature from request functions

[Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>]

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@xxxxxxxxxx>
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > ---
>
> Reviewed-by: Kuppuswamy Sathyanarayanan 
> <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
>
> >   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