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

[Philipp Stanner]

On Thu, 2025-04-17 at 12:56 +0000, Vamsi Krishna Attunuru wrote:
> 
> 
> > -----Original Message-----
> > From: Philipp Stanner <phasta@xxxxxxxxxxx>
> > Sent: Thursday, April 17, 2025 4:09 PM
> > To: Vamsi Krishna Attunuru <vattunuru@xxxxxxxxxxx>; Philipp Stanner
> > <phasta@xxxxxxxxxx>; Srujana Challa <schalla@xxxxxxxxxxx>; Michael
> > S.
> > Tsirkin <mst@xxxxxxxxxx>; Jason Wang <jasowang@xxxxxxxxxx>; Xuan
> > Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>; Eugenio Pérez
> > <eperezma@xxxxxxxxxx>; Shijith Thotton <sthotton@xxxxxxxxxxx>; Dan
> > Carpenter <dan.carpenter@xxxxxxxxxx>; Satha Koteswara Rao Kottidi
> > <skoteshwar@xxxxxxxxxxx>
> > Cc: virtualization@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> > Subject: Re: [EXTERNAL] [PATCH] vdpa/octeon_ep: Use non-hybrid PCI
> > devres API
> > 
> > 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
> > 
> > On Thu, 2025-04-17 at 09:02 +0000, Vamsi Krishna Attunuru wrote:
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Philipp Stanner <phasta@xxxxxxxxxx>
> > > > Sent: Thursday, April 17, 2025 1:32 PM
> > > > To: Srujana Challa <schalla@xxxxxxxxxxx>; Vamsi Krishna
> > > > Attunuru
> > > > <vattunuru@xxxxxxxxxxx>; Michael S. Tsirkin <mst@xxxxxxxxxx>;
> > > > Jason
> > > > Wang <jasowang@xxxxxxxxxx>; Xuan Zhuo
> > <xuanzhuo@xxxxxxxxxxxxxxxxx>;
> > > > Eugenio Pérez <eperezma@xxxxxxxxxx>; Shijith Thotton
> > > > <sthotton@xxxxxxxxxxx>; Dan Carpenter
> > > > <dan.carpenter@xxxxxxxxxx>;
> > > > Philipp Stanner <phasta@xxxxxxxxxx>; Satha Koteswara Rao
> > > > Kottidi
> > > > <skoteshwar@xxxxxxxxxxx>
> > > > Cc: virtualization@xxxxxxxxxxxxxxx;
> > > > linux-kernel@xxxxxxxxxxxxxxx
> > > > 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?
> 
> Driver is not actually broken. Yes, pci_request_region is always
> managed in the driver due to pcim_enable_device().
> But inside pcim_request_region(), res->type is considered as
> PCIM_ADDR_DEVRES_TYPE_REGION and
> pcim_addr_resource_release() releases entire bar. Whereas my driver
> needs type=PCIM_ADDR_DEVRES_TYPE_MAPPING
> so that pci_iounmap() get called. If I use this patch, driver reload
> was failing for VF devices with below errors
> 
> [90662.789921] octep_vdpa 0000:17:02.0: BAR 4: can't reserve [mem
> 0x207ff0000000-0x207ff07fffff 64bit pref]
> [90662.789922] octep_vdpa 0000:17:02.0: Failed to request BAR:4
> region
> 
> As you suggested below, I prefer to have non-managed version (use
> pci_request_region()) and explicit remove() at required
> places for now.

Would you then mind replacing pcim_enable_device() with
pci_enable_device()? Should I send you a patch for that or do you want
to do that yourself?

That should do the trick. 

P.

> 
> > 
> > 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@xxxxxxxxxx>
> > > > ---
> > > > 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
> > > 
>