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 >
