Message-ID: <1983517bf5d0c98894a7d40fbec353ad75160cb4.camel@redhat.com>
Date: Wed, 17 Jan 2024 09:54:47 +0100
From: Philipp Stanner <pstanner@...hat.com>
To: Bjorn Helgaas <helgaas@...nel.org>
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-pci@...r.kernel.org,  linux-kernel@...r.kernel.org,
 dri-devel@...ts.freedesktop.org,  linux-doc@...r.kernel.org
Subject: Re: [PATCH 01/10] pci: add new set of devres functions

On Tue, 2024-01-16 at 12:44 -0600, Bjorn Helgaas wrote:
> On Mon, Jan 15, 2024 at 03:46:12PM +0100, Philipp Stanner wrote:
> > PCI's devres API is not extensible to ranged mappings and has
> > bug-provoking features. Improve that by providing better
> > alternatives.
> 
> I guess "ranged mappings" means a mapping that doesn't cover an
> entire
> BAR?  Maybe there's a way to clarify?

That's what it's supposed to mean, yes.
We could give it the longer title "mappings smaller than the whole BAR"
or something, I guess.


> 
> > When the original devres API for PCI was implemented, priority was
> > given
> > to the creation of a set of "pural functions" such as
> > pcim_request_regions(). These functions have bit masks as
> > parameters to
> > specify which BARs shall get mapped. Most users, however, only use
> > those
> > to mapp 1-3 BARs.
> > A complete set of "singular functions" does not exist.
> 
> s/mapp/map/
> 
> Rewrap to fill 75 columns or add blank lines between paragraphs. 
> Also
> below.
> 
> > As functions mapping / requesting multiple BARs at once have
> > (almost) no
> > mechanism in C to return the resources to the caller of the plural
> > function, the devres API utilizes the iomap-table administrated by
> > the
> > function pcim_iomap_table().
> > 
> > The entire PCI devres implementation was strongly tied to that
> > table
> > which only allows for mapping whole, complete BARs, as the BAR's
> > index
> > is used as table index. Consequently, it's not possible to, e.g.,
> > have a
> > pcim_iomap_range() function with that mechanism.
> > 
> > An additional problem is that pci-devres has been ipmlemented in a
> > sort
> > of "hybrid-mode": Some unmanaged functions have managed
> > counterparts
> > (e.g.: pci_iomap() <-> pcim_iomap()), making their managed nature
> > obvious to the programmer. However, the region-request functions in
> > pci.c, prefixed with pci_, behave either managed or unmanaged,
> > depending
> > on whether pci_enable_device() or pcim_enable_device() has been
> > called
> > in advance.
> 
> s/ipmlemented/implemented/
> 
> > This hybrid API is confusing and should be more cleanly separated
> > by
> > providing always-managed functions prefixed with pcim_.
> > 
> > Thus, the existing devres API is not desirable because:
> >         a) The vast majority of the users of the plural functions
> > only
> >            ever sets a single bit in the bit mask, consequently
> > making
> >            them singular functions anyways.
> >         b) There is no mechanism to request / iomap only part of a
> > BAR.
> >         c) The iomap-table mechanism is over-engineered,
> > complicated and
> >            can by definition not perform bounds checks, thus,
> > provoking
> >            memory faults: pcim_iomap_table(pdev)[42]
> 
> Not sure what "pcim_iomap_table(pdev)[42]" means.

That function currently is implemented with this prototype:
void __iomem * const *pcim_iomap_table(struct pci_dev *pdev);

And apparently, it's intended to index directly over the function. And
that's how at least part of the users use it indeed.

Here in drivers/crypto/inside-secure/safexcel.c, L.1919 for example:

	priv->base = pcim_iomap_table(pdev)[0];

I've never seen something that wonderful in C ever before, so it's not
surprising that you weren't sure what I mean....

pcim_iomap_table() can not and does not perform any bounds check. If
you do

void __iomem *mappy_map_mapface = pcim_iomap_table(pdev)[42];

then it will just return random garbage, or it faults. No -EINVAL or
anything. You won't even get NULL.

That's why this function must die.


> 
> >         d) region-request functions being sometimes managed and
> >            sometimes not is bug-provoking.
> 
> Indent with spaces (not tabs) so it still looks good when "git log"
> adds spaces to indent.
> 
> > + * Legacy struct storing addresses to whole mapped bars.
> 
> s/bar/BAR/ (several places).
> 
> > +       /* A region spaning an entire bar. */
> > +       PCIM_ADDR_DEVRES_TYPE_REGION,
> > +
> > +       /* A region spaning an entire bar, and a mapping for that
> > whole bar. */
> > +       PCIM_ADDR_DEVRES_TYPE_REGION_MAPPING,
> > +
> > +       /*
> > +        * A mapping within a bar, either spaning the whole bar or
> > just a range.
> > +        * Without a requested region.
> 
> s/spaning/spanning/ (several places).
> 
> > +       if (start == 0 || len == 0) /* that's an unused BAR. */
> 
> s/that/That/
> 
> > +       /*
> > +        * Ranged mappings don't get added to the legacy-table,
> > since the table
> > +        * only ever keeps track of whole BARs.
> > +        */
> > +
> 
> Spurious blank line.


I'll take care of the grammar and spelling stuff in v2.

Thanks,
P.

> 
> > +       devres_add(&pdev->dev, res);
> > +       return mapping;
> > +}
> > +EXPORT_SYMBOL(pcim_iomap_range);
> 
> Bjorn
> 

Powered by blists - more mailing lists