Message-ID: <a893d61e125d58d8e6757c5f1a591682f6ca934c.camel@redhat.com>
Date: Tue, 02 Apr 2024 15:40:58 +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 18:35 +0100, 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.

Thx for the feedback so far. See my answers below:

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

I guess patches 5, 6, 7 and 8 could come first.
However, 9 has to be last because it kills the legacy struct pci_devres
in pci.h and can only do so once the old functions use the new API
below (patches 1, 2 and 4).
So one could say patches 5-9 form a row, aiming at blowing up that
struct, and relying on 1, 2 and 4 to do so.

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

All my functions' docstrings do explicitely name the return code. It's
indeed important to always do that except for super trivial things, I
agree

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

Well, the thought behind it was that to be more verbose and precise is
better than absolute consistency.

I'd agree that we could go for pure NULL implementations for functions
that just ioremap(), but the more sophisticated PCI wrappers do several
things at once:
 * perform sanity (region ranges etc.) checks on user input
 * do region requests
 * iomap
 * do devres specific stuff, including allocations

Especially informing a user specifically about -EBUSY is very valuable
IMO. It certainly is for our use case (Nvidia graphics cards with
several 'competing' drivers) where several drivers could try to grab
the same BAR. A user should get the info "someone is using that thing
already". With NULL he'd just see "failed loading driver for some
reason".

So I think the bes thing would be to remain consistent *within* PCI,
since all PCI users should only ever use PCI functions anyways and not
ioremap() things manually.

So PCI should be as consistent as possible, I think. pcim_iomap()
unfortunately has 50 callers.
The cleanest way would be to port them to a pcim_iomap() that returns
an ERR_PTR(), but I don't have capacity for that right now.

> 
> Then you add support for mapping BAR's partially. I never had such a
> use
> case. Are there use cases for this?

Yes (I was surprised by that as well). Patch #10's vboxvideo does that
and mapps a BAR partially.

The function pcim_iomap_region_range() just also takes care of a
partial region request. It could have been used as an alternative for
solving patch #10's issues.


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

Well, the implementation is 100% based on common infrastructure that's
also used by pcim_iomap_region() (that would be enum
pcim_addr_devres_typ).

One could argue that as long as no one uses the function, it can cause
trouble – and once someone uses it there would be a use case.
I tested the function previously and it behaved as intended.

But, indeed, I'm not sure whether it really hurts to remove or keep it.
I'm quite indifferent about it.

Regards,
P.

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