>-----Original Message-----
>From: Philipp Stanner <[email protected]>
>Sent: Thursday, April 17, 2025 4:09 PM
>To: Vamsi Krishna Attunuru <[email protected]>; Philipp Stanner
><[email protected]>; Srujana Challa <[email protected]>; Michael S.
>Tsirkin <[email protected]>; Jason Wang <[email protected]>; Xuan
>Zhuo <[email protected]>; Eugenio Pérez
><[email protected]>; Shijith Thotton <[email protected]>; Dan
>Carpenter <[email protected]>; Satha Koteswara Rao Kottidi
><[email protected]>
>Cc: [email protected]; [email protected]
>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 <[email protected]>
>> > Sent: Thursday, April 17, 2025 1:32 PM
>> > To: Srujana Challa <[email protected]>; Vamsi Krishna Attunuru
>> > <[email protected]>; Michael S. Tsirkin <[email protected]>; Jason
>> > Wang <[email protected]>; Xuan Zhuo
><[email protected]>;
>> > Eugenio Pérez <[email protected]>; Shijith Thotton
>> > <[email protected]>; Dan Carpenter <[email protected]>;
>> > Philipp Stanner <[email protected]>; Satha Koteswara Rao Kottidi
>> > <[email protected]>
>> > Cc: [email protected]; [email protected]
>> > 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.
>
>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 <[email protected]>
>> > ---
>> > 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
>>
