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] [day] [month] [year] [list]
Message-ID:
 <MW4PR18MB524432BD45DD5E2AC1F3E94DA6BC2@MW4PR18MB5244.namprd18.prod.outlook.com>
Date: Thu, 17 Apr 2025 13:11:10 +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 6:29 PM
>To: Vamsi Krishna Attunuru <vattunuru@...vell.com>; 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 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@...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.
>
>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@...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
>> > >
>>