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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6c749a78-2c98-45a8-b9d4-47f79b56c918@roeck-us.net>
Date: Tue, 25 Nov 2025 08:12:35 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Philipp Stanner <pstanner@...hat.com>
Cc: 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, dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
 linux-parisc@...r.kernel.org, Helge Deller <deller@....de>
Subject: Re: [PATCH v9 02/13] PCI: Add devres helpers for iomap table
 [resulting in backtraces on HPPA]

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

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.

Guenter