| Message ID | Z5XmLKzhKpLAlzHt@p100 (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | serial: 8250_pci: Fix Warning at drivers/pci/devres.c:603 pcim_add_mapping_to_legacy_table | expand |
On 1/26/25 08:37, 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: ... > > 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). Please ignore this patch as it behaves wrong. I'll send an updated patch. Helge
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@gmx.de> > Cc: <stable@vger.kernel.org> # 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.
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;
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@gmx.de> Cc: <stable@vger.kernel.org> # v6.12+