Re: [PATCH 0/2] PCI: Add and use pcim_iomap_region()

From: Heiner Kallweit <hkallweit1@gmail.com>
To: Philipp Stanner <pstanner@redhat.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Realtek linux nic maintainers <nic_swsd@realtek.com>,
	Paolo Abeni <pabeni@redhat.com>, Jakub Kicinski <kuba@kernel.org>,
	Eric Dumazet <edumazet@google.com>,
	David Miller <davem@davemloft.net>
Cc: "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>
Subject: Re: [PATCH 0/2] PCI: Add and use pcim_iomap_region()
Date: Thu, 28 Mar 2024 18:35:40 +0100	[thread overview]
Message-ID: <348fa275-3922-4ad1-944e-0b5d1dd3cff5@gmail.com> (raw)
In-Reply-To: <4cf0d7710a74095a14bedc68ba73612943683db4.camel@redhat.com>

On 27.03.2024 14:20, Philipp Stanner wrote:
> On Wed, 2024-03-27 at 12:52 +0100, Heiner Kallweit wrote:
>> Several drivers use the following sequence for a single BAR:
>> rc = pcim_iomap_regions(pdev, BIT(bar), name);
>> if (rc)
>>         error;
>> addr = pcim_iomap_table(pdev)[bar];
>>
>> Let's create a simpler (from implementation and usage perspective)
>> pcim_iomap_region() for this use case.
> 
> I like that idea – in fact, I liked it so much that I wrote that
> myself, although it didn't make it vor v6.9 ^^
> 
> You can look at the code here [1]
> 
> Since my series cleans up the PCI devres API as much as possible, I'd
> argue that prefering it would be better.
> 
Thanks for the hint. I'm not in a hurry, so yes: We should refactor the
pcim API, and then add functionality.

> But maybe you could do a review, since you're now also familiar with
> the code?
> 
I'm not subscribed to linux-pci, so I missed the cover letter, but had a
look at the patches in patchwork. Few remarks:

Instead of first adding a lot of new stuff and then cleaning up, I'd
propose to start with some cleanups. E.g. patch 7 could come first,
this would already allow to remove member mwi from struct pci_devres.

By the way, in patch 7 it may be a little simpler to have the following
sequence:

rc = pci_set_mwi()
if (rc)
	error
rc = devm_add_action_or_reset(.., __pcim_clear_mwi, ..);
if (rc)
	error

This would avoid the call to devm_remove_action().

We briefly touched the point whether the proposed new function returns
NULL or an ERR_PTR. I find it annoying that often the kernel doc function
description doesn't mention whether a function returns NULL or an ERR_PTR
in the error case. So I have to look at the function code. It's also a
typical bug source.
We won't solve this in general here. But I think we should be in line
with what related functions do.
The iomap() functions typically return NULL in the error case. Therefore
I'd say any new pcim_...iomap...() function should return NULL too. 

Then you add support for mapping BAR's partially. I never had such a use
case. Are there use cases for this?
Maybe we could omit this for now, if it helps to reduce the complexity
of the refactoring.

I also have bisectability in mind, therefore my personal preference would
be to make the single patches as small as possible. Not sure whether there's
a way to reduce the size of what currently is the first patch of the series.

> Greetings,
> P.
> 
> [1] https://lore.kernel.org/all/20240301112959.21947-1-pstanner@redhat.com/
> 
> 
>>
>> Note: The check for !pci_resource_len() is included in
>> pcim_iomap(), so we don't have to duplicate it.
>>
>> Make r8169 the first user of the new function.
>>
>> I'd prefer to handle this via the PCI tree.
>>
>> Heiner Kallweit (2):
>>   PCI: Add pcim_iomap_region
>>   r8169: use new function pcim_iomap_region()
>>
>>  drivers/net/ethernet/realtek/r8169_main.c |  8 +++----
>>  drivers/pci/devres.c                      | 28
>> +++++++++++++++++++++++
>>  include/linux/pci.h                       |  2 ++
>>  3 files changed, 33 insertions(+), 5 deletions(-)
>>
> 

Heiner