>-----Original Message-----
>From: Philipp Stanner <phasta@xxxxxxxxxxx>
>Sent: Thursday, April 17, 2025 6:29 PM
>To: Vamsi Krishna Attunuru <vattunuru@xxxxxxxxxxx>; 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 12: 56 +0000, Vamsi Krishna Attunuru wrote: > > > > -----
>Original Message----- > > From: Philipp Stanner <phasta@ mailbox. org> > >
>Sent: Thursday, April 17, 2025 4: 09 PM > > To: Vamsi Krishna
>
>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.
Either is fine to me, please send the patch, I will test it locally and ack it.
>
>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
>> > >
>>
