[v1] serial: 8250_pcilib: Replace deprecated PCI functions

[PATCH] serial: 8250_pcilib: Replace deprecated PCI functions

Posted by Florian Eckert 1 week ago

When the '8250_exar' module is loaded in to the kernel, a kernel trace
with 'WARN_ON(legacy_iomap_table[bar])' is dumped to the console,
because the old pci table mapping is still used in '8250_pcilib'.

The old function have been deprecated in commit e354bb84a4c1 ("PCI:
Deprecate pcim_iomap_table(), pcim_iomap_regions_request_all()").

The remapping already takes or must take place in the driver that calls
the function 'serial8250_pci_setup_port()'. The remapping should only be
called once via 'pcim_iomap()'. Therefore the remapping moved to the
caller of 'serial8250_pci_setup_port()'.

To use the new functions in '8250_pcilib' the function signature of
'serial8250_pci_setup_port()' has been extended with an already iomapped
address value. So this can be used directly without mapping again.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
---
 drivers/tty/serial/8250/8250_exar.c     |  4 ++--
 drivers/tty/serial/8250/8250_pci.c      | 10 +++++++++-
 drivers/tty/serial/8250/8250_pci1xxxx.c | 10 +++++-----
 drivers/tty/serial/8250/8250_pcilib.c   |  7 ++-----
 drivers/tty/serial/8250/8250_pcilib.h   |  2 +-
 5 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 04a0cbab02c2..3c16a849b474 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -503,7 +503,7 @@ static int default_setup(struct exar8250 *priv, struct pci_dev *pcidev,
 	unsigned char status;
 	int err;
 
-	err = serial8250_pci_setup_port(pcidev, port, 0, offset, board->reg_shift);
+	err = serial8250_pci_setup_port(pcidev, port, 0, offset, board->reg_shift, priv->virt);
 	if (err)
 		return err;
 
@@ -831,7 +831,7 @@ static int cti_port_setup_common(struct exar8250 *priv,
 	port->port.port_id = idx;
 	port->port.uartclk = priv->osc_freq;
 
-	ret = serial8250_pci_setup_port(pcidev, port, 0, offset, 0);
+	ret = serial8250_pci_setup_port(pcidev, port, 0, offset, 0, priv->virt);
 	if (ret)
 		return ret;
 
diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index 152f914c599d..edd5f28f04c6 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 (IS_ERR(iomem))
+			return -ENOMEM;
+	}
+
+	return serial8250_pci_setup_port(priv->dev, port, bar, offset, regshift, iomem);
 }
 
 /*
diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c
index 4c149db84692..feeede164886 100644
--- a/drivers/tty/serial/8250/8250_pci1xxxx.c
+++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
@@ -671,7 +671,7 @@ static int pci1xxxx_resume(struct device *dev)
 }
 
 static int pci1xxxx_setup(struct pci_dev *pdev,
-			  struct uart_8250_port *port, int port_idx, int rev)
+			  struct uart_8250_port *port, int port_idx, struct pci1xxxx_8250 *priv)
 {
 	int ret;
 
@@ -698,12 +698,12 @@ static int pci1xxxx_setup(struct pci_dev *pdev,
 	 * C0 and later revisions support Burst operation.
 	 * RTS workaround in mctrl is applicable only to B0.
 	 */
-	if (rev >= 0xC0)
+	if (priv->dev_rev >= 0xC0)
 		port->port.handle_irq = pci1xxxx_handle_irq;
-	else if (rev == 0xB0)
+	else if (priv->dev_rev == 0xB0)
 		port->port.set_mctrl = pci1xxxx_set_mctrl;
 
-	ret = serial8250_pci_setup_port(pdev, port, 0, PORT_OFFSET * port_idx, 0);
+	ret = serial8250_pci_setup_port(pdev, port, 0, PORT_OFFSET * port_idx, 0, priv->membase);
 	if (ret < 0)
 		return ret;
 
@@ -821,7 +821,7 @@ static int pci1xxxx_serial_probe(struct pci_dev *pdev,
 		else
 			uart.port.irq = pci_irq_vector(pdev, 0);
 
-		rc = pci1xxxx_setup(pdev, &uart, port_idx, priv->dev_rev);
+		rc = pci1xxxx_setup(pdev, &uart, port_idx, priv);
 		if (rc) {
 			dev_warn(dev, "Failed to setup port %u\n", i);
 			continue;
diff --git a/drivers/tty/serial/8250/8250_pcilib.c b/drivers/tty/serial/8250/8250_pcilib.c
index d8d0ae0d7238..9d5d2531a33b 100644
--- a/drivers/tty/serial/8250/8250_pcilib.c
+++ b/drivers/tty/serial/8250/8250_pcilib.c
@@ -22,19 +22,16 @@ int serial_8250_warn_need_ioport(struct pci_dev *dev)
 EXPORT_SYMBOL_NS_GPL(serial_8250_warn_need_ioport, "SERIAL_8250_PCI");
 
 int serial8250_pci_setup_port(struct pci_dev *dev, struct uart_8250_port *port,
-		   u8 bar, unsigned int offset, int regshift)
+		   u8 bar, unsigned int offset, int regshift, void __iomem *iomem)
 {
 	if (bar >= PCI_STD_NUM_BARS)
 		return -EINVAL;
 
 	if (pci_resource_flags(dev, bar) & IORESOURCE_MEM) {
-		if (!pcim_iomap(dev, bar, 0) && !pcim_iomap_table(dev))
-			return -ENOMEM;
-
 		port->port.iotype = UPIO_MEM;
 		port->port.iobase = 0;
 		port->port.mapbase = pci_resource_start(dev, bar) + offset;
-		port->port.membase = pcim_iomap_table(dev)[bar] + offset;
+		port->port.membase = iomem + offset;
 		port->port.regshift = regshift;
 	} else if (IS_ENABLED(CONFIG_HAS_IOPORT)) {
 		port->port.iotype = UPIO_PORT;
diff --git a/drivers/tty/serial/8250/8250_pcilib.h b/drivers/tty/serial/8250/8250_pcilib.h
index 16a274574cde..ab18de8d1355 100644
--- a/drivers/tty/serial/8250/8250_pcilib.h
+++ b/drivers/tty/serial/8250/8250_pcilib.h
@@ -12,6 +12,6 @@ struct pci_dev;
 struct uart_8250_port;
 
 int serial8250_pci_setup_port(struct pci_dev *dev, struct uart_8250_port *port, u8 bar,
-		   unsigned int offset, int regshift);
+		   unsigned int offset, int regshift, void __iomem *iomem);
 
 int serial_8250_warn_need_ioport(struct pci_dev *dev);

base-commit: 07e27ad16399afcd693be20211b0dfae63e0615f
-- 
2.39.5

Re: [PATCH] serial: 8250_pcilib: Replace deprecated PCI functions

Posted by Jiri Slaby 5 days, 15 hours ago

On 24. 09. 25, 15:35, Florian Eckert wrote:
> When the '8250_exar' module is loaded in to the kernel, a kernel trace
> with 'WARN_ON(legacy_iomap_table[bar])' is dumped to the console,
> because the old pci table mapping is still used in '8250_pcilib'.
> 
> The old function have been deprecated in commit e354bb84a4c1 ("PCI:
> Deprecate pcim_iomap_table(), pcim_iomap_regions_request_all()").
> 
> The remapping already takes or must take place in the driver that calls
> the function 'serial8250_pci_setup_port()'. The remapping should only be
> called once via 'pcim_iomap()'. Therefore the remapping moved to the
> caller of 'serial8250_pci_setup_port()'.
> 
> To use the new functions in '8250_pcilib' the function signature of
> 'serial8250_pci_setup_port()' has been extended with an already iomapped
> address value. So this can be used directly without mapping again.
> 
> Signed-off-by: Florian Eckert <fe@dev.tdt.de>
...
> --- 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 (IS_ERR(iomem))
> +			return -ENOMEM;

Why not to propagate the error?

Other than that, LGTM.


-- 
js
suse labs

Re: [PATCH] serial: 8250_pcilib: Replace deprecated PCI functions

Posted by Florian Eckert 5 days, 10 hours ago

>> Signed-off-by: Florian Eckert <fe@dev.tdt.de>
> ...
>> --- 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 (IS_ERR(iomem))
>> +			return -ENOMEM;
> 
> Why not to propagate the error?

Most other calls in the kernel of this function return
-ENOMEM on error. Therefore, I thought that this is the
correct return value. I can fix that in v2 if you like.
Let me know what you prefer.

> 
> Other than that, LGTM.

Thanks for the review :-)

Re: [PATCH] serial: 8250_pcilib: Replace deprecated PCI functions

Posted by Jiri Slaby 2 days, 17 hours ago

On 26. 09. 25, 14:31, Florian Eckert wrote:
>>> Signed-off-by: Florian Eckert <fe@dev.tdt.de>
>> ...
>>> --- 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 (IS_ERR(iomem))
>>> +            return -ENOMEM;
>>
>> Why not to propagate the error?
> 
> Most other calls in the kernel of this function return
> -ENOMEM on error. Therefore, I thought that this is the
> correct return value. I can fix that in v2 if you like.
> Let me know what you prefer.

Ugh, pcim_iomap() returns NULL on error, so the IS_ERR() check is all 
wrong. What other places in the kernel use IS_ERR()? They need fixing.

thanks,
-- 
js
suse labs

Re: [PATCH] serial: 8250_pcilib: Replace deprecated PCI functions

Posted by Florian Eckert 2 days, 16 hours ago

On 2025-09-29 08:00, Jiri Slaby wrote:
> On 26. 09. 25, 14:31, Florian Eckert wrote:
>>>> Signed-off-by: Florian Eckert <fe@dev.tdt.de>
>>> ...
>>>> --- 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 (IS_ERR(iomem))
>>>> +            return -ENOMEM;
>>> 
>>> Why not to propagate the error?
>> 
>> Most other calls in the kernel of this function return
>> -ENOMEM on error. Therefore, I thought that this is the
>> correct return value. I can fix that in v2 if you like.
>> Let me know what you prefer.
> 
> Ugh, pcim_iomap() returns NULL on error, so the IS_ERR() check is all
> wrong. What other places in the kernel use IS_ERR()? They need fixing.

You're right. The function returns a pointer here and my check IS_ERR()
is wrong. I mixed it up with the 'pcim_iomap_region()' function.
Because I wanted to use this function before. As far as I can see, the 
return
value of 'pcim_iomap()' is checked correctly in the linux sources.

Sorry for the noise.

I will adjust this and send a V2 patch.

--
Florian