On 11/25/25 08:28, Philipp Stanner wrote:
On Tue, 2025-11-25 at 08:12 -0800, Guenter Roeck wrote:
On 11/25/25 07:48, Philipp Stanner wrote:
On Sun, 2025-11-23 at 08:42 -0800, Guenter Roeck wrote:
Hi,
On Thu, Jun 13, 2024 at 01:50:15PM +0200, Philipp Stanner wrote:
The pcim_iomap_devres.table administrated by pcim_iomap_table() has its
entries set and unset at several places throughout devres.c using manual
iterations which are effectively code duplications.
Add pcim_add_mapping_to_legacy_table() and
pcim_remove_mapping_from_legacy_table() helper functions and use them where
possible.
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Philipp Stanner <[email protected]>
[bhelgaas: s/short bar/int bar/ for consistency]
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/pci/devres.c | 77 +++++++++++++++++++++++++++++++++-----------
1 file changed, 58 insertions(+), 19 deletions(-)
diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
index f13edd4a3873..845d6fab0ce7 100644
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -297,6 +297,52 @@ void __iomem * const *pcim_iomap_table(struct pci_dev
*pdev)
}
EXPORT_SYMBOL(pcim_iomap_table);
+/*
+ * Fill the legacy mapping-table, so that drivers using the old API can
+ * still get a BAR's mapping address through pcim_iomap_table().
+ */
+static int pcim_add_mapping_to_legacy_table(struct pci_dev *pdev,
+ void __iomem *mapping, int bar)
+{
+ void __iomem **legacy_iomap_table;
+
+ if (bar >= PCI_STD_NUM_BARS)
+ return -EINVAL;
+
+ legacy_iomap_table = (void __iomem **)pcim_iomap_table(pdev);
+ if (!legacy_iomap_table)
+ return -ENOMEM;
+
+ /* The legacy mechanism doesn't allow for duplicate mappings. */
+ WARN_ON(legacy_iomap_table[bar]);
+
Ever since this patch has been applied, I see this warning on all hppa
(parisc) systems.
[ 0.978177] WARNING: CPU: 0 PID: 1 at drivers/pci/devres.c:473
pcim_add_mapping_to_legacy_table.part.0+0x54/0x80
[ 0.978850] Modules linked in:
[ 0.979277] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted
6.18.0-rc6-64bit+ #1 NONE
[ 0.979519] Hardware name: 9000/785/C3700
[ 0.979715]
[ 0.979768] YZrvWESTHLNXBCVMcbcbcbcbOGFRQPDI
[ 0.979886] PSW: 00001000000001000000000000001111 Not tainted
[ 0.980006] r00-03 000000000804000f 00000000414e10a0 0000000040acb300
00000000434b1440
[ 0.980167] r04-07 00000000414a78a0 0000000000029000 0000000000000000
0000000043522000
[ 0.980314] r08-11 0000000000000000 0000000000000008 0000000000000000
00000000434b0de8
[ 0.980461] r12-15 00000000434b11b0 000000004156a8a0 0000000043c655a0
0000000000000000
[ 0.980608] r16-19 000000004016e080 000000004019e7d8 0000000000000030
0000000043549780
[ 0.981106] r20-23 0000000020000000 0000000000000000 000000000800000e
0000000000000000
[ 0.981317] r24-27 0000000000000000 000000000800000f 0000000043522260
00000000414a78a0
[ 0.981480] r28-31 00000000436af480 00000000434b1680 00000000434b14d0
0000000000027000
[ 0.981641] sr00-03 0000000000000000 0000000000000000 0000000000000000
0000000000000000
[ 0.981805] sr04-07 0000000000000000 0000000000000000 0000000000000000
0000000000000000
[ 0.981972]
[ 0.982024] IASQ: 0000000000000000 0000000000000000 IAOQ: 0000000040acb31c
0000000040acb320
[ 0.982185] IIR: 03ffe01f ISR: 0000000000000000 IOR: 00000000436af410
[ 0.982322] CPU: 0 CR30: 0000000043549780 CR31: 0000000000000000
[ 0.982458] ORIG_R28: 00000000434b16b0
[ 0.982548] IAOQ[0]: pcim_add_mapping_to_legacy_table.part.0+0x54/0x80
[ 0.982733] IAOQ[1]: pcim_add_mapping_to_legacy_table.part.0+0x58/0x80
[ 0.982871] RP(r2): pcim_add_mapping_to_legacy_table.part.0+0x38/0x80
[ 0.983100] Backtrace:
[ 0.983439] [<0000000040acba1c>] pcim_iomap+0xc4/0x170
[ 0.983577] [<0000000040ba3e4c>] serial8250_pci_setup_port+0x8c/0x168
[ 0.983725] [<0000000040ba7588>] setup_port+0x38/0x50
[ 0.983837] [<0000000040ba7d94>] pci_hp_diva_setup+0x8c/0xd8
[ 0.983957] [<0000000040baa47c>] pciserial_init_ports+0x2c4/0x358
[ 0.984088] [<0000000040baa8bc>] pciserial_init_one+0x31c/0x330
[ 0.984214] [<0000000040abfab4>] pci_device_probe+0x194/0x270
Looking into serial8250_pci_setup_port():
if (pci_resource_flags(dev, bar) & IORESOURCE_MEM) {
if (!pcim_iomap(dev, bar, 0) && !pcim_iomap_table(dev))
return -ENOMEM;
Strange. From the code I see here the WARN_ON in
pcim_add_mapping_to_legacy_table() should not fire. I suspect that it's
actually triggered somewhere else.
pcim_iomap() calls pcim_add_mapping_to_legacy_table() which triggers the
traceback.
The caller uses the returned error to determine that it needs to call
pcim_iomap_table()
instead. As you point out below, that may not be necessary, but then it is
already
too late and the warning was triggered.
This suggests that the failure is expected. I can see that pcim_iomap_table()
is deprecated, and that one is supposed to use pcim_iomap() instead. However,
pcim_iomap() _is_ alrady used, and I don't see a function which lets the
caller replicate what is done above (attach multiple serial ports to the
same PCI bar).
Is serial8250_pci_setup_port() invoked in a loop somewhere? Where does
the "attach multiple" happen?
It is called for multiple serial ports, each of which are in the same bar but
with different offset into the bar.
How would you suggest to fix the problem ?
I suggest you try to remove the `&& pcim_iomap_table(dev)` from above
to see if that's really the cause. pcim_iomap() already creates the
table, and if it succeeds the table has been created with absolute
certainty. The entries will also be present. So the table-check is
surplus.
How would that fix anything ? The warning would still be triggered from the
failed call to pcim_iomap() for the 2nd and subsequent serial port on the
same bar.
OK, I failed to see that it's really pcim_iomap() which is called
multiple times for the same bar.
The warning itself is harmless, so it's not urgent.
Cleanup is always done through devres callbacks, one per resource. The
table is not used for that, just for accessors of existing mappings. So
at first glance I think that removing the WARN_ON would be OK. I'd like
to hear Bjorn's opinion on that, though.
Maybe you could investigate removing pcim_iomap_table() from this
driver, obtaining the mappings directly from calls to pcim_iomap().
serial8250_pci_setup_port() is an API function. It is called from several
drivers. Its callers have no means to pass "This is the first request for
this bar", and at least for some of the callers this would not be easy
to accomplish unless one changes and adds complexity to all that calling
code.
Calling it multiple times for the same BAR is valid, it's just the
table which complains. Since you are the first party I ever hear from
about that WARN_ON. So with this driver ported one could argue that
removing it is justified..
Maybe the reason for that is that there are more and more pointless
backtraces in the kernel. That only results in people ignoring them.
I do see some more, but then I notice that such warnings proliferate,
and feedback such as "The warning itself is harmless" doesn't really
encourage me doing that.
Another possiblity could be to switch to unmanaged PCI. Use pci_iomap()
and pci_enable_device() etc.
In case you have lots of spare cycles, the cleanest way would be to
remove the legacy table altogether. To do so, one would have to port
all users of pcim_iomap_table(). I have worked on that for a while and
have removed many of them. The most difficult remaining users are AFAIR
in drivers/ata/
Sorry, "spare cycles" is not exactly something that happens in my life.
Guenter
