Date: Tue, 02 Apr 2024 15:17:56 +0200
From: Philipp Stanner <pstanner@...hat.com>
To: Heiner Kallweit <hkallweit1@...il.com>, Bjorn Helgaas
 <bhelgaas@...gle.com>,  Realtek linux nic maintainers
 <nic_swsd@...ltek.com>, Paolo Abeni <pabeni@...hat.com>, Jakub Kicinski
 <kuba@...nel.org>, Eric Dumazet <edumazet@...gle.com>, David Miller
 <davem@...emloft.net>
Cc: "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>, 
	"netdev@...r.kernel.org"
	 <netdev@...r.kernel.org>
Subject: Re: [PATCH 0/2] PCI: Add and use pcim_iomap_region()

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.

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
> 

Powered by blists - more mailing lists