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
