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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <MW4PR18MB5244B9A5E105A514BB3D5272A6BC2@MW4PR18MB5244.namprd18.prod.outlook.com>
Date: Thu, 17 Apr 2025 12:56:14 +0000
From: Vamsi Krishna Attunuru <vattunuru@...vell.com>
To: "phasta@...nel.org" <phasta@...nel.org>,
        Srujana Challa
	<schalla@...vell.com>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Jason Wang
	<jasowang@...hat.com>,
        Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
        Eugenio Pérez <eperezma@...hat.com>,
        Shijith Thotton
	<sthotton@...vell.com>,
        Dan Carpenter <dan.carpenter@...aro.org>,
        Satha
 Koteswara Rao Kottidi <skoteshwar@...vell.com>
CC: "virtualization@...ts.linux.dev" <virtualization@...ts.linux.dev>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [EXTERNAL] [PATCH] vdpa/octeon_ep: Use non-hybrid PCI devres API



>-----Original Message-----
>From: Philipp Stanner <phasta@...lbox.org>
>Sent: Thursday, April 17, 2025 4:09 PM
>To: Vamsi Krishna Attunuru <vattunuru@...vell.com>; Philipp Stanner
><phasta@...nel.org>; Srujana Challa <schalla@...vell.com>; Michael S.
>Tsirkin <mst@...hat.com>; Jason Wang <jasowang@...hat.com>; Xuan
>Zhuo <xuanzhuo@...ux.alibaba.com>; Eugenio Pérez
><eperezma@...hat.com>; Shijith Thotton <sthotton@...vell.com>; Dan
>Carpenter <dan.carpenter@...aro.org>; Satha Koteswara Rao Kottidi
><skoteshwar@...vell.com>
>Cc: virtualization@...ts.linux.dev; linux-kernel@...r.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@...nel.org>
>> > Sent: Thursday, April 17, 2025 1:32 PM
>> > To: Srujana Challa <schalla@...vell.com>; Vamsi Krishna Attunuru
>> > <vattunuru@...vell.com>; Michael S. Tsirkin <mst@...hat.com>; Jason
>> > Wang <jasowang@...hat.com>; Xuan Zhuo
><xuanzhuo@...ux.alibaba.com>;
>> > Eugenio Pérez <eperezma@...hat.com>; Shijith Thotton
>> > <sthotton@...vell.com>; Dan Carpenter <dan.carpenter@...aro.org>;
>> > Philipp Stanner <phasta@...nel.org>; Satha Koteswara Rao Kottidi
>> > <skoteshwar@...vell.com>
>> > Cc: virtualization@...ts.linux.dev; linux-kernel@...r.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@...nel.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
>>