Message-ID: <20240311221511.GA821021@bhelgaas>
Date: Mon, 11 Mar 2024 17:15:11 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Philipp Stanner <pstanner@...hat.com>
Cc: Hans de Goede <hdegoede@...hat.com>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Maxime Ripard <mripard@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Sam Ravnborg <sam@...nborg.org>, dakr@...hat.com,
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org
Subject: Re: [PATCH v4 00/10] Make PCI's devres API more consistent

On Mon, Mar 11, 2024 at 12:45:15PM +0100, Philipp Stanner wrote:
> Gentle ping because the Merge Window opened 8-)

Thanks; I'll look at this for v6.10.  It's too late to add significant
changes for the v6.9 merge window since it's already open.

Bjorn

> On Fri, 2024-03-01 at 12:29 +0100, Philipp Stanner wrote:
> > This v4 now can (hopefully) be applied to linux-next, but not to
> > mainline/master.
> > 
> > Changes in v4:
> >   - Rebase against linux-next
> > 
> > Changes in v3:
> >   - Use the term "PCI devres API" at some forgotten places.
> >   - Fix more grammar errors in patch #3.
> >   - Remove the comment advising to call (the outdated) pcim_intx() in
> > pci.c
> >   - Rename __pcim_request_region_range() flags-field "exclusive" to
> >     "req_flags", since this is what the int actually represents.
> >   - Remove the call to pcim_region_request() from patch #10. (Hans)
> > 
> > Changes in v2:
> >   - Make commit head lines congruent with PCI's style (Bjorn)
> >   - Add missing error checks for devm_add_action(). (Andy)
> >   - Repair the "Returns: " marks for docu generation (Andy)
> >   - Initialize the addr_devres struct with memset(). (Andy)
> >   - Make pcim_intx() a PCI-internal function so that new drivers
> > won't
> >     be encouraged to use the outdated pci_intx() mechanism.
> >     (Andy / Philipp)
> >   - Fix grammar and spelling (Bjorn)
> >   - Be more precise on why pcim_iomap_table() is problematic (Bjorn)
> >   - Provide the actual structs' and functions' names in the commit
> >     messages (Bjorn)
> >   - Remove redundant variable initializers (Andy)
> >   - Regroup PM bitfield members in struct pci_dev (Andy)
> >   - Make pcim_intx() visible only for the PCI subsystem so that
> > new    
> >     drivers won't use this outdated API (Andy, Myself)
> >   - Add a NOTE to pcim_iomap() to warn about this function being
> > the    onee
> >     xception that does just return NULL.
> >   - Consistently use the term "PCI devres API"; also in Patch #10
> > (Bjorn)
> > 
> > 
> > ¡Hola!
> > 
> > PCI's devres API suffers several weaknesses:
> > 
> > 1. There are functions prefixed with pcim_. Those are always managed
> >    counterparts to never-managed functions prefixed with pci_ – or so
> > one
> >    would like to think. There are some apparently unmanaged functions
> >    (all region-request / release functions, and pci_intx()) which
> >    suddenly become managed once the user has initialized the device
> > with
> >    pcim_enable_device() instead of pci_enable_device(). This
> > "sometimes
> >    yes, sometimes no" nature of those functions is confusing and
> >    therefore bug-provoking. In fact, it has already caused a bug in
> > DRM.
> >    The last patch in this series fixes that bug.
> > 2. iomappings: Instead of giving each mapping its own callback, the
> >    existing API uses a statically allocated struct tracking one
> > mapping
> >    per bar. This is not extensible. Especially, you can't create
> >    _ranged_ managed mappings that way, which many drivers want.
> > 3. Managed request functions only exist as "plural versions" with a
> >    bit-mask as a parameter. That's quite over-engineered considering
> >    that each user only ever mapps one, maybe two bars.
> > 
> > This series:
> > - add a set of new "singular" devres functions that use devres the
> > way
> >   its intended, with one callback per resource.
> > - deprecates the existing iomap-table mechanism.
> > - deprecates the hybrid nature of pci_ functions.
> > - preserves backwards compatibility so that drivers using the
> > existing
> >   API won't notice any changes.
> > - adds documentation, especially some warning users about the
> >   complicated nature of PCI's devres.
> > 
> > 
> > Note that this series is based on my "unify pci_iounmap"-series from
> > a
> > few weeks ago. [1]
> > 
> > I tested this on a x86 VM with a simple pci test-device with two
> > regions. Operates and reserves resources as intended on my system.
> > Kasan and kmemleak didn't find any problems.
> > 
> > I believe this series cleans the API up as much as possible without
> > having to port all existing drivers to the new API. Especially, I
> > think
> > that this implementation is easy to extend if the need for new
> > managed
> > functions arises :)
> > 
> > Greetings,
> > P.
> > 
> > Philipp Stanner (10):
> >   PCI: Add new set of devres functions
> >   PCI: Deprecate iomap-table functions
> >   PCI: Warn users about complicated devres nature
> >   PCI: Make devres region requests consistent
> >   PCI: Move dev-enabled status bit to struct pci_dev
> >   PCI: Move pinned status bit to struct pci_dev
> >   PCI: Give pcim_set_mwi() its own devres callback
> >   PCI: Give pci(m)_intx its own devres callback
> >   PCI: Remove legacy pcim_release()
> >   drm/vboxvideo: fix mapping leaks
> > 
> >  drivers/gpu/drm/vboxvideo/vbox_main.c |   20 +-
> >  drivers/pci/devres.c                  | 1013 +++++++++++++++++++++--
> > --
> >  drivers/pci/iomap.c                   |   18 +
> >  drivers/pci/pci.c                     |  123 ++-
> >  drivers/pci/pci.h                     |   21 +-
> >  include/linux/pci.h                   |   18 +-
> >  6 files changed, 1001 insertions(+), 212 deletions(-)
> > 
> 

Powered by blists - more mailing lists