Re: [PATCH] tty: serial: Replace deprecated PCI API

[Philipp Stanner <phasta@mailbox.org>]

+Cc Florian

On Wed, 2025-12-17 at 08:11 -0800, Guenter Roeck wrote:
> On 12/17/25 06:06, Greg Kroah-Hartman wrote:
> > On Thu, Dec 11, 2025 at 02:57:46PM +0100, Philipp Stanner wrote:
> > > On Wed, 2025-11-26 at 09:02 -0800, Guenter Roeck wrote:
> > > > On 11/26/25 01:10, Philipp Stanner wrote:
> > > > > pcim_iomap_table() is deprecated. Moreover, its special usage in 8250,
> > > > > causes a WARN_ON to fire (in pcim_add_mapping_to_legacy_table()).
> > > > > 
> > > > > 8250's function serial8250_pci_setup_port() effectively maps the same
> > > > > BAR multiple times and adds an offset to the start address. While
> > > > > mapping and adding offsets is not a bug, it can be achieved in a far
> > > > > more straightforward way by using the specialized function
> > > > > pcim_iomap_range().
> > > > > 
> > > > > pcim_iomap_range() does not add the mapping addresses to the deprecated
> > > > > iomap table - that's not a problem, however, because non of the users of
> > > > > serial8250_pci_setup_port() uses pcim_iomap_table().
> > > > > 
> > > > > Replace the deprecated PCI functions with pcim_iomap_range().
> > > > > 
> > > > > Cc: Guenter Roeck <linux@roeck-us.net>
> > > > > Link: https://lore.kernel.org/dri-devel/16cd212f-6ea0-471d-bf32-34f55d7292fe@roeck-us.net/
> > > > > Signed-off-by: Philipp Stanner <phasta@kernel.org>
> > > > > ---
> > > > > @Guenther: Can you please test this? I hope it fixes your issue.
> > > > 
> > > > Yes, it does. Thanks a lot for fixing this!
> > > > 
> > > > Tested-by: Guenter Roeck <linux@roeck-us.net>
> > > 
> > > @Greg:
> > > Can you apply this?
> > 
> > Does not apply at all to 6.19-rc1 :(
> > 
> 
> It conflicts with commit b7cefdb663382 ("serial: 8250_pcilib: Replace deprecated
> PCI functions"). Unfortunately that commit does not fix the problem; I still
> see it with v6.19-rc1.

Without me being too familiar with the code, it seems that that commit
didn't do what it promised, which is getting rid of the multiple
mappings of the same BAR. Presumably it is mapped multiple times
through setup_port():

diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index 152f914c599d..f0f13fdda2df 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -165,7 +165,15 @@ static int
 setup_port(struct serial_private *priv, struct uart_8250_port *port,
           u8 bar, unsigned int offset, int regshift)
 {
-       return serial8250_pci_setup_port(priv->dev, port, bar, offset, regshift);
+       void __iomem *iomem = NULL;
+
+       if (pci_resource_flags(priv->dev, bar) & IORESOURCE_MEM) {
+               iomem = pcim_iomap(priv->dev, bar, 0);
+               if (!iomem)
+                       return -ENOMEM;
+       }
+
+       return serial8250_pci_setup_port(priv->dev, port, bar, offset, regshift, iomem);
 }


Note that pcim_iomap() is the party that is *adding* the mappings to
the legacy table, whereas pcim_iomap_table() was just used to access
those.

In any case, I'm quite confident that this description of intended
behavior from the commit message

"the remapping should only be called once via 'pcim_iomap()'. Therefore
the remapping moved to the caller of 'serial8250_pci_setup_port()'."

has not been implemented in the actual code. Otherwise the WARN_ON
couldn't fire.


My personal opinion is that mapping the same BAR multiple times is not
a bug and there is no real practical reason why it shouldn't be done
when it can make sense. pci_iomap_range() exists for that reason since
quite some time.

So my suggested fix for this would be to revert b7cefdb663382 and apply
my patch here instead.


P.


> 
> Guenter
>