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

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

[Philipp Stanner]

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().

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

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

[Vamsi Krishna Attunuru]

>-----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().

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.

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

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

[Philipp Stanner]

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
> 

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

[Vamsi Krishna Attunuru]

>-----Original Message-----
>From: Philipp Stanner <phasta@mailbox.org>
>Sent: Thursday, April 17, 2025 4:09 PM
>To: Vamsi Krishna Attunuru <vattunuru@marvell.com>; Philipp Stanner
><phasta@kernel.org>; Srujana Challa <schalla@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>; Satha Koteswara Rao Kottidi
><skoteshwar@marvell.com>
>Cc: virtualization@lists.linux.dev; linux-kernel@vger.kernel.org
>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@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?

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

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@mailbox.org>
> > Sent: Thursday, April 17, 2025 4:09 PM
> > To: Vamsi Krishna Attunuru <vattunuru@marvell.com>; Philipp Stanner
> > <phasta@kernel.org>; Srujana Challa <schalla@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>; Satha Koteswara Rao Kottidi
> > <skoteshwar@marvell.com>
> > Cc: virtualization@lists.linux.dev; linux-kernel@vger.kernel.org
> > 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@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?
> 
> 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@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
> > > 
> 

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

[Vamsi Krishna Attunuru]

>-----Original Message-----
>From: Philipp Stanner <phasta@mailbox.org>
>Sent: Thursday, April 17, 2025 6:29 PM
>To: Vamsi Krishna Attunuru <vattunuru@marvell.com>; phasta@kernel.org;
>Srujana Challa <schalla@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>; Satha Koteswara Rao Kottidi
><skoteshwar@marvell.com>
>Cc: virtualization@lists.linux.dev; linux-kernel@vger.kernel.org
>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@mailbox.org>
>> > Sent: Thursday, April 17, 2025 4:09 PM
>> > To: Vamsi Krishna Attunuru <vattunuru@marvell.com>; Philipp Stanner
>> > <phasta@kernel.org>; Srujana Challa <schalla@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>; Satha Koteswara Rao Kottidi
>> > <skoteshwar@marvell.com>
>> > Cc: virtualization@lists.linux.dev; linux-kernel@vger.kernel.org
>> > 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@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?
>>
>> 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@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
>> > >
>>