| Message ID | 20241025145959.185373-1-pstanner@redhat.com |
|---|---|
| Headers | show |
| Series | Remove pcim_iomap_regions_request_all() | expand |
On Fri, 25 Oct 2024, Philipp Stanner wrote: > In order to remove the deprecated function > pcim_iomap_regions_request_all(), a few drivers need an interface to > request all BARs a PCI-Device offers. Hi Philipp, I don't know why you put that dash there, it looks a bit odd. Other than that, Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> -- i. > Make pcim_request_all_regions() a public interface. > > Signed-off-by: Philipp Stanner <pstanner@redhat.com> > Reviewed-by: Damien Le Moal <dlemoal@kernel.org> > --- > drivers/pci/devres.c | 3 ++- > include/linux/pci.h | 1 + > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c > index b133967faef8..2a64da5c91fb 100644 > --- a/drivers/pci/devres.c > +++ b/drivers/pci/devres.c > @@ -939,7 +939,7 @@ static void pcim_release_all_regions(struct pci_dev *pdev) > * desired, release individual regions with pcim_release_region() or all of > * them at once with pcim_release_all_regions(). > */ > -static int pcim_request_all_regions(struct pci_dev *pdev, const char *name) > +int pcim_request_all_regions(struct pci_dev *pdev, const char *name) > { > int ret; > int bar; > @@ -957,6 +957,7 @@ static int pcim_request_all_regions(struct pci_dev *pdev, const char *name) > > return ret; > } > +EXPORT_SYMBOL(pcim_request_all_regions); > > /** > * pcim_iomap_regions_request_all - Request all BARs and iomap specified ones > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 573b4c4c2be6..3b151c8331e5 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -2293,6 +2293,7 @@ static inline void pci_fixup_device(enum pci_fixup_pass pass, > struct pci_dev *dev) { } > #endif > > +int pcim_request_all_regions(struct pci_dev *pdev, const char *name); > void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned long maxlen); > void __iomem *pcim_iomap_region(struct pci_dev *pdev, int bar, > const char *name); >
On Fri, 25 Oct 2024, 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(). > > Signed-off-by: Philipp Stanner <pstanner@redhat.com> > Acked-by: Kalle Valo <kvalo@kernel.org> > --- > drivers/net/wireless/intel/iwlwifi/pcie/trans.c | 16 ++++------------ > 1 file changed, 4 insertions(+), 12 deletions(-) > > diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c > index 3b9943eb6934..4b41613ad89d 100644 > --- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c > +++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c > @@ -3533,7 +3533,6 @@ struct iwl_trans *iwl_trans_pcie_alloc(struct pci_dev *pdev, > struct iwl_trans_pcie *trans_pcie, **priv; > struct iwl_trans *trans; > int ret, addr_size; > - void __iomem * const *table; > u32 bar0; > > /* reassign our BAR 0 if invalid due to possible runtime PM races */ > @@ -3659,22 +3658,15 @@ struct iwl_trans *iwl_trans_pcie_alloc(struct pci_dev *pdev, > } > } > > - ret = pcim_iomap_regions_request_all(pdev, BIT(0), DRV_NAME); > + ret = pcim_request_all_regions(pdev, DRV_NAME); > if (ret) { > - dev_err(&pdev->dev, "pcim_iomap_regions_request_all failed\n"); > + dev_err(&pdev->dev, "pcim_request_all_regions failed\n"); > goto out_no_pci; > } > > - table = pcim_iomap_table(pdev); > - if (!table) { > - dev_err(&pdev->dev, "pcim_iomap_table failed\n"); > - ret = -ENOMEM; > - goto out_no_pci; > - } > - > - trans_pcie->hw_base = table[0]; > + trans_pcie->hw_base = pcim_iomap(pdev, 0, 0); > if (!trans_pcie->hw_base) { > - dev_err(&pdev->dev, "couldn't find IO mem in first BAR\n"); > + dev_err(&pdev->dev, "pcim_iomap failed\n"); This seems a step backwards as a human readable English error message was replaced with a reference to a function name.
On Fri, 25 Oct 2024, Philipp Stanner wrote: > On Fri, 2024-10-25 at 19:11 +0300, Ilpo Järvinen wrote: > > On Fri, 25 Oct 2024, Philipp Stanner wrote: > > > > > On Fri, 2024-10-25 at 18:31 +0300, Ilpo Järvinen wrote: > > > > On Fri, 25 Oct 2024, 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(). > > > > > > > > > > Signed-off-by: Philipp Stanner <pstanner@redhat.com> > > > > > Acked-by: Kalle Valo <kvalo@kernel.org> > > > > > --- > > > > > drivers/net/wireless/intel/iwlwifi/pcie/trans.c | 16 ++++----- > > > > > ---- > > > > > --- > > > > > 1 file changed, 4 insertions(+), 12 deletions(-) > > > > > > > > > > diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c > > > > > b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c > > > > > index 3b9943eb6934..4b41613ad89d 100644 > > > > > --- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c > > > > > +++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c > > > > > @@ -3533,7 +3533,6 @@ struct iwl_trans > > > > > *iwl_trans_pcie_alloc(struct > > > > > pci_dev *pdev, > > > > > struct iwl_trans_pcie *trans_pcie, **priv; > > > > > struct iwl_trans *trans; > > > > > int ret, addr_size; > > > > > - void __iomem * const *table; > > > > > u32 bar0; > > > > > > > > > > /* reassign our BAR 0 if invalid due to possible > > > > > runtime > > > > > PM races */ > > > > > @@ -3659,22 +3658,15 @@ struct iwl_trans > > > > > *iwl_trans_pcie_alloc(struct pci_dev *pdev, > > > > > } > > > > > } > > > > > > > > > > - ret = pcim_iomap_regions_request_all(pdev, BIT(0), > > > > > DRV_NAME); > > > > > + ret = pcim_request_all_regions(pdev, DRV_NAME); > > > > > if (ret) { > > > > > - dev_err(&pdev->dev, > > > > > "pcim_iomap_regions_request_all failed\n"); > > > > > + dev_err(&pdev->dev, "pcim_request_all_regions > > > > > failed\n"); > > > > > goto out_no_pci; > > > > > } > > > > > > > > > > - table = pcim_iomap_table(pdev); > > > > > - if (!table) { > > > > > - dev_err(&pdev->dev, "pcim_iomap_table > > > > > failed\n"); > > > > > - ret = -ENOMEM; > > > > > - goto out_no_pci; > > > > > - } > > > > > - > > > > > - trans_pcie->hw_base = table[0]; > > > > > + trans_pcie->hw_base = pcim_iomap(pdev, 0, 0); > > > > > if (!trans_pcie->hw_base) { > > > > > - dev_err(&pdev->dev, "couldn't find IO mem in > > > > > first > > > > > BAR\n"); > > > > > + dev_err(&pdev->dev, "pcim_iomap failed\n"); > > > > > > > > This seems a step backwards as a human readable English error > > > > message > > > > was > > > > replaced with a reference to a function name. > > > > > > I think it's still an improvement because "couldn't find IO mem in > > > first BAR" is a nonsensical statement. What the author probably > > > meant > > > was: "Couldn't find first BAR's IO mem in magic pci_iomap_table" ;) > > > > Well, that's just spelling things on a too low level too. It's > > irrelevant > > detail to the _user_ that kernel used some "magic table". Similarly, > > it's > > irrelevant to the user that function called pcim_iomap failed. > > > > > The reason I just wrote "pcim_iomap failed\n" is that this seems to > > > be > > > this driver's style for those messages. See the dev_err() above, > > > there > > > they also just state that this or that function failed. > > > > The problem in using function names is they have obvious meaning for > > developers/coders but dev_err() is presented to user with varying > > level > > of knowledge about kernel internals/code. > > > > While users might be able to derive some information from the > > function > > name, it would be simply better to explain on higher level what > > failed > > which is what I think the original message tried to do even if it was > > a bit clumsy. There is zero need to know about kernel internals to > > interpret that message (arguably one needs to know some PCI to > > understand > > BAR, though). > > > > (Developers can find the internals by looking up the error message > > from > > the code so it doesn't take away something from developers.) > > Feel free to make a suggestion for a better error message. > > sth like "could not ioremap PCI BAR 0.\n" could satisfy your criteria. Yes.
All Acked-by's are in place now. Changes in v5: - Add Acked-by's from Alexander and Bharat (the latter sent off-list, because of some issue with receiving the previous patch sets). Changes in v4: - Add Acked-by's from Giovanni and Kalle. Changes in v3: - Add missing full stops to commit messages (Andy). Changes in v2: - Fix a bug in patch №4 ("crypto: marvell ...") where an error code was not set before printing it. (Me) - Apply Damien's Reviewed- / Acked-by to patches 1, 2 and 10. (Damien) - Apply Serge's Acked-by to patch №7. (Serge) - Apply Jiri's Reviewed-by to patch №8. (Jiri) - Apply Takashi Iwai's Reviewed-by to patch №9. (Takashi) Hi all, the PCI subsystem is currently working on cleaning up its devres API. To do so, a few functions will be replaced with better alternatives. This series removes pcim_iomap_regions_request_all(), which has been deprecated already, and accordingly replaces the calls to pcim_iomap_table() (which were only necessary because of pcim_iomap_regions_request_all() in the first place) with calls to pcim_iomap(). Would be great if you can take a look whether this behaves as you intended for your respective component. Cheers, Philipp Philipp Stanner (10): PCI: Make pcim_request_all_regions() a public function ata: ahci: Replace deprecated PCI functions crypto: qat - replace deprecated PCI functions crypto: marvell - replace deprecated PCI functions intel_th: pci: Replace deprecated PCI functions wifi: iwlwifi: replace deprecated PCI functions ntb: idt: Replace deprecated PCI functions serial: rp2: Replace deprecated PCI functions ALSA: korg1212: Replace deprecated PCI functions PCI: Remove pcim_iomap_regions_request_all() .../driver-api/driver-model/devres.rst | 1 - drivers/ata/acard-ahci.c | 6 +- drivers/ata/ahci.c | 6 +- drivers/crypto/intel/qat/qat_420xx/adf_drv.c | 11 +++- drivers/crypto/intel/qat/qat_4xxx/adf_drv.c | 11 +++- .../marvell/octeontx2/otx2_cptpf_main.c | 14 +++-- .../marvell/octeontx2/otx2_cptvf_main.c | 13 ++-- drivers/hwtracing/intel_th/pci.c | 9 ++- .../net/wireless/intel/iwlwifi/pcie/trans.c | 16 ++--- drivers/ntb/hw/idt/ntb_hw_idt.c | 13 ++-- drivers/pci/devres.c | 59 +------------------ drivers/tty/serial/rp2.c | 12 ++-- include/linux/pci.h | 3 +- sound/pci/korg1212/korg1212.c | 6 +- 14 files changed, 76 insertions(+), 104 deletions(-)