vdpa/octeon_ep: Use non-hybrid PCI devres API

PATCH In review Philipp Stanner 3 participants 6 messages Apr 17, 2025

Subsystems (2)

virtio core Marvell octeon endpoint virtio data path accelerator

lore

Share & tools

lore

Philipp Stanner

author

2025-04-17 08:03 UTC #0 lore raw

PATCH 1/1 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. 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 <[email protected]>

---
 drivers/vdpa/octeon_ep/octep_vdpa_main.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

drivers/vdpa/octeon_ep/octep_vdpa_main.c +1 -3

index f3d4dda4e04c..e0da6367661e 100644
@@ -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)

Signature · 1 line

2.48.1

Vamsi Krishna Attunuru

reviewer

2025-04-17 09:02 UTC #1 lore raw

Quoted 28 lines

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

Quoted 7 lines

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.

Quoted 40 lines

Signed-off-by Philipp Stanner <[email protected]>

---
drivers/vdpa/octeon_ep/octep_vdpa_main.c | 4 +---

1 file changed, 1 insertion(+), 3 deletions(-)

drivers/vdpa/octeon_ep/octep_vdpa_main.c +1 -1

index f3d4dda4e04c..e0da6367661e 100644
@@ -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;

drivers/vdpa/octeon_ep/octep_vdpa_main.c +0 -1

@@ -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]);

drivers/vdpa/octeon_ep/octep_vdpa_main.c +0 -1

-	pci_release_region(pdev, bar);

}

static void octep_vdpa_pf_bar_shrink(struct octep_pf *octpf) --
2.48.1

Philipp Stanner

2025-04-17 10:39 UTC #2 lore raw

Quoted 40 lines · Vamsi Krishna Attunuru

Quoted 40 lines
-----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.

Quoted 6 lines

Quoted 5 lines
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,

Quoted 7 lines

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.

Quoted 51 lines

Quoted 50 lines
Signed-off-by Philipp Stanner <[email protected]>
---
drivers/vdpa/octeon_ep/octep_vdpa_main.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
drivers/vdpa/octeon_ep/octep_vdpa_main.c +1 -1
index f3d4dda4e04c..e0da6367661e 100644
@@ -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;
drivers/vdpa/octeon_ep/octep_vdpa_main.c +0 -0
@@ -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);
drivers/vdpa/octeon_ep/octep_vdpa_main.c +0 -1
-		pci_release_region(pdev, bar);
		ret = -ENOMEM;
	}
drivers/vdpa/octeon_ep/octep_vdpa_main.c +0 -0
@@ -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]);
drivers/vdpa/octeon_ep/octep_vdpa_main.c +0 -1
-	pci_release_region(pdev, bar);
}
static void octep_vdpa_pf_bar_shrink(struct octep_pf *octpf) --
2.48.1

Vamsi Krishna Attunuru

reviewer

2025-04-17 12:56 UTC #3 lore raw

Quoted 69 lines

-----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
Quoted 8 lines · Vamsi Krishna Attunuru
Quoted 8 lines
-----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]>;
Quoted 20 lines
Quoted 20 lines
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.
Quoted 9 lines
Quoted 8 lines
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,
Quoted 6 lines
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.

Quoted 3 lines

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.

Quoted 63 lines

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.
Quoted 17 lines
Quoted 17 lines
Signed-off-by Philipp Stanner <[email protected]>
---
drivers/vdpa/octeon_ep/octep_vdpa_main.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
drivers/vdpa/octeon_ep/octep_vdpa_main.c +0 -0
index f3d4dda4e04c..e0da6367661e 100644
@@ -391,7 +391,7 @@ static int octep_iomap_region(struct pci_dev
 *pdev,
 u8 __iomem **tbl, u8 bar)  {
 	int ret;
 
Quoted 6 lines
Quoted 6 lines
drivers/vdpa/octeon_ep/octep_vdpa_main.c +1 -1
-	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);
Quoted 23 lines
Quoted 22 lines
		return ret;
drivers/vdpa/octeon_ep/octep_vdpa_main.c +0 -1
@@ -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;
	}
drivers/vdpa/octeon_ep/octep_vdpa_main.c +0 -0
@@ -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]);
drivers/vdpa/octeon_ep/octep_vdpa_main.c +0 -1
-	pci_release_region(pdev, bar);
}
static void octep_vdpa_pf_bar_shrink(struct octep_pf *octpf) --
2.48.1

Philipp Stanner

2025-04-17 12:59 UTC #4 lore raw

Quoted 101 lines · Vamsi Krishna Attunuru

Quoted 87 lines
-----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: >
Quoted 1 line
Quoted 1 line
Quoted 1 line
-----
Original Message----- > > From: Philipp Stanner <phasta@ kernel. org> > >
Sent: Thursday, April 17, 2025 1: 32 PM > > To: Srujana
Quoted 10 lines · Vamsi Krishna Attunuru
Quoted 10 lines
-----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]>;
Quoted 32 lines
Quoted 32 lines
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.
Quoted 6 lines
Quoted 5 lines
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,
Quoted 10 lines
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.
Quoted 4 lines
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

Quoted 8 lines

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

Quoted 77 lines

Quoted 76 lines
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.
Quoted 18 lines
Quoted 18 lines
Signed-off-by Philipp Stanner <[email protected]>
---
drivers/vdpa/octeon_ep/octep_vdpa_main.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
drivers/vdpa/octeon_ep/octep_vdpa_main.c +0 -0
index f3d4dda4e04c..e0da6367661e 100644
@@ -391,7 +391,7 @@ static int octep_iomap_region(struct
pci_dev
*pdev,
u8 __iomem **tbl, u8 bar)  {
	int ret;
Quoted 7 lines
Quoted 7 lines
drivers/vdpa/octeon_ep/octep_vdpa_main.c +0 -1
-	ret = pci_request_region(pdev, bar,
OCTEP_VDPA_DRIVER_NAME);
drivers/vdpa/octeon_ep/octep_vdpa_main.c +1 -0
+	ret = pcim_request_region(pdev, bar,
OCTEP_VDPA_DRIVER_NAME);
	if (ret) {
		dev_err(&pdev->dev, "Failed to request BAR:%u
region\n",
bar);
Quoted 28 lines
Quoted 27 lines
		return ret;
drivers/vdpa/octeon_ep/octep_vdpa_main.c +0 -0
@@ -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);
drivers/vdpa/octeon_ep/octep_vdpa_main.c +0 -1
-		pci_release_region(pdev, bar);
		ret = -ENOMEM;
	}
drivers/vdpa/octeon_ep/octep_vdpa_main.c +0 -0
@@ -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]);
drivers/vdpa/octeon_ep/octep_vdpa_main.c +0 -1
-	pci_release_region(pdev, bar);
}
static void octep_vdpa_pf_bar_shrink(struct octep_pf *octpf) --
2.48.1

Vamsi Krishna Attunuru

reviewer

2025-04-17 13:11 UTC #5 lore raw

Quoted 116 lines

-----Original Message-----
From: Philipp Stanner <[email protected]>
Sent: Thursday, April 17, 2025 6:29 PM
To: Vamsi Krishna Attunuru <[email protected]>; [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 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
Quoted 87 lines · Vamsi Krishna Attunuru
Quoted 74 lines
-----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: >
Quoted 1 line
Quoted 1 line
Quoted 1 line
-----
Original Message----- > > From: Philipp Stanner <phasta@ kernel. org> > >
Sent: Thursday, April 17, 2025 1: 32 PM > > To: Srujana
Quoted 8 lines · Vamsi Krishna Attunuru
Quoted 8 lines
-----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]>;
Quoted 21 lines
Quoted 21 lines
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.
Quoted 9 lines
Quoted 8 lines
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,
Quoted 7 lines
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.
Quoted 3 lines
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
Quoted 7 lines
[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.

Quoted 5 lines

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.

Quoted 71 lines

P.
Quoted 68 lines
Quoted 67 lines
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.
Quoted 17 lines
Quoted 17 lines
Signed-off-by Philipp Stanner <[email protected]>
---
drivers/vdpa/octeon_ep/octep_vdpa_main.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
drivers/vdpa/octeon_ep/octep_vdpa_main.c +0 -0
index f3d4dda4e04c..e0da6367661e 100644
@@ -391,7 +391,7 @@ static int octep_iomap_region(struct pci_dev
 *pdev,
 u8 __iomem **tbl, u8 bar)  {
 	int ret;
 
Quoted 6 lines
Quoted 6 lines
drivers/vdpa/octeon_ep/octep_vdpa_main.c +1 -1
-	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);
Quoted 24 lines
Quoted 23 lines
		return ret;
drivers/vdpa/octeon_ep/octep_vdpa_main.c +0 -1
@@ -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;
	}
drivers/vdpa/octeon_ep/octep_vdpa_main.c +0 -0
@@ -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]);
drivers/vdpa/octeon_ep/octep_vdpa_main.c +0 -1
-	pci_release_region(pdev, bar);
}
static void octep_vdpa_pf_bar_shrink(struct octep_pf *octpf) --
2.48.1