linux-kernel - Re: [PATCH 00/10] Make PCI's devres API more consistent

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <96f355a41bf42c6a604aec37449d35246395d929.camel@redhat.com>
Date: Thu, 18 Jan 2024 09:48:11 +0100
From: Philipp Stanner <pstanner@...hat.com>
To: andy.shevchenko@...il.com
Cc: Jonathan Corbet <corbet@....net>, 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,
 linux-doc@...r.kernel.org,  linux-kernel@...r.kernel.org,
 dri-devel@...ts.freedesktop.org,  linux-pci@...r.kernel.org
Subject: Re: [PATCH 00/10] Make PCI's devres API more consistent

On Wed, 2024-01-17 at 10:59 +0100, Philipp Stanner wrote:
> On Tue, 2024-01-16 at 23:17 +0200, andy.shevchenko@...il.com wrote:
> > Mon, Jan 15, 2024 at 03:46:11PM +0100, Philipp Stanner kirjoitti:

[...]

> > 
> > 
> > > 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.
> > 
> > Instead of adding pcim_intx(), please provide proper one for
> > pci_alloc_irq_vectors(). Ideally it would be nice to deprecate
> > old IRQ management functions in PCI core and delete them in the
> > future.
> > 
> 
> In order to deprecate the intermingling with half-managed hyprid
> devres
> in pci.c, you need to have pci_intx() be backwards compatible. Unless
> you can remove it at once.
> And the least broken way to do that I thought would be pcim_intx(),
> because that's consistent with how I make pci_request_region() & Co.
> call into their managed counterparts.
> 
> There are 25 users of pci_intx().
> We'd have to look how many of them call pcim_enable_device() and how
> easy they would be to port to... pci_alloc_irq_vectors() you say? I
> haven't used that before. Would have to look into it and see how we
> could do that.

Alright, so I thought about that a bit.

So pci_intx() is the old way to do it and you would like to deprecate
it for good. Understood, makes sense

This series, however, is about deprecating PCI's broken devres
implementation – not about deprecating outdated PCI features in
general.

So I wouldn't like to touch anything here unless cleaning up devres
demands it.
Now the question would be: how can we solve this?

My suggestion would be:
Let's implement pcim_intx(), but only make it visible through
drivers/pci/pci.h. So we won't make it usable for other drivers, don't
EXPORT_SYMBOL() it and basically only have it as a tool to move the
devres-part clearly and cleanly from pci.c to devres.c

Further deprecating old PCI stuff could then be done in a separate
series.

ACK?

P.



> 
> 
> P.