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] [thread-next>] [day] [month] [year] [list]

Message-ID: <70a70c74be9ba1a6ae6297ac646fa82600d9296c.camel@redhat.com>
Date: Mon, 12 Aug 2024 20:16:07 +0200
From: Philipp Stanner <pstanner@...hat.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.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

Yo Andy!

On Mon, 2024-08-12 at 18:57 +0300, Andy Shevchenko wrote:
> (Reduced Cc list a lot)
> 
> On Mon, Aug 05, 2024 at 10:01:31AM +0200, Philipp Stanner wrote:
> > pcim_iomap_table() and pcim_iomap_regions_request_all() have been
> > deprecated by the PCI subsystem in commit e354bb84a4c1 ("PCI:
> > Deprecate
> > pcim_iomap_table(), pcim_iomap_regions_request_all()").
> > 
> > Replace these functions with their successors, pcim_iomap() and
> > pcim_request_all_regions()
> 
> Missing period at the end.

ACK

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

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

P.

> 
> 
> > - cptpf->reg_base = pcim_iomap_table(pdev)[PCI_PF_REG_BAR_NUM];
> > + /* Map PF's configuration registers */
> > + cptpf->reg_base = pcim_iomap(pdev, PCI_PF_REG_BAR_NUM, 0);
> > + if (!cptpf->reg_base) {
> > + err = -ENOMEM;
> > + dev_err(dev, "Couldn't ioremap PCI resource 0x%x\n", err);
> > + goto clear_drvdata;
> > + }
> 
> (Yes, I see this).
> 
> ...
> 
> > --- a/drivers/crypto/marvell/octeontx2/otx2_cptvf_main.c
> > +++ b/drivers/crypto/marvell/octeontx2/otx2_cptvf_main.c
> 
> Ditto. here.
>