Re: [EXTERNAL] [PATCH] vdpa/octeon_ep: Use non-hybrid PCI devres API

[Philipp Stanner <phasta@mailbox.org>]

On Thu, 2025-04-17 at 09:02 +0000, Vamsi Krishna Attunuru wrote:
> 
> 
> > -----Original Message-----
> > From: Philipp Stanner <phasta@kernel.org>
> > Sent: Thursday, April 17, 2025 1:32 PM
> > To: Srujana Challa <schalla@marvell.com>; Vamsi Krishna Attunuru
> > <vattunuru@marvell.com>; Michael S. Tsirkin <mst@redhat.com>; Jason
> > Wang <jasowang@redhat.com>; Xuan Zhuo <xuanzhuo@linux.alibaba.com>;
> > Eugenio Pérez <eperezma@redhat.com>; Shijith Thotton
> > <sthotton@marvell.com>; Dan Carpenter <dan.carpenter@linaro.org>;
> > Philipp Stanner <phasta@kernel.org>; Satha Koteswara Rao Kottidi
> > <skoteshwar@marvell.com>
> > Cc: virtualization@lists.linux.dev; linux-kernel@vger.kernel.org
> > Subject: [EXTERNAL] [PATCH] vdpa/octeon_ep: Use non-hybrid PCI
> > devres
> > API
> > 
> > octeon enables its PCI device with pcim_enable_device(). This,
> > implicitly,
> > switches the function pci_request_region() into managed mode, where
> > it
> > becomes a devres function. The PCI subsystem wants to remove this
> > hybrid
> > nature from its interfaces. 
> > octeon enables its PCI device with pcim_enable_device(). This,
> > implicitly,
> > switches the function pci_request_region() 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_region() are unnecessary.
> > 
> > Remove the calls to pci_release_region().
> > 
> > Replace the call to sometimes-managed pci_request_region() with one
> > to the
> > always-managed pcim_request_region().
> 

Hi,

> Thanks you, Philipps, for the patch. The Octeon EP driver does not
> use managed calls for handling resource regions. 
> This is because the PCIe PF function reduces its resources to share
> them with its VFs and later restores them to original size
> once the VFs are released. Due to this resource sharing requirement,
> the driver cannot use the always-managed request
> regions calls.

so this would mean that the driver is already broken.
pci_request_region() in your driver is always-managed since you call
pcim_enable_device(). Or am I missing something?

The only way for you to, currently, have non-managed versions is by
using pci_enable_device() instead and then doing pci_disable_device()
manually in remove() and the other appropriate places.

This patch should not change behavior in any way.

If you're sure that having no management is not a problem, then we
could simply drop this patch and I later remove the hybrid feature from
PCI. Then your call to pci_request_region() will become unmanaged, even
if you keep the pcim_enable_device().

Tell me if you have a preference.

P.

> 
> > 
> > Signed-off-by: Philipp Stanner <phasta@kernel.org>
> > ---
> > drivers/vdpa/octeon_ep/octep_vdpa_main.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/vdpa/octeon_ep/octep_vdpa_main.c
> > b/drivers/vdpa/octeon_ep/octep_vdpa_main.c
> > index f3d4dda4e04c..e0da6367661e 100644
> > --- a/drivers/vdpa/octeon_ep/octep_vdpa_main.c
> > +++ b/drivers/vdpa/octeon_ep/octep_vdpa_main.c
> > @@ -391,7 +391,7 @@ static int octep_iomap_region(struct pci_dev
> > *pdev,
> > u8 __iomem **tbl, u8 bar)  {
> > 	int ret;
> > 
> > -	ret = pci_request_region(pdev, bar,
> > OCTEP_VDPA_DRIVER_NAME);
> > +	ret = pcim_request_region(pdev, bar,
> > OCTEP_VDPA_DRIVER_NAME);
> > 	if (ret) {
> > 		dev_err(&pdev->dev, "Failed to request BAR:%u
> > region\n",
> > bar);
> > 		return ret;
> > @@ -400,7 +400,6 @@ static int octep_iomap_region(struct pci_dev
> > *pdev,
> > u8 __iomem **tbl, u8 bar)
> > 	tbl[bar] = pci_iomap(pdev, bar, pci_resource_len(pdev,
> > bar));
> > 	if (!tbl[bar]) {
> > 		dev_err(&pdev->dev, "Failed to iomap BAR:%u\n",
> > bar);
> > -		pci_release_region(pdev, bar);
> > 		ret = -ENOMEM;
> > 	}
> > 
> > @@ -410,7 +409,6 @@ static int octep_iomap_region(struct pci_dev
> > *pdev,
> > u8 __iomem **tbl, u8 bar)  static void octep_iounmap_region(struct
> > pci_dev
> > *pdev, u8 __iomem **tbl, u8 bar)  {
> > 	pci_iounmap(pdev, tbl[bar]);
> > -	pci_release_region(pdev, bar);
> > }
> > 
> > static void octep_vdpa_pf_bar_shrink(struct octep_pf *octpf)
> > --
> > 2.48.1
>