| Message ID | 20241025145959.185373-7-pstanner@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Remove pcim_iomap_regions_request_all() | expand |
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" ;) 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. I am indifferent about the message, though. Whatever the maintainer prefers is fine. P.
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.)
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. (I just now noticed that so far it called BAR 0 the "first bar", which is also not gold standard) P. >
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"); ret = -ENODEV; goto out_no_pci; }
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(-)