Re: [PATCH v9 02/13] PCI: Add devres helpers for iomap table [resulting in backtraces on HPPA]

[Philipp Stanner <pstanner@redhat.com>]

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/20240605081605.18769-4-pstanner@redhat.com
> > Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> > [bhelgaas: s/short bar/int bar/ for consistency]
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  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.

> 
> 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?

> 
> 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.

Report back please if that helps, I'd want to understand what's
happening here.


Regards
P.