[PATCH v2] drm/vmgfx: Use non-hybrid PCI devres API

[PATCH v2] drm/vmgfx: Use non-hybrid PCI devres API

[Philipp Stanner]

vmgfx enables its PCI device with pcim_enable_device(). This,
implicitly, switches the function pci_request_regions() into managed
mode, where it becomes a devres function.

The PCI subsystem wants to remove this hybrid nature from its
interfaces. To do so, users of the aforementioned combination of
functions must be ported to non-hybrid functions.

Moreover, since both functions are already managed in this driver, the
calls to pci_release_regions() are unnecessary.

Remove the calls to pci_release_regions().

Replace the call to sometimes-managed pci_request_regions() with one to
the always-managed pcim_request_all_regions().

Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
Changes in v2:
  - Fix unused variable error.
---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 0f32471c8533..1e3ebace32ae 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -733,7 +733,7 @@ static int vmw_setup_pci_resources(struct vmw_private *dev,
 
 	pci_set_master(pdev);
 
-	ret = pci_request_regions(pdev, "vmwgfx probe");
+	ret = pcim_request_all_regions(pdev, "vmwgfx probe");
 	if (ret)
 		return ret;
 
@@ -753,7 +753,6 @@ static int vmw_setup_pci_resources(struct vmw_private *dev,
 		if (!dev->rmmio) {
 			drm_err(&dev->drm,
 				"Failed mapping registers mmio memory.\n");
-			pci_release_regions(pdev);
 			return -ENOMEM;
 		}
 	} else if (pci_id == VMWGFX_PCI_ID_SVGA2) {
@@ -774,11 +773,9 @@ static int vmw_setup_pci_resources(struct vmw_private *dev,
 		if (IS_ERR(dev->fifo_mem)) {
 			drm_err(&dev->drm,
 				  "Failed mapping FIFO memory.\n");
-			pci_release_regions(pdev);
 			return PTR_ERR(dev->fifo_mem);
 		}
 	} else {
-		pci_release_regions(pdev);
 		return -EINVAL;
 	}
 
@@ -856,7 +853,6 @@ static int vmw_driver_load(struct vmw_private *dev_priv, u32 pci_id)
 	int ret;
 	enum vmw_res_type i;
 	bool refuse_dma = false;
-	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
 
 	vmw_sw_context_init(dev_priv);
 
@@ -872,7 +868,7 @@ static int vmw_driver_load(struct vmw_private *dev_priv, u32 pci_id)
 		return ret;
 	ret = vmw_detect_version(dev_priv);
 	if (ret)
-		goto out_no_pci_or_version;
+		return ret;
 
 
 	for (i = vmw_res_context; i < vmw_res_max; ++i) {
@@ -1172,15 +1168,13 @@ static int vmw_driver_load(struct vmw_private *dev_priv, u32 pci_id)
 
 	if (dev_priv->ctx.staged_bindings)
 		vmw_binding_state_free(dev_priv->ctx.staged_bindings);
-out_no_pci_or_version:
-	pci_release_regions(pdev);
+
 	return ret;
 }
 
 static void vmw_driver_unload(struct drm_device *dev)
 {
 	struct vmw_private *dev_priv = vmw_priv(dev);
-	struct pci_dev *pdev = to_pci_dev(dev->dev);
 	enum vmw_res_type i;
 
 	unregister_pm_notifier(&dev_priv->pm_nb);
@@ -1216,8 +1210,6 @@ static void vmw_driver_unload(struct drm_device *dev)
 		idr_destroy(&dev_priv->res_idr[i]);
 
 	vmw_mksstat_remove_all(dev_priv);
-
-	pci_release_regions(pdev);
 }
 
 static void vmw_postclose(struct drm_device *dev,
-- 
2.48.1

Re: [PATCH v2] drm/vmgfx: Use non-hybrid PCI devres API

[Philipp Stanner]

On Wed, 2025-04-23 at 14:06 +0200, Philipp Stanner wrote:
> vmgfx enables its PCI device with pcim_enable_device(). This,
> implicitly, switches the function pci_request_regions() into managed
> mode, where it becomes a devres function.
> 
> The PCI subsystem wants to remove this hybrid nature from its
> interfaces. To do so, users of the aforementioned combination of
> functions must be ported to non-hybrid functions.
> 
> Moreover, since both functions are already managed in this driver,
> the
> calls to pci_release_regions() are unnecessary.
> 
> Remove the calls to pci_release_regions().
> 
> Replace the call to sometimes-managed pci_request_regions() with one
> to
> the always-managed pcim_request_all_regions().
> 
> Signed-off-by: Philipp Stanner <phasta@kernel.org>

*PING*

> ---
> Changes in v2:
>   - Fix unused variable error.
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index 0f32471c8533..1e3ebace32ae 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -733,7 +733,7 @@ static int vmw_setup_pci_resources(struct
> vmw_private *dev,
>  
>  	pci_set_master(pdev);
>  
> -	ret = pci_request_regions(pdev, "vmwgfx probe");
> +	ret = pcim_request_all_regions(pdev, "vmwgfx probe");
>  	if (ret)
>  		return ret;
>  
> @@ -753,7 +753,6 @@ static int vmw_setup_pci_resources(struct
> vmw_private *dev,
>  		if (!dev->rmmio) {
>  			drm_err(&dev->drm,
>  				"Failed mapping registers mmio
> memory.\n");
> -			pci_release_regions(pdev);
>  			return -ENOMEM;
>  		}
>  	} else if (pci_id == VMWGFX_PCI_ID_SVGA2) {
> @@ -774,11 +773,9 @@ static int vmw_setup_pci_resources(struct
> vmw_private *dev,
>  		if (IS_ERR(dev->fifo_mem)) {
>  			drm_err(&dev->drm,
>  				  "Failed mapping FIFO memory.\n");
> -			pci_release_regions(pdev);
>  			return PTR_ERR(dev->fifo_mem);
>  		}
>  	} else {
> -		pci_release_regions(pdev);
>  		return -EINVAL;
>  	}
>  
> @@ -856,7 +853,6 @@ static int vmw_driver_load(struct vmw_private
> *dev_priv, u32 pci_id)
>  	int ret;
>  	enum vmw_res_type i;
>  	bool refuse_dma = false;
> -	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
>  
>  	vmw_sw_context_init(dev_priv);
>  
> @@ -872,7 +868,7 @@ static int vmw_driver_load(struct vmw_private
> *dev_priv, u32 pci_id)
>  		return ret;
>  	ret = vmw_detect_version(dev_priv);
>  	if (ret)
> -		goto out_no_pci_or_version;
> +		return ret;
>  
>  
>  	for (i = vmw_res_context; i < vmw_res_max; ++i) {
> @@ -1172,15 +1168,13 @@ static int vmw_driver_load(struct vmw_private
> *dev_priv, u32 pci_id)
>  
>  	if (dev_priv->ctx.staged_bindings)
>  		vmw_binding_state_free(dev_priv-
> >ctx.staged_bindings);
> -out_no_pci_or_version:
> -	pci_release_regions(pdev);
> +
>  	return ret;
>  }
>  
>  static void vmw_driver_unload(struct drm_device *dev)
>  {
>  	struct vmw_private *dev_priv = vmw_priv(dev);
> -	struct pci_dev *pdev = to_pci_dev(dev->dev);
>  	enum vmw_res_type i;
>  
>  	unregister_pm_notifier(&dev_priv->pm_nb);
> @@ -1216,8 +1210,6 @@ static void vmw_driver_unload(struct drm_device
> *dev)
>  		idr_destroy(&dev_priv->res_idr[i]);
>  
>  	vmw_mksstat_remove_all(dev_priv);
> -
> -	pci_release_regions(pdev);
>  }
>  
>  static void vmw_postclose(struct drm_device *dev,

Re: [PATCH v2] drm/vmgfx: Use non-hybrid PCI devres API

[Zack Rusin]

[-- Attachment #1: Type: text/plain, Size: 1189 bytes --]

On Thu, May 8, 2025 at 6:40 AM Philipp Stanner <phasta@mailbox.org> wrote:
>
> On Wed, 2025-04-23 at 14:06 +0200, Philipp Stanner wrote:
> > vmgfx enables its PCI device with pcim_enable_device(). This,
> > implicitly, switches the function pci_request_regions() into managed
> > mode, where it becomes a devres function.
> >
> > The PCI subsystem wants to remove this hybrid nature from its
> > interfaces. To do so, users of the aforementioned combination of
> > functions must be ported to non-hybrid functions.
> >
> > Moreover, since both functions are already managed in this driver,
> > the
> > calls to pci_release_regions() are unnecessary.
> >
> > Remove the calls to pci_release_regions().
> >
> > Replace the call to sometimes-managed pci_request_regions() with one
> > to
> > the always-managed pcim_request_all_regions().
> >
> > Signed-off-by: Philipp Stanner <phasta@kernel.org>
>
> *PING*

Thanks, that looks great. I missed it because the driver's name is
vmwgfx. I'd be happy to fix the subject for you while pushing this to
drm-misc-fixes, if you're ok with it of course. Otherwise:
Reviewed-by: Zack Rusin <zack.rusin@broadcom.com>

z

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5414 bytes --]

Re: [PATCH v2] drm/vmgfx: Use non-hybrid PCI devres API

[Philipp Stanner]

On Thu, 2025-05-08 at 11:39 -0400, Zack Rusin wrote:
> On Thu, May 8, 2025 at 6:40 AM Philipp Stanner <phasta@mailbox.org>
> wrote:
> > 
> > On Wed, 2025-04-23 at 14:06 +0200, Philipp Stanner wrote:
> > > vmgfx enables its PCI device with pcim_enable_device(). This,
> > > implicitly, switches the function pci_request_regions() into
> > > managed
> > > mode, where it becomes a devres function.
> > > 
> > > The PCI subsystem wants to remove this hybrid nature from its
> > > interfaces. To do so, users of the aforementioned combination of
> > > functions must be ported to non-hybrid functions.
> > > 
> > > Moreover, since both functions are already managed in this
> > > driver,
> > > the
> > > calls to pci_release_regions() are unnecessary.
> > > 
> > > Remove the calls to pci_release_regions().
> > > 
> > > Replace the call to sometimes-managed pci_request_regions() with
> > > one
> > > to
> > > the always-managed pcim_request_all_regions().
> > > 
> > > Signed-off-by: Philipp Stanner <phasta@kernel.org>
> > 
> > *PING*
> 
> Thanks, that looks great. I missed it because the driver's name is
> vmwgfx. I'd be happy to fix the subject for you while pushing this to
> drm-misc-fixes, if you're ok with it of course.

Ah, my bad! (although having a driver called "Bob" would be handy at
times)

Sure thing, go ahead and rephrase it as you like.

P.

>  Otherwise:
> Reviewed-by: Zack Rusin <zack.rusin@broadcom.com>
> 
> z

Re: [PATCH v2] drm/vmgfx: Use non-hybrid PCI devres API

[Philipp Stanner]

On Thu, 2025-05-08 at 11:39 -0400, Zack Rusin wrote:
> On Thu, May 8, 2025 at 6:40 AM Philipp Stanner <phasta@mailbox.org>
> wrote:
> > 
> > On Wed, 2025-04-23 at 14:06 +0200, Philipp Stanner wrote:
> > > vmgfx enables its PCI device with pcim_enable_device(). This,
> > > implicitly, switches the function pci_request_regions() into
> > > managed
> > > mode, where it becomes a devres function.
> > > 
> > > The PCI subsystem wants to remove this hybrid nature from its
> > > interfaces. To do so, users of the aforementioned combination of
> > > functions must be ported to non-hybrid functions.
> > > 
> > > Moreover, since both functions are already managed in this
> > > driver,
> > > the
> > > calls to pci_release_regions() are unnecessary.
> > > 
> > > Remove the calls to pci_release_regions().
> > > 
> > > Replace the call to sometimes-managed pci_request_regions() with
> > > one
> > > to
> > > the always-managed pcim_request_all_regions().
> > > 
> > > Signed-off-by: Philipp Stanner <phasta@kernel.org>
> > 
> > *PING*
> 
> Thanks, that looks great. I missed it because the driver's name is
> vmwgfx. I'd be happy to fix the subject for you while pushing this to
> drm-misc-fixes,

But, I don't think that this is a bug. It's just a bit ugly and this
patch performs cleanup work, but all the code behaves as intended,
without leaks or stuff like that.

Thus, I think it should go to drm-misc-next instead of drm-misc-fixes.

Regards

>  if you're ok with it of course. Otherwise:
> Reviewed-by: Zack Rusin <zack.rusin@broadcom.com>
> 
> z