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

[Heiner Kallweit <hkallweit1@gmail.com>]

On 28.03.2024 18:35, Heiner Kallweit wrote:
> 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.
> 
When looking at the intx members of struct pci_devres:
Why not simply store the initial state of bit PCI_COMMAND_INTX_DISABLE
in struct pci_dev and restore this bit in do_pci_disable_device()?
This should allow us to get rid of these members.

> 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