Re: [PATCH] serial: 8250_pci: Fix Warning at drivers/pci/devres.c:603 pcim_add_mapping_to_legacy_table

[Philipp Stanner]

On Sun, 2025-01-26 at 08:37 +0100, Helge Deller wrote:
> Some PA-RISC servers have BMC management cards (Diva) with up to 5
> serial
> UARTS per memory PCI bar. This triggers since at least kernel 6.12
> for each of
> the UARTS (beside the first one) the following warning in devres.c:
> 
>  0000:00:02.0: ttyS2 at MMIO 0xf0822000 (irq = 21, base_baud =
> 115200) is a 16550A
>  0000:00:02.0: ttyS3 at MMIO 0xf0822010 (irq = 21, base_baud =
> 115200) is a 16550A
>  ------------[ cut here ]------------
>  WARNING: CPU: 1 PID: 1 at drivers/pci/devres.c:603
> pcim_add_mapping_to_legacy_table+0x5c/0x8c
>  CPU: 1 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.11+ #2621
>  Hardware name: 9000/778/B160L
>  
>   IAOQ[0]: pcim_add_mapping_to_legacy_table+0x5c/0x8c
>   IAOQ[1]: pcim_add_mapping_to_legacy_table+0x60/0x8c
>   RP(r2): pcim_add_mapping_to_legacy_table+0x4c/0x8c
>  Backtrace:
>   [<10c1eb10>] pcim_iomap+0xd4/0x10c
>   [<10ca8784>] serial8250_pci_setup_port+0xa8/0x11c
>   [<10ca9a34>] pci_hp_diva_setup+0x6c/0xc4
>   [<10cab134>] pciserial_init_ports+0x150/0x324
>   [<10cab470>] pciserial_init_one+0xfc/0x20c
>   [<10c14780>] pci_device_probe+0xc0/0x190
>   ...
>  ---[ end trace 0000000000000000 ]---
> 
> I see three options to avoid this warning:
> a) drop the WARNING() from devrec.c,
> b) modify pcim_iomap() to return an existing mapping if it exists
>    instead of creating a new mapping, or
> c) change serial8250_pci_setup_port() to only create a new mapping
>    if none exists yet.
> 
> This patch implements option c).
> 
> Signed-off-by: Helge Deller <deller@xxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx> # v6.12+
> 
> 
> diff --git a/drivers/tty/serial/8250/8250_pcilib.c
> b/drivers/tty/serial/8250/8250_pcilib.c
> index ea906d721b2c..fc024bf86c1f 100644
> --- a/drivers/tty/serial/8250/8250_pcilib.c
> +++ b/drivers/tty/serial/8250/8250_pcilib.c
> @@ -19,7 +19,9 @@ int serial8250_pci_setup_port(struct pci_dev *dev,
> struct uart_8250_port *port,
>  		return -EINVAL;
>  
>  	if (pci_resource_flags(dev, bar) & IORESOURCE_MEM) {
> -		if (!pcim_iomap(dev, bar, 0) &&
> !pcim_iomap_table(dev))
> +		/* might have been mapped already with other offset
> */
> +		if (!pcim_iomap_table(dev) ||
> !pcim_iomap_table(dev)[bar] ||
> +			!pcim_iomap(dev, bar, 0))
>  			return -ENOMEM;
>  
>  		port->port.iotype = UPIO_MEM;
> 

[Answering despite the ignore-hint to provide some tips for a v2]

Alright, so this worked before 6.12 because pcim_iomap() just silently
returned NULL if a mapping already existed. Which it still does, but
with a warning.

If you have to rework 8250_pcilib.c anyways, would be good to get rid
of pcim_iomap_table() as you're at it, because it's deprecated and
problematic.

It seems to me that this driver would be best of it could store in its
state (uart_8250_port) whether that BAR has already been mapped, and if
yes, take the existing addr and add the offset as needed for your new
addr.

Feel free to reach out if I can provide further tips


Greetings
P.