| Message ID | 20240805080150.9739-6-pstanner@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Remove pcim_iomap_regions_request_all() | expand |
(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. ... > - /* 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? Note, the resource may be requested, OR mapped, OR both. In accordance with the naming above I assume that this is not equivalent change with potential breakages. > - 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.
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. >
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.
diff --git a/drivers/crypto/marvell/octeontx2/otx2_cptpf_main.c b/drivers/crypto/marvell/octeontx2/otx2_cptpf_main.c index 400e36d9908f..94d0e73e42de 100644 --- a/drivers/crypto/marvell/octeontx2/otx2_cptpf_main.c +++ b/drivers/crypto/marvell/octeontx2/otx2_cptpf_main.c @@ -739,18 +739,22 @@ static int otx2_cptpf_probe(struct pci_dev *pdev, dev_err(dev, "Unable to get usable DMA configuration\n"); goto clear_drvdata; } - /* 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; } pci_set_master(pdev); pci_set_drvdata(pdev, cptpf); cptpf->pdev = pdev; - 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; + } /* Check if AF driver is up, otherwise defer probe */ err = cpt_is_pf_usable(cptpf); diff --git a/drivers/crypto/marvell/octeontx2/otx2_cptvf_main.c b/drivers/crypto/marvell/octeontx2/otx2_cptvf_main.c index 527d34cc258b..d0b6ee901f62 100644 --- a/drivers/crypto/marvell/octeontx2/otx2_cptvf_main.c +++ b/drivers/crypto/marvell/octeontx2/otx2_cptvf_main.c @@ -358,9 +358,8 @@ static int otx2_cptvf_probe(struct pci_dev *pdev, dev_err(dev, "Unable to get usable DMA configuration\n"); goto clear_drvdata; } - /* Map VF's configuration registers */ - ret = pcim_iomap_regions_request_all(pdev, 1 << PCI_PF_REG_BAR_NUM, - OTX2_CPTVF_DRV_NAME); + + ret = pcim_request_all_regions(pdev, OTX2_CPTVF_DRV_NAME); if (ret) { dev_err(dev, "Couldn't get PCI resources 0x%x\n", ret); goto clear_drvdata; @@ -369,7 +368,13 @@ static int otx2_cptvf_probe(struct pci_dev *pdev, pci_set_drvdata(pdev, cptvf); cptvf->pdev = pdev; - cptvf->reg_base = pcim_iomap_table(pdev)[PCI_PF_REG_BAR_NUM]; + /* Map VF's configuration registers */ + cptvf->reg_base = pcim_iomap(pdev, PCI_PF_REG_BAR_NUM, 0); + if (!cptvf->reg_base) { + ret = -ENOMEM; + dev_err(dev, "Couldn't ioremap PCI resource 0x%x\n", ret); + goto clear_drvdata; + } otx2_cpt_set_hw_caps(pdev, &cptvf->cap_flag);
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() Signed-off-by: Philipp Stanner <pstanner@redhat.com> --- drivers/crypto/marvell/octeontx2/otx2_cptpf_main.c | 14 +++++++++----- drivers/crypto/marvell/octeontx2/otx2_cptvf_main.c | 13 +++++++++---- 2 files changed, 18 insertions(+), 9 deletions(-)