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>
Cc: "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>
Subject: Re: [PATCH 0/2] PCI: Add and use pcim_iomap_region()
Date: Tue, 2 Apr 2024 15:54:16 +0200	[thread overview]
Message-ID: <38eb4bed-f2e9-4e3e-993b-78da54bf988e@gmail.com> (raw)
In-Reply-To: <a0d0b6b1269babb6a8f4e3bcceafee87bb49dcd1.camel@redhat.com>

On 02.04.2024 15:17, Philipp Stanner wrote:
> On Thu, 2024-03-28 at 23:03 +0100, Heiner Kallweit wrote:
>> 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.
> 
> Those members have been there before I touched anything.
> Patch #8 removes them entirely, so I'd say that result has been
> achieved.
> 

- all the networking people because discussion is solely about PCI core now

I think the proposed pcim_intx() is more complex than needed, and I see
issues if it's called multiple times. In addition you state that pci_intx()
is outdated, but add an API call for the same functionality.

Did you see the RFC patches I sent? they could help to reduce the complexity
of the pcim refactoring.

> Besides, considering the current fragmentation of devres within the PCI
> subsystem, I think it's wise to do 100% of devres operations in
> devres.c
> 
> P.
> 
>>
>>> 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
>>
> 