[v4] vdpa/octeon_ep: Control PCI dev enabling manually

[PATCH v4] vdpa/octeon_ep: Control PCI dev enabling manually

Posted by Philipp Stanner 7 months, 2 weeks ago

PCI region request functions such as pci_request_region() currently have
the problem of becoming sometimes managed functions, if
pcim_enable_device() instead of pci_enable_device() was called. The PCI
subsystem wants to remove this deprecated behavior from its interfaces.

octeopn_ep enables its device with pcim_enable_device() (for VF. PF uses
manual management), but does so only to get automatic disablement. The
driver wants to manage its PCI resources for VF manually, without devres.

The easiest way not to use automatic resource management at all is by
also handling device enable- and disablement manually.

Replace pcim_enable_device() with pci_enable_device(). Add the necessary
calls to pci_disable_device().

Signed-off-by: Philipp Stanner <phasta@kernel.org>
Acked-by: Vamsi Attunuru <vattunuru@marvell.com>
---
Changes in v4:
  - s/AF/PF
  - Add Vamsi's AB

Changes in v3:
  - Only call pci_disable_device() for the PF version. For AF it would
    cause a WARN_ON because pcim_enable_device()'s callback will also
    try to disable.
---
 drivers/vdpa/octeon_ep/octep_vdpa_main.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/vdpa/octeon_ep/octep_vdpa_main.c b/drivers/vdpa/octeon_ep/octep_vdpa_main.c
index f3d4dda4e04c..9b49efd24391 100644
--- a/drivers/vdpa/octeon_ep/octep_vdpa_main.c
+++ b/drivers/vdpa/octeon_ep/octep_vdpa_main.c
@@ -454,6 +454,9 @@ static void octep_vdpa_remove_pf(struct pci_dev *pdev)
 		octep_iounmap_region(pdev, octpf->base, OCTEP_HW_MBOX_BAR);
 
 	octep_vdpa_pf_bar_expand(octpf);
+
+	/* The pf version does not use managed PCI. */
+	pci_disable_device(pdev);
 }
 
 static void octep_vdpa_vf_bar_shrink(struct pci_dev *pdev)
@@ -825,7 +828,7 @@ static int octep_vdpa_probe_pf(struct pci_dev *pdev)
 	struct octep_pf *octpf;
 	int ret;
 
-	ret = pcim_enable_device(pdev);
+	ret = pci_enable_device(pdev);
 	if (ret) {
 		dev_err(dev, "Failed to enable device\n");
 		return ret;
@@ -834,15 +837,17 @@ static int octep_vdpa_probe_pf(struct pci_dev *pdev)
 	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
 	if (ret) {
 		dev_err(dev, "No usable DMA configuration\n");
-		return ret;
+		goto disable_pci;
 	}
 	octpf = devm_kzalloc(dev, sizeof(*octpf), GFP_KERNEL);
-	if (!octpf)
-		return -ENOMEM;
+	if (!octpf) {
+		ret = -ENOMEM;
+		goto disable_pci;
+	}
 
 	ret = octep_iomap_region(pdev, octpf->base, OCTEP_HW_MBOX_BAR);
 	if (ret)
-		return ret;
+		goto disable_pci;
 
 	pci_set_master(pdev);
 	pci_set_drvdata(pdev, octpf);
@@ -856,6 +861,8 @@ static int octep_vdpa_probe_pf(struct pci_dev *pdev)
 
 unmap_region:
 	octep_iounmap_region(pdev, octpf->base, OCTEP_HW_MBOX_BAR);
+disable_pci:
+	pci_disable_device(pdev);
 	return ret;
 }
 
-- 
2.48.1

Re: [PATCH v4] vdpa/octeon_ep: Control PCI dev enabling manually

Posted by Philipp Stanner 7 months, 1 week ago

On Thu, 2025-05-08 at 10:51 +0200, Philipp Stanner wrote:
> PCI region request functions such as pci_request_region() currently
> have
> the problem of becoming sometimes managed functions, if
> pcim_enable_device() instead of pci_enable_device() was called. The
> PCI
> subsystem wants to remove this deprecated behavior from its
> interfaces.
> 
> octeopn_ep enables its device with pcim_enable_device() (for VF. PF
> uses
> manual management), but does so only to get automatic disablement.
> The
> driver wants to manage its PCI resources for VF manually, without
> devres.
> 
> The easiest way not to use automatic resource management at all is by
> also handling device enable- and disablement manually.
> 
> Replace pcim_enable_device() with pci_enable_device(). Add the
> necessary
> calls to pci_disable_device().
> 
> Signed-off-by: Philipp Stanner <phasta@kernel.org>
> Acked-by: Vamsi Attunuru <vattunuru@marvell.com>

Hi again,

this is the last of 12 drivers blocking me from removing a few hundred
lines of broken code from PCI. Would be great if it could be sent to
Linus next merge window.

Can someone take this patch in?

Thx
P.

> ---
> Changes in v4:
>   - s/AF/PF
>   - Add Vamsi's AB
> 
> Changes in v3:
>   - Only call pci_disable_device() for the PF version. For AF it
> would
>     cause a WARN_ON because pcim_enable_device()'s callback will also
>     try to disable.
> ---
>  drivers/vdpa/octeon_ep/octep_vdpa_main.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/vdpa/octeon_ep/octep_vdpa_main.c
> b/drivers/vdpa/octeon_ep/octep_vdpa_main.c
> index f3d4dda4e04c..9b49efd24391 100644
> --- a/drivers/vdpa/octeon_ep/octep_vdpa_main.c
> +++ b/drivers/vdpa/octeon_ep/octep_vdpa_main.c
> @@ -454,6 +454,9 @@ static void octep_vdpa_remove_pf(struct pci_dev
> *pdev)
>  		octep_iounmap_region(pdev, octpf->base,
> OCTEP_HW_MBOX_BAR);
>  
>  	octep_vdpa_pf_bar_expand(octpf);
> +
> +	/* The pf version does not use managed PCI. */
> +	pci_disable_device(pdev);
>  }
>  
>  static void octep_vdpa_vf_bar_shrink(struct pci_dev *pdev)
> @@ -825,7 +828,7 @@ static int octep_vdpa_probe_pf(struct pci_dev
> *pdev)
>  	struct octep_pf *octpf;
>  	int ret;
>  
> -	ret = pcim_enable_device(pdev);
> +	ret = pci_enable_device(pdev);
>  	if (ret) {
>  		dev_err(dev, "Failed to enable device\n");
>  		return ret;
> @@ -834,15 +837,17 @@ static int octep_vdpa_probe_pf(struct pci_dev
> *pdev)
>  	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
>  	if (ret) {
>  		dev_err(dev, "No usable DMA configuration\n");
> -		return ret;
> +		goto disable_pci;
>  	}
>  	octpf = devm_kzalloc(dev, sizeof(*octpf), GFP_KERNEL);
> -	if (!octpf)
> -		return -ENOMEM;
> +	if (!octpf) {
> +		ret = -ENOMEM;
> +		goto disable_pci;
> +	}
>  
>  	ret = octep_iomap_region(pdev, octpf->base,
> OCTEP_HW_MBOX_BAR);
>  	if (ret)
> -		return ret;
> +		goto disable_pci;
>  
>  	pci_set_master(pdev);
>  	pci_set_drvdata(pdev, octpf);
> @@ -856,6 +861,8 @@ static int octep_vdpa_probe_pf(struct pci_dev
> *pdev)
>  
>  unmap_region:
>  	octep_iounmap_region(pdev, octpf->base, OCTEP_HW_MBOX_BAR);
> +disable_pci:
> +	pci_disable_device(pdev);
>  	return ret;
>  }
>

Re: [PATCH v4] vdpa/octeon_ep: Control PCI dev enabling manually

Posted by Michael S. Tsirkin 7 months, 1 week ago

On Thu, May 15, 2025 at 09:14:22AM +0200, Philipp Stanner wrote:
> On Thu, 2025-05-08 at 10:51 +0200, Philipp Stanner wrote:
> > PCI region request functions such as pci_request_region() currently
> > have
> > the problem of becoming sometimes managed functions, if
> > pcim_enable_device() instead of pci_enable_device() was called. The
> > PCI
> > subsystem wants to remove this deprecated behavior from its
> > interfaces.
> > 
> > octeopn_ep enables its device with pcim_enable_device() (for VF. PF
> > uses
> > manual management), but does so only to get automatic disablement.
> > The
> > driver wants to manage its PCI resources for VF manually, without
> > devres.
> > 
> > The easiest way not to use automatic resource management at all is by
> > also handling device enable- and disablement manually.
> > 
> > Replace pcim_enable_device() with pci_enable_device(). Add the
> > necessary
> > calls to pci_disable_device().
> > 
> > Signed-off-by: Philipp Stanner <phasta@kernel.org>
> > Acked-by: Vamsi Attunuru <vattunuru@marvell.com>
> 
> Hi again,
> 
> this is the last of 12 drivers blocking me from removing a few hundred
> lines of broken code from PCI. Would be great if it could be sent to
> Linus next merge window.
> 
> Can someone take this patch in?
> 
> Thx
> P.

I intend to, working on packing things up for -next as we speak.

> > ---
> > Changes in v4:
> >   - s/AF/PF
> >   - Add Vamsi's AB
> > 
> > Changes in v3:
> >   - Only call pci_disable_device() for the PF version. For AF it
> > would
> >     cause a WARN_ON because pcim_enable_device()'s callback will also
> >     try to disable.
> > ---
> >  drivers/vdpa/octeon_ep/octep_vdpa_main.c | 17 ++++++++++++-----
> >  1 file changed, 12 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/vdpa/octeon_ep/octep_vdpa_main.c
> > b/drivers/vdpa/octeon_ep/octep_vdpa_main.c
> > index f3d4dda4e04c..9b49efd24391 100644
> > --- a/drivers/vdpa/octeon_ep/octep_vdpa_main.c
> > +++ b/drivers/vdpa/octeon_ep/octep_vdpa_main.c
> > @@ -454,6 +454,9 @@ static void octep_vdpa_remove_pf(struct pci_dev
> > *pdev)
> >  		octep_iounmap_region(pdev, octpf->base,
> > OCTEP_HW_MBOX_BAR);
> >  
> >  	octep_vdpa_pf_bar_expand(octpf);
> > +
> > +	/* The pf version does not use managed PCI. */
> > +	pci_disable_device(pdev);
> >  }
> >  
> >  static void octep_vdpa_vf_bar_shrink(struct pci_dev *pdev)
> > @@ -825,7 +828,7 @@ static int octep_vdpa_probe_pf(struct pci_dev
> > *pdev)
> >  	struct octep_pf *octpf;
> >  	int ret;
> >  
> > -	ret = pcim_enable_device(pdev);
> > +	ret = pci_enable_device(pdev);
> >  	if (ret) {
> >  		dev_err(dev, "Failed to enable device\n");
> >  		return ret;
> > @@ -834,15 +837,17 @@ static int octep_vdpa_probe_pf(struct pci_dev
> > *pdev)
> >  	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> >  	if (ret) {
> >  		dev_err(dev, "No usable DMA configuration\n");
> > -		return ret;
> > +		goto disable_pci;
> >  	}
> >  	octpf = devm_kzalloc(dev, sizeof(*octpf), GFP_KERNEL);
> > -	if (!octpf)
> > -		return -ENOMEM;
> > +	if (!octpf) {
> > +		ret = -ENOMEM;
> > +		goto disable_pci;
> > +	}
> >  
> >  	ret = octep_iomap_region(pdev, octpf->base,
> > OCTEP_HW_MBOX_BAR);
> >  	if (ret)
> > -		return ret;
> > +		goto disable_pci;
> >  
> >  	pci_set_master(pdev);
> >  	pci_set_drvdata(pdev, octpf);
> > @@ -856,6 +861,8 @@ static int octep_vdpa_probe_pf(struct pci_dev
> > *pdev)
> >  
> >  unmap_region:
> >  	octep_iounmap_region(pdev, octpf->base, OCTEP_HW_MBOX_BAR);
> > +disable_pci:
> > +	pci_disable_device(pdev);
> >  	return ret;
> >  }
> >