linux-kernel - Re: [PATCH v2 04/10] crypto: marvell - replace deprecated PCI functions

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: <ZrpnGTO5XGkk0TET@smile.fi.intel.com>
Date: Mon, 12 Aug 2024 22:48:41 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Philipp Stanner <pstanner@...hat.com>
Cc: Damien Le Moal <dlemoal@...nel.org>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
	Jonathan Cameron <Jonathan.Cameron@...wei.com>,
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
	linux-kernel@...r.kernel.org, linux-crypto@...r.kernel.org,
	linux-pci@...r.kernel.org
Subject: Re: [PATCH v2 04/10] crypto: marvell - replace deprecated PCI
 functions

On Mon, Aug 12, 2024 at 08:16:07PM +0200, Philipp Stanner wrote:
> On Mon, 2024-08-12 at 18:57 +0300, Andy Shevchenko wrote:
> > On Mon, Aug 05, 2024 at 10:01:31AM +0200, Philipp Stanner wrote:

...

> > > - /* Map PF's configuration registers */
> > > - err = pcim_iomap_regions_request_all(pdev, 1 <<
> > > PCI_PF_REG_BAR_NUM,
> > > -      OTX2_CPT_DRV_NAME);
> > > + err = pcim_request_all_regions(pdev, OTX2_CPT_DRV_NAME);
> > >   if (err) {
> > > - dev_err(dev, "Couldn't get PCI resources 0x%x\n", err);
> > > + dev_err(dev, "Couldn't request PCI resources 0x%x\n", err);
> > >   goto clear_drvdata;
> > >   }
> > 
> > I haven't looked at the implementation differences of those two, but
> > would it
> > be really an equivalent change now?
> 
> Well, if I weren't convinced that it's 100% equivalent I weren't
> posting it :)
> 
> pcim_iomap_regions_request_all() already uses
> pcim_request_all_regions() internally.
> 
> The lines you quote here are not equivalent to the old version, but in
> combination with the following lines the functionality is identical:
>    1. Request all regions
>    2. ioremap BAR OTX2_CPT_BAR_NUM
> 
> > 
> > Note, the resource may be requested, OR mapped, OR both.
> 
> Negative, that is not how pcim_iomap_regions_request_all() works.

(I was talking from the generic resource management in the kernel perspective)

> That
> overengineered function requests *all* PCI BARs and ioremap()s those
> specified in the bit mask.

> If you don't set a bit, you'll request all regions and ioremap() none.
> However you choose to use it, it will always request all regions and
> map between 0 and PCI_STD_NUM_BARS.

Oh, thanks to you we are getting rid of this awfully interfaced API!

> > In accordance with the
> > naming above I assume that this is not equivalent change with
> > potential
> > breakages.
> 
> The nasty thing of us in PCI is that you more or less already use the
> code above anyways, because in v6.11 I reworked most of
> drivers/pci/devres.c, so pcim_iomap_regions_request_all() uses both
> pcim_request_all_regions() and pcim_iomap() in precisely that order
> already.
> 
> The only hypothetical breakages which are not already in v6.11 anyways
> I could imagine are:
>  * Someone complaining about changed error codes in case of failure
>  * Someone racing between the calls to pcim_request_all_regions() and
>    pcim_iomap(). But that's why the region request is actually there in
>    the first place, to block off drivers competing for the same
>    resource. And AFAIU probe() functions don't race anyways.
> 
> Anything I might have overlooked?

Dunno, but the above sounds like a good explanation.

-- 
With Best Regards,
Andy Shevchenko