[PATCH] tty: serial: 8250: exar: fix kernel warning in default_setup function

[PATCH] tty: serial: 8250: exar: fix kernel warning in default_setup function

[Wilken Gottwalt]

Fixes:

[    2.601213] WARNING: CPU: 3 PID: 1 at pcim_iomap.part.0+0xbc/0xc0
[    2.601224] Modules linked in:
[    2.601230] CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.38-vanilla-standard #1
[    2.601237] Hardware name: Default string Default string/CXC-BT-JERO, BIOS S1.61.1.0# 09/17/2018
[    2.601241] RIP: 0010:pcim_iomap.part.0+0xbc/0xc0
[    2.601248] Code: 31 ed e8 07 a5 14 00 4c 89 e8 5b 41 5c 41 5d 41 5e 5d c3 cc cc cc cc 45 31 ed 5b 41 5c 4c 89 e8 41 5d 41 5e 5d c3 cc cc cc cc <0f> 0b eb a0 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 0f 1f
[    2.601253] RSP: 0000:ffffa7444001f830 EFLAGS: 00010286
[    2.601259] RAX: ffffa066c1321f28 RBX: 0000000000000000 RCX: ffffa066c1215330
[    2.601264] RDX: 0000000000000001 RSI: 0000000000000286 RDI: ffffa066c121532c
[    2.601268] RBP: ffffa7444001f850 R08: 0000000000000000 R09: 0000000000000286
[    2.601272] R10: 0000000000000000 R11: 000ffffffffff000 R12: ffffa066c1215000
[    2.601275] R13: ffffa744403b0000 R14: ffffa066c1310208 R15: ffffa066c12150c8
[    2.601279] FS:  0000000000000000(0000) GS:ffffa067d7d80000(0000) knlGS:0000000000000000
[    2.601284] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    2.601288] CR2: 0000000000000000 CR3: 000000005f22a000 CR4: 00000000001006f0
[    2.601292] Call Trace:
[    2.601296]  <TASK>
[    2.601301]  pcim_iomap+0x1a/0x20
[    2.601308]  serial8250_pci_setup_port+0xea/0x190
[    2.601337]  default_setup.constprop.0+0x27/0x90
[    2.601347]  pci_xr17v35x_setup+0xd0/0x190
[    2.601355]  exar_pci_probe+0x297/0x400
[    2.601365]  ? __pfx_generic_rs485_config+0x10/0x10
[    2.601383]  local_pci_probe+0x4b/0xb0
[    2.601390]  pci_device_probe+0xc5/0x200
[    2.601397]  really_probe+0xe5/0x390
[    2.601405]  __driver_probe_device+0x7e/0x160
[    2.601412]  driver_probe_device+0x23/0xa0
[    2.601418]  __driver_attach+0xe4/0x1e0
[    2.601424]  ? __pfx___driver_attach+0x10/0x10
[    2.601431]  bus_for_each_dev+0x7d/0xd0
[    2.601438]  driver_attach+0x1e/0x30
[    2.601444]  bus_add_driver+0x114/0x240
[    2.601450]  driver_register+0x64/0x130
[    2.601457]  ? __pfx_exar_pci_driver_init+0x10/0x10
[    2.601466]  __pci_register_driver+0x61/0x70
[    2.601471]  exar_pci_driver_init+0x1e/0x30
[    2.601479]  do_one_initcall+0x49/0x310
[    2.601487]  kernel_init_freeable+0x1aa/0x2e0
[    2.601495]  ? __pfx_kernel_init+0x10/0x10
[    2.601503]  kernel_init+0x1a/0x1c0
[    2.601510]  ret_from_fork+0x3c/0x60
[    2.601516]  ? __pfx_kernel_init+0x10/0x10
[    2.601523]  ret_from_fork_asm+0x1a/0x30
[    2.601531]  </TASK>

From kernel 6.8 to 6.9 the default_setup function was changed to use the
more generic serial8250_pci_setup_port function to setup the serial
ports, but that results in this kernel warning.

The serial8250_pci_setup_port function internally relies on the
pcim_iomap function. The way this function works was changed from kernel
6.10 to 6.11 and now clearly states in the description "This SHOULD only
be used once per BAR". And this is the issue. Basically all the hardware
handled by the 8250_exar driver are multi-port cards, which have
multiple ports on one PCI bar. The serial8250_pci_setup_port/pcim_iomap
functions can not be used with that driver anymore. Reverting the code
back to the older pci_resource_start approach fixes this issue.

Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
---
 drivers/tty/serial/8250/8250_exar.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 04a0cbab02c2..5660bb897803 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -500,12 +500,13 @@ static int default_setup(struct exar8250 *priv, struct pci_dev *pcidev,
 			 struct uart_8250_port *port)
 {
 	const struct exar8250_board *board = priv->board;
+	unsigned int bar = 0;
 	unsigned char status;
-	int err;
 
-	err = serial8250_pci_setup_port(pcidev, port, 0, offset, board->reg_shift);
-	if (err)
-		return err;
+	port->port.iotype = UPIO_MEM;
+	port->port.mapbase = pci_resource_start(pcidev, bar) + offset;
+	port->port.membase = priv->virt + offset;
+	port->port.regshift = board->reg_shift;
 
 	/*
 	 * XR17V35x UARTs have an extra divisor register, DLD that gets enabled
-- 
2.50.1

Re: [PATCH] tty: serial: 8250: exar: fix kernel warning in default_setup function

[Greg Kroah-Hartman]

On Tue, Jul 29, 2025 at 08:17:04AM +0000, Wilken Gottwalt wrote:
> Fixes:

But no "Fixes:" tag in the commit?  No cc: stable added?

> 
> [    2.601213] WARNING: CPU: 3 PID: 1 at pcim_iomap.part.0+0xbc/0xc0
> [    2.601224] Modules linked in:
> [    2.601230] CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.38-vanilla-standard #1
> [    2.601237] Hardware name: Default string Default string/CXC-BT-JERO, BIOS S1.61.1.0# 09/17/2018
> [    2.601241] RIP: 0010:pcim_iomap.part.0+0xbc/0xc0
> [    2.601248] Code: 31 ed e8 07 a5 14 00 4c 89 e8 5b 41 5c 41 5d 41 5e 5d c3 cc cc cc cc 45 31 ed 5b 41 5c 4c 89 e8 41 5d 41 5e 5d c3 cc cc cc cc <0f> 0b eb a0 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 0f 1f
> [    2.601253] RSP: 0000:ffffa7444001f830 EFLAGS: 00010286
> [    2.601259] RAX: ffffa066c1321f28 RBX: 0000000000000000 RCX: ffffa066c1215330
> [    2.601264] RDX: 0000000000000001 RSI: 0000000000000286 RDI: ffffa066c121532c
> [    2.601268] RBP: ffffa7444001f850 R08: 0000000000000000 R09: 0000000000000286
> [    2.601272] R10: 0000000000000000 R11: 000ffffffffff000 R12: ffffa066c1215000
> [    2.601275] R13: ffffa744403b0000 R14: ffffa066c1310208 R15: ffffa066c12150c8
> [    2.601279] FS:  0000000000000000(0000) GS:ffffa067d7d80000(0000) knlGS:0000000000000000
> [    2.601284] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    2.601288] CR2: 0000000000000000 CR3: 000000005f22a000 CR4: 00000000001006f0
> [    2.601292] Call Trace:
> [    2.601296]  <TASK>
> [    2.601301]  pcim_iomap+0x1a/0x20
> [    2.601308]  serial8250_pci_setup_port+0xea/0x190
> [    2.601337]  default_setup.constprop.0+0x27/0x90
> [    2.601347]  pci_xr17v35x_setup+0xd0/0x190
> [    2.601355]  exar_pci_probe+0x297/0x400
> [    2.601365]  ? __pfx_generic_rs485_config+0x10/0x10
> [    2.601383]  local_pci_probe+0x4b/0xb0
> [    2.601390]  pci_device_probe+0xc5/0x200
> [    2.601397]  really_probe+0xe5/0x390
> [    2.601405]  __driver_probe_device+0x7e/0x160
> [    2.601412]  driver_probe_device+0x23/0xa0
> [    2.601418]  __driver_attach+0xe4/0x1e0
> [    2.601424]  ? __pfx___driver_attach+0x10/0x10
> [    2.601431]  bus_for_each_dev+0x7d/0xd0
> [    2.601438]  driver_attach+0x1e/0x30
> [    2.601444]  bus_add_driver+0x114/0x240
> [    2.601450]  driver_register+0x64/0x130
> [    2.601457]  ? __pfx_exar_pci_driver_init+0x10/0x10
> [    2.601466]  __pci_register_driver+0x61/0x70
> [    2.601471]  exar_pci_driver_init+0x1e/0x30
> [    2.601479]  do_one_initcall+0x49/0x310
> [    2.601487]  kernel_init_freeable+0x1aa/0x2e0
> [    2.601495]  ? __pfx_kernel_init+0x10/0x10
> [    2.601503]  kernel_init+0x1a/0x1c0
> [    2.601510]  ret_from_fork+0x3c/0x60
> [    2.601516]  ? __pfx_kernel_init+0x10/0x10
> [    2.601523]  ret_from_fork_asm+0x1a/0x30
> [    2.601531]  </TASK>
> 
> >From kernel 6.8 to 6.9 the default_setup function was changed to use the
> more generic serial8250_pci_setup_port function to setup the serial
> ports, but that results in this kernel warning.
> 
> The serial8250_pci_setup_port function internally relies on the
> pcim_iomap function. The way this function works was changed from kernel
> 6.10 to 6.11 and now clearly states in the description "This SHOULD only
> be used once per BAR". And this is the issue. Basically all the hardware
> handled by the 8250_exar driver are multi-port cards, which have
> multiple ports on one PCI bar. The serial8250_pci_setup_port/pcim_iomap
> functions can not be used with that driver anymore. Reverting the code
> back to the older pci_resource_start approach fixes this issue.

These two paragraphs should be above the warning splat.


> 
> Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
> ---
>  drivers/tty/serial/8250/8250_exar.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index 04a0cbab02c2..5660bb897803 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -500,12 +500,13 @@ static int default_setup(struct exar8250 *priv, struct pci_dev *pcidev,
>  			 struct uart_8250_port *port)
>  {
>  	const struct exar8250_board *board = priv->board;
> +	unsigned int bar = 0;

Why is this needed?

>  	unsigned char status;
> -	int err;
>  
> -	err = serial8250_pci_setup_port(pcidev, port, 0, offset, board->reg_shift);
> -	if (err)
> -		return err;
> +	port->port.iotype = UPIO_MEM;
> +	port->port.mapbase = pci_resource_start(pcidev, bar) + offset;
> +	port->port.membase = priv->virt + offset;
> +	port->port.regshift = board->reg_shift;

And so now serial8250_pci_setup_port() is never called?  Are you sure
that's ok?

thanks,

greg k-h

Re: [PATCH] tty: serial: 8250: exar: fix kernel warning in default_setup function

[Wilken Gottwalt]

On Tue, 29 Jul 2025 10:48:17 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Tue, Jul 29, 2025 at 08:17:04AM +0000, Wilken Gottwalt wrote:
> > Fixes:
> 
> But no "Fixes:" tag in the commit?  No cc: stable added?

It is a long time I ago I did a fixes patch, argh. Hmm, it was a line like this
Fixes: 123456789012 ("bla: bla: aaaahhh"), right? I will fix that. Though, I'm
not sure if I should refer the pcim_iomap change here, or the change in the exar
drivers default_setup. The point is, the first change, the default_setup one was
fine until the later pcim_iomap change was done. The later change basically
affects hundrets of drivers, while the default_setup change is limited to the
8250 exar driver only. I will not touch pcim_iomap. What do you think about
this?

> > 
> > [    2.601213] WARNING: CPU: 3 PID: 1 at pcim_iomap.part.0+0xbc/0xc0
> > [    2.601224] Modules linked in:
> > [    2.601230] CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.38-vanilla-standard #1
> > [    2.601237] Hardware name: Default string Default string/CXC-BT-JERO, BIOS S1.61.1.0#
> > 09/17/2018 [    2.601241] RIP: 0010:pcim_iomap.part.0+0xbc/0xc0
> > [    2.601248] Code: 31 ed e8 07 a5 14 00 4c 89 e8 5b 41 5c 41 5d 41 5e 5d c3 cc cc cc cc 45 31
> > ed 5b 41 5c 4c 89 e8 41 5d 41 5e 5d c3 cc cc cc cc <0f> 0b eb a0 90 90 90 90 90 90 90 90 90 90
> > 90 90 90 90 90 90 0f 1f [    2.601253] RSP: 0000:ffffa7444001f830 EFLAGS: 00010286 [
> > 2.601259] RAX: ffffa066c1321f28 RBX: 0000000000000000 RCX: ffffa066c1215330 [    2.601264] RDX:
> > 0000000000000001 RSI: 0000000000000286 RDI: ffffa066c121532c [    2.601268] RBP:
> > ffffa7444001f850 R08: 0000000000000000 R09: 0000000000000286 [    2.601272] R10:
> > 0000000000000000 R11: 000ffffffffff000 R12: ffffa066c1215000 [    2.601275] R13:
> > ffffa744403b0000 R14: ffffa066c1310208 R15: ffffa066c12150c8 [    2.601279] FS:
> > 0000000000000000(0000) GS:ffffa067d7d80000(0000) knlGS:0000000000000000 [    2.601284] CS:
> > 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [    2.601288] CR2: 0000000000000000 CR3:
> > 000000005f22a000 CR4: 00000000001006f0 [    2.601292] Call Trace:
> > [    2.601296]  <TASK>
> > [    2.601301]  pcim_iomap+0x1a/0x20
> > [    2.601308]  serial8250_pci_setup_port+0xea/0x190
> > [    2.601337]  default_setup.constprop.0+0x27/0x90
> > [    2.601347]  pci_xr17v35x_setup+0xd0/0x190
> > [    2.601355]  exar_pci_probe+0x297/0x400
> > [    2.601365]  ? __pfx_generic_rs485_config+0x10/0x10
> > [    2.601383]  local_pci_probe+0x4b/0xb0
> > [    2.601390]  pci_device_probe+0xc5/0x200
> > [    2.601397]  really_probe+0xe5/0x390
> > [    2.601405]  __driver_probe_device+0x7e/0x160
> > [    2.601412]  driver_probe_device+0x23/0xa0
> > [    2.601418]  __driver_attach+0xe4/0x1e0
> > [    2.601424]  ? __pfx___driver_attach+0x10/0x10
> > [    2.601431]  bus_for_each_dev+0x7d/0xd0
> > [    2.601438]  driver_attach+0x1e/0x30
> > [    2.601444]  bus_add_driver+0x114/0x240
> > [    2.601450]  driver_register+0x64/0x130
> > [    2.601457]  ? __pfx_exar_pci_driver_init+0x10/0x10
> > [    2.601466]  __pci_register_driver+0x61/0x70
> > [    2.601471]  exar_pci_driver_init+0x1e/0x30
> > [    2.601479]  do_one_initcall+0x49/0x310
> > [    2.601487]  kernel_init_freeable+0x1aa/0x2e0
> > [    2.601495]  ? __pfx_kernel_init+0x10/0x10
> > [    2.601503]  kernel_init+0x1a/0x1c0
> > [    2.601510]  ret_from_fork+0x3c/0x60
> > [    2.601516]  ? __pfx_kernel_init+0x10/0x10
> > [    2.601523]  ret_from_fork_asm+0x1a/0x30
> > [    2.601531]  </TASK>
> > 
> > >From kernel 6.8 to 6.9 the default_setup function was changed to use the
> > more generic serial8250_pci_setup_port function to setup the serial
> > ports, but that results in this kernel warning.
> > 
> > The serial8250_pci_setup_port function internally relies on the
> > pcim_iomap function. The way this function works was changed from kernel
> > 6.10 to 6.11 and now clearly states in the description "This SHOULD only
> > be used once per BAR". And this is the issue. Basically all the hardware
> > handled by the 8250_exar driver are multi-port cards, which have
> > multiple ports on one PCI bar. The serial8250_pci_setup_port/pcim_iomap
> > functions can not be used with that driver anymore. Reverting the code
> > back to the older pci_resource_start approach fixes this issue.
> 
> These two paragraphs should be above the warning splat.

Yeah, I wasn't sure about this. Will fix this.

> > 
> > Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
> > ---
> >  drivers/tty/serial/8250/8250_exar.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> > index 04a0cbab02c2..5660bb897803 100644
> > --- a/drivers/tty/serial/8250/8250_exar.c
> > +++ b/drivers/tty/serial/8250/8250_exar.c
> > @@ -500,12 +500,13 @@ static int default_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> >  			 struct uart_8250_port *port)
> >  {
> >  	const struct exar8250_board *board = priv->board;
> > +	unsigned int bar = 0;
> 
> Why is this needed?

It was the original code, I just reverted it. The idea was to go back to code,
which was there for several kernel generations and worked flawlessly. But yes,
that can be optimized. I can change this.
 
> >  	unsigned char status;
> > -	int err;
> >  
> > -	err = serial8250_pci_setup_port(pcidev, port, 0, offset, board->reg_shift);
> > -	if (err)
> > -		return err;
> > +	port->port.iotype = UPIO_MEM;
> > +	port->port.mapbase = pci_resource_start(pcidev, bar) + offset;
> > +	port->port.membase = priv->virt + offset;
> > +	port->port.regshift = board->reg_shift;
> 
> And so now serial8250_pci_setup_port() is never called?  Are you sure
> that's ok?

Not by this exar driver, but other serial port drivers (8250_pci1xxxx, 8250_pci)
use it, just like it was before the change in kernel 6.9.

> thanks,
> 
> greg k-h

Thanks for the feedback.

greetings,
Wilken

Re: [PATCH] tty: serial: 8250: exar: fix kernel warning in default_setup function

[Wilken Gottwalt]

On Tue, 29 Jul 2025 10:48:17 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> > index 04a0cbab02c2..5660bb897803 100644
> > --- a/drivers/tty/serial/8250/8250_exar.c
> > +++ b/drivers/tty/serial/8250/8250_exar.c
> > @@ -500,12 +500,13 @@ static int default_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> >  			 struct uart_8250_port *port)
> >  {
> >  	const struct exar8250_board *board = priv->board;
> > +	unsigned int bar = 0;
> >  	unsigned char status;
> > -	int err;
> >  
> > -	err = serial8250_pci_setup_port(pcidev, port, 0, offset, board->reg_shift);
> > -	if (err)
> > -		return err;
> > +	port->port.iotype = UPIO_MEM;
> > +	port->port.mapbase = pci_resource_start(pcidev, bar) + offset;
> > +	port->port.membase = priv->virt + offset;
> > +	port->port.regshift = board->reg_shift;
> 
> And so now serial8250_pci_setup_port() is never called?  Are you sure
> that's ok?

Hi Greg,

I will not provide a second version of this patch, because this is a bigger
problem involving 8250_exar, 8250_pci and 8250_pci1xxxx. With the changes from
kernel 6.10 to 6.11 the underlying pcim_* functions where changed. The
serial8250_pci_setup_port() does checks on pci_dev + BAR where resources were
already mapped via pcim_iomap(), pci_iomap() or even pci_ioremap_bar(). Not
sure if mixing this is a good idea after the kernel 6.11 changes.

serial8250_pci_setup_port() uses pcim_iomap() and pcim_iomap_table() for checking
these already mapped resources. But the pcim_iomap_table() is deprecated and
suggests to use pcim_iomap() function to aquire the pointer to the resources
while at the same time pcim_iomap() description states, don't use this function 
twice on the same BAR. I think the most sane approach would be to drop the
pcim_iomap() and pcim_iomap_table() checks from the serial8250_pci_setup_port()
function. But I can not fully test this, I only have access to some hardware
used by the 8250_exar driver. I also CC Andy and Parker, both worked on the
affected code.

greetings,
Wilken

Re: [PATCH] tty: serial: 8250: exar: fix kernel warning in default_setup function

[Andy Shevchenko]

On Wed, Jul 30, 2025 at 11:03:50AM +0000, Wilken Gottwalt wrote:
> On Tue, 29 Jul 2025 10:48:17 +0200
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> > > index 04a0cbab02c2..5660bb897803 100644
> > > --- a/drivers/tty/serial/8250/8250_exar.c
> > > +++ b/drivers/tty/serial/8250/8250_exar.c
> > > @@ -500,12 +500,13 @@ static int default_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> > >  			 struct uart_8250_port *port)
> > >  {
> > >  	const struct exar8250_board *board = priv->board;
> > > +	unsigned int bar = 0;
> > >  	unsigned char status;
> > > -	int err;
> > >  
> > > -	err = serial8250_pci_setup_port(pcidev, port, 0, offset, board->reg_shift);
> > > -	if (err)
> > > -		return err;
> > > +	port->port.iotype = UPIO_MEM;
> > > +	port->port.mapbase = pci_resource_start(pcidev, bar) + offset;
> > > +	port->port.membase = priv->virt + offset;
> > > +	port->port.regshift = board->reg_shift;
> > 
> > And so now serial8250_pci_setup_port() is never called?  Are you sure
> > that's ok?
> 
> Hi Greg,
> 
> I will not provide a second version of this patch, because this is a bigger
> problem involving 8250_exar, 8250_pci and 8250_pci1xxxx. With the changes from
> kernel 6.10 to 6.11 the underlying pcim_* functions where changed. The
> serial8250_pci_setup_port() does checks on pci_dev + BAR where resources were
> already mapped via pcim_iomap(), pci_iomap() or even pci_ioremap_bar(). Not
> sure if mixing this is a good idea after the kernel 6.11 changes.
> 
> serial8250_pci_setup_port() uses pcim_iomap() and pcim_iomap_table() for checking
> these already mapped resources. But the pcim_iomap_table() is deprecated and
> suggests to use pcim_iomap() function to aquire the pointer to the resources
> while at the same time pcim_iomap() description states, don't use this function 
> twice on the same BAR. I think the most sane approach would be to drop the
> pcim_iomap() and pcim_iomap_table() checks from the serial8250_pci_setup_port()
> function. But I can not fully test this, I only have access to some hardware
> used by the 8250_exar driver. I also CC Andy and Parker, both worked on the
> affected code.

I'm on vacations right now and I lost context of this a long ago, please Cc me
to any new version of this change to have a fresh look.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] tty: serial: 8250: exar: fix kernel warning in default_setup function

[Wilken Gottwalt]

On Wed, 6 Aug 2025 00:34:44 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Wed, Jul 30, 2025 at 11:03:50AM +0000, Wilken Gottwalt wrote:
> > On Tue, 29 Jul 2025 10:48:17 +0200
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > 
> > > > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> > > > index 04a0cbab02c2..5660bb897803 100644
> > > > --- a/drivers/tty/serial/8250/8250_exar.c
> > > > +++ b/drivers/tty/serial/8250/8250_exar.c
> > > > @@ -500,12 +500,13 @@ static int default_setup(struct exar8250 *priv, struct pci_dev
> > > > *pcidev, struct uart_8250_port *port)
> > > >  {
> > > >  	const struct exar8250_board *board = priv->board;
> > > > +	unsigned int bar = 0;
> > > >  	unsigned char status;
> > > > -	int err;
> > > >  
> > > > -	err = serial8250_pci_setup_port(pcidev, port, 0, offset, board->reg_shift);
> > > > -	if (err)
> > > > -		return err;
> > > > +	port->port.iotype = UPIO_MEM;
> > > > +	port->port.mapbase = pci_resource_start(pcidev, bar) + offset;
> > > > +	port->port.membase = priv->virt + offset;
> > > > +	port->port.regshift = board->reg_shift;
> > > 
> > > And so now serial8250_pci_setup_port() is never called?  Are you sure
> > > that's ok?
> > 
> > Hi Greg,
> > 
> > I will not provide a second version of this patch, because this is a bigger
> > problem involving 8250_exar, 8250_pci and 8250_pci1xxxx. With the changes from
> > kernel 6.10 to 6.11 the underlying pcim_* functions where changed. The
> > serial8250_pci_setup_port() does checks on pci_dev + BAR where resources were
> > already mapped via pcim_iomap(), pci_iomap() or even pci_ioremap_bar(). Not
> > sure if mixing this is a good idea after the kernel 6.11 changes.
> > 
> > serial8250_pci_setup_port() uses pcim_iomap() and pcim_iomap_table() for checking
> > these already mapped resources. But the pcim_iomap_table() is deprecated and
> > suggests to use pcim_iomap() function to aquire the pointer to the resources
> > while at the same time pcim_iomap() description states, don't use this function 
> > twice on the same BAR. I think the most sane approach would be to drop the
> > pcim_iomap() and pcim_iomap_table() checks from the serial8250_pci_setup_port()
> > function. But I can not fully test this, I only have access to some hardware
> > used by the 8250_exar driver. I also CC Andy and Parker, both worked on the
> > affected code.
> 
> I'm on vacations right now and I lost context of this a long ago, please Cc me
> to any new version of this change to have a fresh look.

Hi Andy,

there is not much to add here. It is basically a recursivly added issue and
affects the three mentioned drivers. In my opinion it is safe to remove the
pcim_iomap() and pcim_iomap_table() functions checks from the generic
serial8250_pci_setup_port() function. To me it looks like the "newly"
implemented pcim_iomap(), which should not be used twice on the same pci bar,
is the only issue here. But I can only speak for the 8250_exar driver. We
use the fix in productions systems and it solves the issue. But beyond that?
To me the change of the pcim_iomap() looks like a design flaw in general,
allowing io-mapping only one resource per pci bar looks odd to me, but I am
not knowleged enough about these subsystems.

greetings

Re: [PATCH] tty: serial: 8250: exar: fix kernel warning in default_setup function

[Parker Newman]

On Wed, 13 Aug 2025 11:06:31 +0000
Wilken Gottwalt <wilken.gottwalt@posteo.net> wrote:

> On Wed, 6 Aug 2025 00:34:44 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > On Wed, Jul 30, 2025 at 11:03:50AM +0000, Wilken Gottwalt wrote:  
> > > On Tue, 29 Jul 2025 10:48:17 +0200
> > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > >   
> > > > > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> > > > > index 04a0cbab02c2..5660bb897803 100644
> > > > > --- a/drivers/tty/serial/8250/8250_exar.c
> > > > > +++ b/drivers/tty/serial/8250/8250_exar.c
> > > > > @@ -500,12 +500,13 @@ static int default_setup(struct exar8250 *priv, struct pci_dev
> > > > > *pcidev, struct uart_8250_port *port)
> > > > >  {
> > > > >  	const struct exar8250_board *board = priv->board;
> > > > > +	unsigned int bar = 0;
> > > > >  	unsigned char status;
> > > > > -	int err;
> > > > >  
> > > > > -	err = serial8250_pci_setup_port(pcidev, port, 0, offset, board->reg_shift);
> > > > > -	if (err)
> > > > > -		return err;
> > > > > +	port->port.iotype = UPIO_MEM;
> > > > > +	port->port.mapbase = pci_resource_start(pcidev, bar) + offset;
> > > > > +	port->port.membase = priv->virt + offset;
> > > > > +	port->port.regshift = board->reg_shift;  
> > > > 
> > > > And so now serial8250_pci_setup_port() is never called?  Are you sure
> > > > that's ok?  
> > > 
> > > Hi Greg,
> > > 
> > > I will not provide a second version of this patch, because this is a bigger
> > > problem involving 8250_exar, 8250_pci and 8250_pci1xxxx. With the changes from
> > > kernel 6.10 to 6.11 the underlying pcim_* functions where changed. The
> > > serial8250_pci_setup_port() does checks on pci_dev + BAR where resources were
> > > already mapped via pcim_iomap(), pci_iomap() or even pci_ioremap_bar(). Not
> > > sure if mixing this is a good idea after the kernel 6.11 changes.
> > > 
> > > serial8250_pci_setup_port() uses pcim_iomap() and pcim_iomap_table() for checking
> > > these already mapped resources. But the pcim_iomap_table() is deprecated and
> > > suggests to use pcim_iomap() function to aquire the pointer to the resources
> > > while at the same time pcim_iomap() description states, don't use this function 
> > > twice on the same BAR. I think the most sane approach would be to drop the
> > > pcim_iomap() and pcim_iomap_table() checks from the serial8250_pci_setup_port()
> > > function. But I can not fully test this, I only have access to some hardware
> > > used by the 8250_exar driver. I also CC Andy and Parker, both worked on the
> > > affected code.  
> > 
> > I'm on vacations right now and I lost context of this a long ago, please Cc me
> > to any new version of this change to have a fresh look.  
> 
> Hi Andy,
> 
> there is not much to add here. It is basically a recursivly added issue and
> affects the three mentioned drivers. In my opinion it is safe to remove the
> pcim_iomap() and pcim_iomap_table() functions checks from the generic
> serial8250_pci_setup_port() function. To me it looks like the "newly"
> implemented pcim_iomap(), which should not be used twice on the same pci bar,
> is the only issue here. But I can only speak for the 8250_exar driver. We
> use the fix in productions systems and it solves the issue. But beyond that?
> To me the change of the pcim_iomap() looks like a design flaw in general,
> allowing io-mapping only one resource per pci bar looks odd to me, but I am
> not knowleged enough about these subsystems.
> 
Hi All,
It looks like there are 2 issues in serial8250_pci_setup_port() after the
previously mentioned changes to the PCI core. 

1. pcim_iomap() should only be called once per BAR 
2. pcim_iomap_table() is deprecated

It seems like fixing serial8250_pci_setup_port() makes the most sense as the 
deprecated code is in that function. This function is used in 8250_exar,
8250_pci, and 8250_pci1xxxx. 

The fix would be to have pcim_iomap() called in probe prior to setting up the 
ports. This is already done in 8250_exar and 8250_pci1xxxx but not in 8250_pci. 

Then 8250_pci_setup_port() would need a new argument to pass the serial card's 
BAR address. Or maybe add a "struct serial8250_pci_port" that wrappers
all the arguments?

Making these changes for 8250_pci1xxxx and 8250_exar would be trivial but 
8250_pci will take some more effort. 

I can try to get this done in the next few weeks if this makes sense to 
everyone. I can test the 8250_exar driver and potentially can find a 8250_pci
based card but I don't think I have access to anything that uses 8250_pci1xxxx.

-Parker

Re: [PATCH] tty: serial: 8250: exar: fix kernel warning in default_setup function

[Andy Shevchenko]

On Wed, Aug 13, 2025 at 09:30:47AM -0400, Parker Newman wrote:
> On Wed, 13 Aug 2025 11:06:31 +0000
> Wilken Gottwalt <wilken.gottwalt@posteo.net> wrote:

...

> I can try to get this done in the next few weeks if this makes sense to 
> everyone. I can test the 8250_exar driver and potentially can find a 8250_pci
> based card but I don't think I have access to anything that uses 8250_pci1xxxx.

Dunno if I missed this series, but if not, I think it makes sense to go this way.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH] tty: serial: 8250: exar: fix kernel warning in default_setup function

[Parker Newman]

Hi Andy,
Florian ended up implementing this fix in this patch before I had a chance to look at it: 
https://lore.kernel.org/linux-serial/20250930072743.791580-1-fe@dev.tdt.de/

This issue should be solved 😊.

Thanks,
Parker

-----Original Message-----
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> 
Sent: October 27, 2025 8:18 AM
To: Parker Newman <parker@finest.io>
Cc: Wilken Gottwalt <wilken.gottwalt@posteo.net>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux-kernel@vger.kernel.org; linux-serial@vger.kernel.org; Jiri Slaby <jirislaby@kernel.org>; Parker Newman <pnewman@connecttech.com>
Subject: Re: [PATCH] tty: serial: 8250: exar: fix kernel warning in default_setup function

On Wed, Aug 13, 2025 at 09:30:47AM -0400, Parker Newman wrote:
> On Wed, 13 Aug 2025 11:06:31 +0000
> Wilken Gottwalt <wilken.gottwalt@posteo.net> wrote:

...

> I can try to get this done in the next few weeks if this makes sense 
> to everyone. I can test the 8250_exar driver and potentially can find 
> a 8250_pci based card but I don't think I have access to anything that uses 8250_pci1xxxx.

Dunno if I missed this series, but if not, I think it makes sense to go this way.

--
With Best Regards,
Andy Shevchenko

Re: [PATCH] tty: serial: 8250: exar: fix kernel warning in default_setup function

[Andy Shevchenko]

On Mon, Oct 27, 2025 at 12:31:21PM +0000, Parker Newman wrote:
> Hi Andy,
> Florian ended up implementing this fix in this patch before I had a chance to look at it: 
> https://lore.kernel.org/linux-serial/20250930072743.791580-1-fe@dev.tdt.de/
> 

Right, but refactoring as you suggested may also be welcome!

> This issue should be solved 😊.

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> 
> Sent: October 27, 2025 8:18 AM

> On Wed, Aug 13, 2025 at 09:30:47AM -0400, Parker Newman wrote:
> > On Wed, 13 Aug 2025 11:06:31 +0000
> > Wilken Gottwalt <wilken.gottwalt@posteo.net> wrote:

...

> > I can try to get this done in the next few weeks if this makes sense 
> > to everyone. I can test the 8250_exar driver and potentially can find 
> > a 8250_pci based card but I don't think I have access to anything that uses 8250_pci1xxxx.
> 
> Dunno if I missed this series, but if not, I think it makes sense to go this way.

-- 
With Best Regards,
Andy Shevchenko