[PATCH] serial: imx: Ensure that imx_uart_rs485_config() is called with enabled clock

[PATCH] serial: imx: Ensure that imx_uart_rs485_config() is called with enabled clock

[Christoph Niedermaier]

There are register accesses in the function imx_uart_rs485_config(). The
clock must be enabled for these accesses. This was ensured by calling it
via the function uart_rs485_config() in the probe() function within the
range where the clock is enabled. With the commit 7c7f9bc986e6 ("serial:
Deassert Transmit Enable on probe in driver-specific way") it was removed
from the probe() function and is now only called through the function
uart_add_one_port() which is located at the end of the probe() function.
But the clock is already switched off in this area. To ensure that the
clock is enabled during register access, move the disabling of the clock
to the very end of the probe() function.

Fixes: 7c7f9bc986e6 ("serial: Deassert Transmit Enable on probe in driver-specific way")
Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
---
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jirislaby@kernel.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <festevam@denx.de>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: NXP Linux Team <linux-imx@nxp.com>
Cc: Sergey Organov <sorganov@gmail.com>
Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Rob Herring <robh@kernel.org>
Cc: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
Cc: Tom Rix <trix@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Lukas Wunner <lukas@wunner.de>
To: linux-serial@vger.kernel.org
To: linux-arm-kernel@lists.infradead.org
---
 drivers/tty/serial/imx.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index e7e952bb7bb8..2ec2e8a55884 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -2419,8 +2419,6 @@ static int imx_uart_probe(struct platform_device *pdev)
 		imx_uart_writel(sport, ucr3, UCR3);
 	}
 
-	clk_disable_unprepare(sport->clk_ipg);
-
 	hrtimer_init(&sport->trigger_start_tx, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
 	hrtimer_init(&sport->trigger_stop_tx, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
 	sport->trigger_start_tx.function = imx_trigger_start_tx;
@@ -2467,7 +2465,11 @@ static int imx_uart_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, sport);
 
-	return uart_add_one_port(&imx_uart_uart_driver, &sport->port);
+	ret = uart_add_one_port(&imx_uart_uart_driver, &sport->port);
+
+	clk_disable_unprepare(sport->clk_ipg);
+
+	return ret;
 }
 
 static void imx_uart_remove(struct platform_device *pdev)
-- 
2.11.0

[PATCH] serial: imx: Ensure that imx_uart_rs485_config() is called with enabled clock

[Christoph Niedermaier]

There are register accesses in the function imx_uart_rs485_config(). The
clock must be enabled for these accesses. This was ensured by calling it
via the function uart_rs485_config() in the probe() function within the
range where the clock is enabled. With the commit 7c7f9bc986e6 ("serial:
Deassert Transmit Enable on probe in driver-specific way") it was removed
from the probe() function and is now only called through the function
uart_add_one_port() which is located at the end of the probe() function.
But the clock is already switched off in this area. To ensure that the
clock is enabled during register access, move the disabling of the clock
to the very end of the probe() function.

Fixes: 7c7f9bc986e6 ("serial: Deassert Transmit Enable on probe in driver-specific way")
Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
---
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jirislaby@kernel.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <festevam@denx.de>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: NXP Linux Team <linux-imx@nxp.com>
Cc: Sergey Organov <sorganov@gmail.com>
Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Rob Herring <robh@kernel.org>
Cc: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
Cc: Tom Rix <trix@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Lukas Wunner <lukas@wunner.de>
To: linux-serial@vger.kernel.org
To: linux-arm-kernel@lists.infradead.org
---
 drivers/tty/serial/imx.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index e7e952bb7bb8..2ec2e8a55884 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -2419,8 +2419,6 @@ static int imx_uart_probe(struct platform_device *pdev)
 		imx_uart_writel(sport, ucr3, UCR3);
 	}
 
-	clk_disable_unprepare(sport->clk_ipg);
-
 	hrtimer_init(&sport->trigger_start_tx, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
 	hrtimer_init(&sport->trigger_stop_tx, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
 	sport->trigger_start_tx.function = imx_trigger_start_tx;
@@ -2467,7 +2465,11 @@ static int imx_uart_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, sport);
 
-	return uart_add_one_port(&imx_uart_uart_driver, &sport->port);
+	ret = uart_add_one_port(&imx_uart_uart_driver, &sport->port);
+
+	clk_disable_unprepare(sport->clk_ipg);
+
+	return ret;
 }
 
 static void imx_uart_remove(struct platform_device *pdev)
-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] serial: imx: Ensure that imx_uart_rs485_config() is called with enabled clock

[Lukas Wunner]

On Fri, Dec 22, 2023 at 11:11:01PM +0100, Christoph Niedermaier wrote:
> There are register accesses in the function imx_uart_rs485_config(). The
> clock must be enabled for these accesses. This was ensured by calling it
> via the function uart_rs485_config() in the probe() function within the
> range where the clock is enabled. With the commit 7c7f9bc986e6 ("serial:
> Deassert Transmit Enable on probe in driver-specific way") it was removed
> from the probe() function and is now only called through the function
> uart_add_one_port() which is located at the end of the probe() function.
> But the clock is already switched off in this area. To ensure that the
> clock is enabled during register access, move the disabling of the clock
> to the very end of the probe() function.

Thanks for catching this and sorry for the breakage.


> @@ -2467,7 +2465,11 @@ static int imx_uart_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, sport);
>  
> -	return uart_add_one_port(&imx_uart_uart_driver, &sport->port);
> +	ret = uart_add_one_port(&imx_uart_uart_driver, &sport->port);
> +
> +	clk_disable_unprepare(sport->clk_ipg);
> +
> +	return ret;
>  }

There are a bunch of return statements in the "if (txirq > 0) ... else"
clause a little further up.  You need to add a goto label in front of
the "clk_disable_unprepare()" and change the return statements to gotos
to avoid leaking enabled clocks in those error paths.

Thanks,

Lukas

RE: [PATCH] serial: imx: Ensure that imx_uart_rs485_config() is called with enabled clock

[Christoph Niedermaier]

From: Lukas Wunner [mailto:lukas@wunner.de]
Sent: Sunday, December 24, 2023 1:33 PM
> On Fri, Dec 22, 2023 at 11:11:01PM +0100, Christoph Niedermaier wrote:
>> There are register accesses in the function imx_uart_rs485_config(). The
>> clock must be enabled for these accesses. This was ensured by calling it
>> via the function uart_rs485_config() in the probe() function within the
>> range where the clock is enabled. With the commit 7c7f9bc986e6 ("serial:
>> Deassert Transmit Enable on probe in driver-specific way") it was removed
>> from the probe() function and is now only called through the function
>> uart_add_one_port() which is located at the end of the probe() function.
>> But the clock is already switched off in this area. To ensure that the
>> clock is enabled during register access, move the disabling of the clock
>> to the very end of the probe() function.
> 
> Thanks for catching this and sorry for the breakage.
> 
> 
>> @@ -2467,7 +2465,11 @@ static int imx_uart_probe(struct platform_device *pdev)
>>
>>       platform_set_drvdata(pdev, sport);
>>
>> -     return uart_add_one_port(&imx_uart_uart_driver, &sport->port);
>> +     ret = uart_add_one_port(&imx_uart_uart_driver, &sport->port);
>> +
>> +     clk_disable_unprepare(sport->clk_ipg);
>> +
>> +     return ret;
>>  }
> 
> There are a bunch of return statements in the "if (txirq > 0) ... else"
> clause a little further up.  You need to add a goto label in front of
> the "clk_disable_unprepare()" and change the return statements to gotos
> to avoid leaking enabled clocks in those error paths.

You are right. I overlooked that.
I will fix that in version 2.
 

Thanks and Regards
Christoph

RE: [PATCH] serial: imx: Ensure that imx_uart_rs485_config() is called with enabled clock

[Christoph Niedermaier]

From: Lukas Wunner [mailto:lukas@wunner.de]
Sent: Sunday, December 24, 2023 1:33 PM
> On Fri, Dec 22, 2023 at 11:11:01PM +0100, Christoph Niedermaier wrote:
>> There are register accesses in the function imx_uart_rs485_config(). The
>> clock must be enabled for these accesses. This was ensured by calling it
>> via the function uart_rs485_config() in the probe() function within the
>> range where the clock is enabled. With the commit 7c7f9bc986e6 ("serial:
>> Deassert Transmit Enable on probe in driver-specific way") it was removed
>> from the probe() function and is now only called through the function
>> uart_add_one_port() which is located at the end of the probe() function.
>> But the clock is already switched off in this area. To ensure that the
>> clock is enabled during register access, move the disabling of the clock
>> to the very end of the probe() function.
> 
> Thanks for catching this and sorry for the breakage.
> 
> 
>> @@ -2467,7 +2465,11 @@ static int imx_uart_probe(struct platform_device *pdev)
>>
>>       platform_set_drvdata(pdev, sport);
>>
>> -     return uart_add_one_port(&imx_uart_uart_driver, &sport->port);
>> +     ret = uart_add_one_port(&imx_uart_uart_driver, &sport->port);
>> +
>> +     clk_disable_unprepare(sport->clk_ipg);
>> +
>> +     return ret;
>>  }
> 
> There are a bunch of return statements in the "if (txirq > 0) ... else"
> clause a little further up.  You need to add a goto label in front of
> the "clk_disable_unprepare()" and change the return statements to gotos
> to avoid leaking enabled clocks in those error paths.

You are right. I overlooked that.
I will fix that in version 2.
 

Thanks and Regards
Christoph
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel