Re: [PATCH 01/10] pci: add new set of devres functions
On Wed, Jan 17, 2024 at 09:54:47AM +0100, Philipp Stanner wrote: > 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. "partial BAR mappings"? > > > to the creation of a set of "pural functions" such as s/pural/plural/ (I missed this before). > > > 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. No argument except that this example only makes sense after one looks up the prototype and connects the dots. Bjorn
Re: [PATCH 01/10] pci: add new set of devres functions
On Tue, 2024-01-16 at 23:15 +0200, andy.shevche...@gmail.com wrote:
> Mon, Jan 15, 2024 at 03:46:12PM +0100, Philipp Stanner kirjoitti:
> > PCI's devres API is not extensible to ranged mappings and has
> > bug-provoking features. Improve that by providing better
> > alternatives.
> >
> > 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.
> >
> > 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.
> >
> > 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]
> > d) region-request functions being sometimes managed and
> > sometimes not is bug-provoking.
> >
> > Implement a set of singular pcim_ functions that use devres
> > directly
> > and bypass the legacy iomap table mechanism.
> > Add devres.c to driver-api documentation.
>
> ...
>
> > +struct pcim_addr_devres {
> > + enum pcim_addr_devres_type type;
> > + void __iomem *baseaddr;
> > + unsigned long offset;
> > + unsigned long len;
> > + short bar;
> > +};
> > +
> > +static inline void pcim_addr_devres_clear(struct pcim_addr_devres
> > *res)
> > +{
> > + res->type = PCIM_ADDR_DEVRES_TYPE_INVALID;
> > + res->bar = -1;
> > + res->baseaddr = NULL;
> > + res->offset = 0;
> > + res->len = 0;
>
> More robust (in case the data type gets extended) is memset() +
> individual
> (non-0) sets.
ACK
>
> > +}
>
> ...
>
> > +static int __pcim_request_region_range(struct pci_dev *pdev, int
> > bar,
> > + unsigned long offset, unsigned long maxlen,
> > + const char *name, int exclusive)
> > +{
> > + resource_size_t start = pci_resource_start(pdev, bar);
> > + resource_size_t len = pci_resource_len(pdev, bar);
> > + unsigned long flags = pci_resource_flags(pdev, bar);
> > +
> > + if (start == 0 || len == 0) /* that's an unused BAR. */
> > + return 0;
> > + if (len <= offset)
> > + return -EINVAL;
> > +
> > + start += offset;
> > + len -= offset;
>
> > + if (len > maxlen && maxlen != 0)
> > + len = maxlen;
>
> if (maxlen && ...)
>
> ?
I very much dislike this style, although I'm aware it's used in many
(but not all) regions of the kernel.
It makes your style inconsistent, because sometimes you do indeed check
for something larger or smaller than 0.
Plus, by checking for a number, everyone immediately sees that this is
an integer, not a pointer, which improves readability at 0 cost.
>
> > + if (flags & IORESOURCE_IO) {
> > + if (!request_region(start, len, name))
> > + return -EBUSY;
> > + } else if (flags & IORESOURCE_MEM) {
> > + if (!__request_mem_region(start, len, name,
> > exclusive))
> > + return -EBUSY;
> > + } else {
> > + /* That's not a device we can request anything on.
> > */
> > + return -ENODEV;
> > + }
>
> Hmm... Not sure, but the switch-case against type might be
> considered:
>
> switc
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 >
Re: [PATCH 01/10] pci: add new set of devres functions
Mon, Jan 15, 2024 at 03:46:12PM +0100, Philipp Stanner kirjoitti:
> PCI's devres API is not extensible to ranged mappings and has
> bug-provoking features. Improve that by providing better alternatives.
>
> 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.
>
> 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.
>
> 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]
> d) region-request functions being sometimes managed and
> sometimes not is bug-provoking.
>
> Implement a set of singular pcim_ functions that use devres directly
> and bypass the legacy iomap table mechanism.
> Add devres.c to driver-api documentation.
...
> +struct pcim_addr_devres {
> + enum pcim_addr_devres_type type;
> + void __iomem *baseaddr;
> + unsigned long offset;
> + unsigned long len;
> + short bar;
> +};
> +
> +static inline void pcim_addr_devres_clear(struct pcim_addr_devres *res)
> +{
> + res->type = PCIM_ADDR_DEVRES_TYPE_INVALID;
> + res->bar = -1;
> + res->baseaddr = NULL;
> + res->offset = 0;
> + res->len = 0;
More robust (in case the data type gets extended) is memset() + individual
(non-0) sets.
> +}
...
> +static int __pcim_request_region_range(struct pci_dev *pdev, int bar,
> + unsigned long offset, unsigned long maxlen,
> + const char *name, int exclusive)
> +{
> + resource_size_t start = pci_resource_start(pdev, bar);
> + resource_size_t len = pci_resource_len(pdev, bar);
> + unsigned long flags = pci_resource_flags(pdev, bar);
> +
> + if (start == 0 || len == 0) /* that's an unused BAR. */
> + return 0;
> + if (len <= offset)
> + return -EINVAL;
> +
> + start += offset;
> + len -= offset;
> + if (len > maxlen && maxlen != 0)
> + len = maxlen;
if (maxlen && ...)
?
> + if (flags & IORESOURCE_IO) {
> + if (!request_region(start, len, name))
> + return -EBUSY;
> + } else if (flags & IORESOURCE_MEM) {
> + if (!__request_mem_region(start, len, name, exclusive))
> + return -EBUSY;
> + } else {
> + /* That's not a device we can request anything on. */
> + return -ENODEV;
> + }
Hmm... Not sure, but the switch-case against type might be considered:
switch (resource_type(...)) {
...
}
> + return 0;
> +}
> +static void __pcim_release_region_range(struct pci_dev *pdev, int bar,
> + unsigned long offset, unsigned long maxlen)
> +{
> + resource_size_t start = pci_resource_start(pdev, bar);
> + resource_size_t len = pci_resource_len(pdev, bar);
> + unsigned long flags = pci_resource_flags(pdev, bar);
> +
> + if (len <= offset || start == 0)
> + return;
> +
> + if (len == 0 || maxlen == 0) /* This an unused BAR. Do nothing. */
> + return;
> +
> + start += offset;
> + len -= offset;
> +
> + if (len > maxlen)
> + len = maxlen;
This part is quite a duplication of the above function, no?
> + if (flags & IORESOURCE_IO)
> + release_region(start, len);
> +
Re: [PATCH 01/10] pci: add new set of devres functions
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? > 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. > 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. > + devres_add(&pdev->dev, res); > + return mapping; > +} > +EXPORT_SYMBOL(pcim_iomap_range); Bjorn
[PATCH 01/10] pci: add new set of devres functions
PCI's devres API is not extensible to ranged mappings and has
bug-provoking features. Improve that by providing better alternatives.
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.
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.
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]
d) region-request functions being sometimes managed and
sometimes not is bug-provoking.
Implement a set of singular pcim_ functions that use devres directly
and bypass the legacy iomap table mechanism.
Add devres.c to driver-api documentation.
Signed-off-by: Philipp Stanner
---
Documentation/driver-api/pci/pci.rst | 3 +
drivers/pci/devres.c | 481 ++-
include/linux/pci.h | 11 +
3 files changed, 490 insertions(+), 5 deletions(-)
diff --git a/Documentation/driver-api/pci/pci.rst
b/Documentation/driver-api/pci/pci.rst
index 4843cfad4f60..92b11775344e 100644
--- a/Documentation/driver-api/pci/pci.rst
+++ b/Documentation/driver-api/pci/pci.rst
@@ -4,6 +4,9 @@ PCI Support Library
.. kernel-doc:: drivers/pci/pci.c
:export:
+.. kernel-doc:: drivers/pci/devres.c
+ :export:
+
.. kernel-doc:: drivers/pci/pci-driver.c
:export:
diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
index 4bd1e125bca1..cc8c1501eb13 100644
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -8,10 +8,223 @@
*/
#define PCIM_IOMAP_MAX PCI_STD_NUM_BARS
+/*
+ * Legacy struct storing addresses to whole mapped bars.
+ */
struct pcim_iomap_devres {
void __iomem *table[PCIM_IOMAP_MAX];
};
+enum pcim_addr_devres_type {
+ /* Default initializer. */
+ PCIM_ADDR_DEVRES_TYPE_INVALID,
+
+ /* 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.
+*/
+ PCIM_ADDR_DEVRES_TYPE_MAPPING,
+
+ /* A ranged region within a bar, with a mapping spaning that range. */
+ PCIM_ADDR_DEVRES_TYPE_REGION_RANGE_MAPPING
+};
+
+/*
+ * This struct envelopes IO or MEM addresses, that means mappings and region
+ * requests, because those are very frequently requested and released together.
+ */
+struct pcim_addr_devres {
+ enum pcim_addr_devres_type type;
+ void __iomem *baseaddr;
+ unsigned long offset;
+ unsigned long len;
+ short bar;
+};
+
+static inline void pcim_addr_devres_clear(struct pcim_addr_devres *res)
+{
+ res->type = PCIM_ADDR_DEVRES_TYPE_INVALID;
+ res->bar = -1;
+ res->baseaddr = NULL;
+ res->offset = 0;
+ res->len = 0;
+}
+
+/*
+ * The following functions, __pcim_*_region*, exist as counterparts to the
+ * versions from pci.c - which, unfortunately, can be in "hybrid mode", i.e.,
+ * sometimes managed, sometimes not.
+ *
+ * To separate the APIs cleanly, we define our own, simplified versions here.
+ */
+
+/**
+ * __pcim_request_region_range - Request a ranged region
+ * @pdev: PCI device the region belongs to
+ * @bar: The BAR the region is wit
