Re: [PATCH 06/10] wifi: iwlwifi: replace deprecated PCI functions

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3993 bytes --]

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.)

-- 
 i.

> I am indifferent about the message, though. Whatever the maintainer
> prefers is fine.