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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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