[PATCH] serial: amba-pl011: do not request memory region twice

[PATCH] serial: amba-pl011: do not request memory region twice

[Lino Sanfilippo]

The driver attempts to request and release the IO memory region for a uart
port twice:

First during the probe() function devm_ioremap_resource() is used to
allocate and map the ports memory.
Then a combo of pl011_config_port() and pl011_release_port() is used to
request/release the same memory area. These functions are called by the
serial core as soon as the uart is registered/unregistered.

However since the allocation request via devm_ioremap_resource() already
succeeds, the attempt to claim the memory again via pl011_config_port()
fails. This failure remains unnoticed, since the concerning return value is
not evaluated.
Later at module unload also the attempt to release the unclaimed memory
in pl011_release_port() fails. This time the failure results in a “Trying
to free nonexistent resource" warning printed by the serial core.

Fix these issues by removing the callbacks that implement the redundant
memory allocation/release.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---

This patch was tested on a 5.10 Raspberry Pi kernel with a CM3.


 drivers/tty/serial/amba-pl011.c | 25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index d361cd84ff8c..91670ee25485 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2183,32 +2183,13 @@ static const char *pl011_type(struct uart_port *port)
 	return uap->port.type == PORT_AMBA ? uap->type : NULL;
 }
 
-/*
- * Release the memory region(s) being used by 'port'
- */
-static void pl011_release_port(struct uart_port *port)
-{
-	release_mem_region(port->mapbase, SZ_4K);
-}
-
-/*
- * Request the memory region(s) being used by 'port'
- */
-static int pl011_request_port(struct uart_port *port)
-{
-	return request_mem_region(port->mapbase, SZ_4K, "uart-pl011")
-			!= NULL ? 0 : -EBUSY;
-}
-
 /*
  * Configure/autoconfigure the port.
  */
 static void pl011_config_port(struct uart_port *port, int flags)
 {
-	if (flags & UART_CONFIG_TYPE) {
+	if (flags & UART_CONFIG_TYPE)
 		port->type = PORT_AMBA;
-		pl011_request_port(port);
-	}
 }
 
 /*
@@ -2275,8 +2256,6 @@ static const struct uart_ops amba_pl011_pops = {
 	.flush_buffer	= pl011_dma_flush_buffer,
 	.set_termios	= pl011_set_termios,
 	.type		= pl011_type,
-	.release_port	= pl011_release_port,
-	.request_port	= pl011_request_port,
 	.config_port	= pl011_config_port,
 	.verify_port	= pl011_verify_port,
 #ifdef CONFIG_CONSOLE_POLL
@@ -2306,8 +2285,6 @@ static const struct uart_ops sbsa_uart_pops = {
 	.shutdown	= sbsa_uart_shutdown,
 	.set_termios	= sbsa_uart_set_termios,
 	.type		= pl011_type,
-	.release_port	= pl011_release_port,
-	.request_port	= pl011_request_port,
 	.config_port	= pl011_config_port,
 	.verify_port	= pl011_verify_port,
 #ifdef CONFIG_CONSOLE_POLL

base-commit: a4849f6000e29235a2707f22e39da6b897bb9543
-- 
2.33.1

Re: [PATCH] serial: amba-pl011: do not request memory region twice

[Greg KH]

On Fri, Nov 26, 2021 at 03:39:25PM +0100, Lino Sanfilippo wrote:
> The driver attempts to request and release the IO memory region for a uart
> port twice:
> 
> First during the probe() function devm_ioremap_resource() is used to
> allocate and map the ports memory.
> Then a combo of pl011_config_port() and pl011_release_port() is used to
> request/release the same memory area. These functions are called by the
> serial core as soon as the uart is registered/unregistered.
> 
> However since the allocation request via devm_ioremap_resource() already
> succeeds, the attempt to claim the memory again via pl011_config_port()
> fails. This failure remains unnoticed, since the concerning return value is
> not evaluated.
> Later at module unload also the attempt to release the unclaimed memory
> in pl011_release_port() fails. This time the failure results in a “Trying
> to free nonexistent resource" warning printed by the serial core.
> 
> Fix these issues by removing the callbacks that implement the redundant
> memory allocation/release.
> 
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
> ---
> 
> This patch was tested on a 5.10 Raspberry Pi kernel with a CM3.

What commit id does this change fix?

thanks,

greg k-h

Re: [PATCH] serial: amba-pl011: do not request memory region twice

[Lino Sanfilippo]

Hi,

On 26.11.21 at 16:28, Greg KH wrote:
> On Fri, Nov 26, 2021 at 03:39:25PM +0100, Lino Sanfilippo wrote:
>> The driver attempts to request and release the IO memory region for a uart
>> port twice:
>>
>> First during the probe() function devm_ioremap_resource() is used to
>> allocate and map the ports memory.
>> Then a combo of pl011_config_port() and pl011_release_port() is used to
>> request/release the same memory area. These functions are called by the
>> serial core as soon as the uart is registered/unregistered.
>>
>> However since the allocation request via devm_ioremap_resource() already
>> succeeds, the attempt to claim the memory again via pl011_config_port()
>> fails. This failure remains unnoticed, since the concerning return value is
>> not evaluated.
>> Later at module unload also the attempt to release the unclaimed memory
>> in pl011_release_port() fails. This time the failure results in a “Trying
>> to free nonexistent resource" warning printed by the serial core.
>>
>> Fix these issues by removing the callbacks that implement the redundant
>> memory allocation/release.
>>
>> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
>> ---
>>
>> This patch was tested on a 5.10 Raspberry Pi kernel with a CM3.
>
> What commit id does this change fix?
>
> thanks,
>
> greg k-h
>

AFAICS its commit 3873e2d7f63a ("drivers: PL011: refactor pl011_probe()") which
changed  devm_ioremap() in pl011_setup_port() to  devm_ioremap_resource().
Since the latter not only remaps but also requests the memory region it
collides with the memory request in pl011_config_port(). I will add an appropriate
Fixes tag for this.

Regards,
Lino

Re: [PATCH] serial: amba-pl011: do not request memory region twice

[Russell King (Oracle)]

On Fri, Nov 26, 2021 at 03:39:25PM +0100, Lino Sanfilippo wrote:
> The driver attempts to request and release the IO memory region for a uart
> port twice:
> 
> First during the probe() function devm_ioremap_resource() is used to
> allocate and map the ports memory.
> Then a combo of pl011_config_port() and pl011_release_port() is used to
> request/release the same memory area. These functions are called by the
> serial core as soon as the uart is registered/unregistered.
> 
> However since the allocation request via devm_ioremap_resource() already
> succeeds, the attempt to claim the memory again via pl011_config_port()
> fails. This failure remains unnoticed, since the concerning return value is
> not evaluated.
> Later at module unload also the attempt to release the unclaimed memory
> in pl011_release_port() fails. This time the failure results in a “Trying
> to free nonexistent resource" warning printed by the serial core.
> 
> Fix these issues by removing the callbacks that implement the redundant
> memory allocation/release.

I think you will also need the verify_port method to also deny changing
port->mapbase.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] serial: amba-pl011: do not request memory region twice

[Lino Sanfilippo]

Hi,

On 26.11.21 at 17:09, Russell King (Oracle) wrote:
> On Fri, Nov 26, 2021 at 03:39:25PM +0100, Lino Sanfilippo wrote:
>> The driver attempts to request and release the IO memory region for a uart
>> port twice:
>>
>> First during the probe() function devm_ioremap_resource() is used to
>> allocate and map the ports memory.
>> Then a combo of pl011_config_port() and pl011_release_port() is used to
>> request/release the same memory area. These functions are called by the
>> serial core as soon as the uart is registered/unregistered.
>>
>> However since the allocation request via devm_ioremap_resource() already
>> succeeds, the attempt to claim the memory again via pl011_config_port()
>> fails. This failure remains unnoticed, since the concerning return value is
>> not evaluated.
>> Later at module unload also the attempt to release the unclaimed memory
>> in pl011_release_port() fails. This time the failure results in a “Trying
>> to free nonexistent resource" warning printed by the serial core.
>>
>> Fix these issues by removing the callbacks that implement the redundant
>> memory allocation/release.
>
> I think you will also need the verify_port method to also deny changing
> port->mapbase.
>


Right, I will add this, thanks for the hint!

Regards,
Lino