The 8250_mtk driver's runtime PM support has some issues:
- The bus clock is enabled (through runtime PM callback) later than a
register write
- runtime PM resume callback directly called in probe, but no
pm_runtime_set_active() call is present
- UART PM function calls the callbacks directly, _and_ calls runtime
PM API
- runtime PM callbacks try to do reference counting, adding yet another
count between runtime PM and clocks
This fragile setup worked in a way, but broke recently with runtime PM
support added to the serial core. The system would hang when the UART
console was probed and brought up.
Tony provided some potential fixes [1][2], though they were still a bit
complicated. The 8250_dw driver, which the 8250_mtk driver might have
been based on, has a similar structure but simpler runtime PM usage.
Simplify clock sequencing and runtime PM support in the 8250_mtk driver.
Specifically, the clock is acquired enabled and assumed to be active,
unless toggled through runtime PM suspend/resume. Reference counting is
removed and left to the runtime PM core. The serial pm function now
only calls the runtime PM API.
[1] https://lore.kernel.org/linux-serial/[email protected]/
[2] https://lore.kernel.org/linux-serial/[email protected]/
Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
Suggested-by: Tony Lindgren <[email protected]>
Signed-off-by: Chen-Yu Tsai <[email protected]>
---
drivers/tty/serial/8250/8250_mtk.c | 50 ++++++------------------------
1 file changed, 10 insertions(+), 40 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
index aa8e98164d68..74da5676ce67 100644
--- a/drivers/tty/serial/8250/8250_mtk.c
+++ b/drivers/tty/serial/8250/8250_mtk.c
@@ -431,12 +431,7 @@ static int __maybe_unused mtk8250_runtime_suspend(struct device *dev)
while
(serial_in(up, MTK_UART_DEBUG0));
- if (data->clk_count == 0U) {
- dev_dbg(dev, "%s clock count is 0\n", __func__);
- } else {
- clk_disable_unprepare(data->bus_clk);
- data->clk_count--;
- }
+ clk_disable_unprepare(data->bus_clk);
return 0;
}
@@ -444,19 +439,8 @@ static int __maybe_unused mtk8250_runtime_suspend(struct device *dev)
static int __maybe_unused mtk8250_runtime_resume(struct device *dev)
{
struct mtk8250_data *data = dev_get_drvdata(dev);
- int err;
- if (data->clk_count > 0U) {
- dev_dbg(dev, "%s clock count is %d\n", __func__,
- data->clk_count);
- } else {
- err = clk_prepare_enable(data->bus_clk);
- if (err) {
- dev_warn(dev, "Can't enable bus clock\n");
- return err;
- }
- data->clk_count++;
- }
+ clk_prepare_enable(data->bus_clk);
return 0;
}
@@ -465,14 +449,12 @@ static void
mtk8250_do_pm(struct uart_port *port, unsigned int state, unsigned int old)
{
if (!state)
- if (!mtk8250_runtime_resume(port->dev))
- pm_runtime_get_sync(port->dev);
+ pm_runtime_get_sync(port->dev);
serial8250_do_pm(port, state, old);
if (state)
- if (!pm_runtime_put_sync_suspend(port->dev))
- mtk8250_runtime_suspend(port->dev);
+ pm_runtime_put_sync_suspend(port->dev);
}
#ifdef CONFIG_SERIAL_8250_DMA
@@ -504,7 +486,7 @@ static int mtk8250_probe_of(struct platform_device *pdev, struct uart_port *p,
return 0;
}
- data->bus_clk = devm_clk_get(&pdev->dev, "bus");
+ data->bus_clk = devm_clk_get_enabled(&pdev->dev, "bus");
if (IS_ERR(data->bus_clk))
return PTR_ERR(data->bus_clk);
@@ -587,25 +569,16 @@ static int mtk8250_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, data);
- pm_runtime_enable(&pdev->dev);
- err = mtk8250_runtime_resume(&pdev->dev);
- if (err)
- goto err_pm_disable;
-
data->line = serial8250_register_8250_port(&uart);
- if (data->line < 0) {
- err = data->line;
- goto err_pm_disable;
- }
+ if (data->line < 0)
+ return data->line;
data->rx_wakeup_irq = platform_get_irq_optional(pdev, 1);
- return 0;
-
-err_pm_disable:
- pm_runtime_disable(&pdev->dev);
+ pm_runtime_set_active(&pdev->dev);
+ pm_runtime_enable(&pdev->dev);
- return err;
+ return 0;
}
static int mtk8250_remove(struct platform_device *pdev)
@@ -619,9 +592,6 @@ static int mtk8250_remove(struct platform_device *pdev)
pm_runtime_disable(&pdev->dev);
pm_runtime_put_noidle(&pdev->dev);
- if (!pm_runtime_status_suspended(&pdev->dev))
- mtk8250_runtime_suspend(&pdev->dev);
-
return 0;
}
--
2.41.0.rc0.172.g3f132b7071-goog
Il 06/06/23 11:17, Chen-Yu Tsai ha scritto:
> The 8250_mtk driver's runtime PM support has some issues:
>
> - The bus clock is enabled (through runtime PM callback) later than a
> register write
> - runtime PM resume callback directly called in probe, but no
> pm_runtime_set_active() call is present
> - UART PM function calls the callbacks directly, _and_ calls runtime
> PM API
> - runtime PM callbacks try to do reference counting, adding yet another
> count between runtime PM and clocks
>
> This fragile setup worked in a way, but broke recently with runtime PM
> support added to the serial core. The system would hang when the UART
> console was probed and brought up.
>
> Tony provided some potential fixes [1][2], though they were still a bit
> complicated. The 8250_dw driver, which the 8250_mtk driver might have
> been based on, has a similar structure but simpler runtime PM usage.
>
> Simplify clock sequencing and runtime PM support in the 8250_mtk driver.
> Specifically, the clock is acquired enabled and assumed to be active,
> unless toggled through runtime PM suspend/resume. Reference counting is
> removed and left to the runtime PM core. The serial pm function now
> only calls the runtime PM API.
>
> [1] https://lore.kernel.org/linux-serial/[email protected]/
> [2] https://lore.kernel.org/linux-serial/[email protected]/
>
> Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
> Suggested-by: Tony Lindgren <[email protected]>
> Signed-off-by: Chen-Yu Tsai <[email protected]>
You're both cleaning this up and solving a critical issue and I completely agree
about doing that.
I can imagine what actually fixes the driver, but still, is it possible to split
this commit in two?
One that solves the issue, one that performs the much needed cleanups.
If it's not possible, then we can leave this commit as it is... and if the problem
about splitting is the Fixes tag... well, we don't forcefully need it: after all,
issues started arising after runtime PM support for 8250 landed and before that the
driver technically worked, even though it was fragile.
Thanks,
Angelo
> ---
> drivers/tty/serial/8250/8250_mtk.c | 50 ++++++------------------------
> 1 file changed, 10 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
> index aa8e98164d68..74da5676ce67 100644
> --- a/drivers/tty/serial/8250/8250_mtk.c
> +++ b/drivers/tty/serial/8250/8250_mtk.c
> @@ -431,12 +431,7 @@ static int __maybe_unused mtk8250_runtime_suspend(struct device *dev)
> while
> (serial_in(up, MTK_UART_DEBUG0));
>
> - if (data->clk_count == 0U) {
> - dev_dbg(dev, "%s clock count is 0\n", __func__);
> - } else {
> - clk_disable_unprepare(data->bus_clk);
> - data->clk_count--;
> - }
> + clk_disable_unprepare(data->bus_clk);
>
> return 0;
> }
> @@ -444,19 +439,8 @@ static int __maybe_unused mtk8250_runtime_suspend(struct device *dev)
> static int __maybe_unused mtk8250_runtime_resume(struct device *dev)
> {
> struct mtk8250_data *data = dev_get_drvdata(dev);
> - int err;
>
> - if (data->clk_count > 0U) {
> - dev_dbg(dev, "%s clock count is %d\n", __func__,
> - data->clk_count);
> - } else {
> - err = clk_prepare_enable(data->bus_clk);
> - if (err) {
> - dev_warn(dev, "Can't enable bus clock\n");
> - return err;
> - }
> - data->clk_count++;
> - }
> + clk_prepare_enable(data->bus_clk);
>
> return 0;
> }
> @@ -465,14 +449,12 @@ static void
> mtk8250_do_pm(struct uart_port *port, unsigned int state, unsigned int old)
> {
> if (!state)
> - if (!mtk8250_runtime_resume(port->dev))
> - pm_runtime_get_sync(port->dev);
> + pm_runtime_get_sync(port->dev);
>
> serial8250_do_pm(port, state, old);
>
> if (state)
> - if (!pm_runtime_put_sync_suspend(port->dev))
> - mtk8250_runtime_suspend(port->dev);
> + pm_runtime_put_sync_suspend(port->dev);
> }
>
> #ifdef CONFIG_SERIAL_8250_DMA
> @@ -504,7 +486,7 @@ static int mtk8250_probe_of(struct platform_device *pdev, struct uart_port *p,
> return 0;
> }
>
> - data->bus_clk = devm_clk_get(&pdev->dev, "bus");
> + data->bus_clk = devm_clk_get_enabled(&pdev->dev, "bus");
> if (IS_ERR(data->bus_clk))
> return PTR_ERR(data->bus_clk);
>
> @@ -587,25 +569,16 @@ static int mtk8250_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, data);
>
> - pm_runtime_enable(&pdev->dev);
> - err = mtk8250_runtime_resume(&pdev->dev);
> - if (err)
> - goto err_pm_disable;
> -
> data->line = serial8250_register_8250_port(&uart);
> - if (data->line < 0) {
> - err = data->line;
> - goto err_pm_disable;
> - }
> + if (data->line < 0)
> + return data->line;
>
> data->rx_wakeup_irq = platform_get_irq_optional(pdev, 1);
>
> - return 0;
> -
> -err_pm_disable:
> - pm_runtime_disable(&pdev->dev);
> + pm_runtime_set_active(&pdev->dev);
> + pm_runtime_enable(&pdev->dev);
>
> - return err;
> + return 0;
> }
>
> static int mtk8250_remove(struct platform_device *pdev)
> @@ -619,9 +592,6 @@ static int mtk8250_remove(struct platform_device *pdev)
> pm_runtime_disable(&pdev->dev);
> pm_runtime_put_noidle(&pdev->dev);
>
> - if (!pm_runtime_status_suspended(&pdev->dev))
> - mtk8250_runtime_suspend(&pdev->dev);
> -
> return 0;
> }
>
On Tue, 6 Jun 2023, Chen-Yu Tsai wrote:
> On Tue, Jun 6, 2023 at 5:36 PM AngeloGioacchino Del Regno
> <[email protected]> wrote:
> >
> > Il 06/06/23 11:17, Chen-Yu Tsai ha scritto:
> > > The 8250_mtk driver's runtime PM support has some issues:
> > >
> > > - The bus clock is enabled (through runtime PM callback) later than a
> > > register write
> > > - runtime PM resume callback directly called in probe, but no
> > > pm_runtime_set_active() call is present
> > > - UART PM function calls the callbacks directly, _and_ calls runtime
> > > PM API
> > > - runtime PM callbacks try to do reference counting, adding yet another
> > > count between runtime PM and clocks
> > >
> > > This fragile setup worked in a way, but broke recently with runtime PM
> > > support added to the serial core. The system would hang when the UART
> > > console was probed and brought up.
> > >
> > > Tony provided some potential fixes [1][2], though they were still a bit
> > > complicated. The 8250_dw driver, which the 8250_mtk driver might have
> > > been based on, has a similar structure but simpler runtime PM usage.
> > >
> > > Simplify clock sequencing and runtime PM support in the 8250_mtk driver.
> > > Specifically, the clock is acquired enabled and assumed to be active,
> > > unless toggled through runtime PM suspend/resume. Reference counting is
> > > removed and left to the runtime PM core. The serial pm function now
> > > only calls the runtime PM API.
> > >
> > > [1] https://lore.kernel.org/linux-serial/[email protected]/
> > > [2] https://lore.kernel.org/linux-serial/[email protected]/
> > >
> > > Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
> > > Suggested-by: Tony Lindgren <[email protected]>
> > > Signed-off-by: Chen-Yu Tsai <[email protected]>
> >
> > You're both cleaning this up and solving a critical issue and I
> > completely agree about doing that.
> >
> > I can imagine what actually fixes the driver, but still, is it
> > possible to split this commit in two?
> > One that solves the issue, one that performs the much needed cleanups.
> >
> > If it's not possible, then we can leave this commit as it is... and if the problem
> > about splitting is the Fixes tag... well, we don't forcefully need it: after all,
> > issues started arising after runtime PM support for 8250 landed and before that the
> > driver technically worked, even though it was fragile.
>
> The pure fix would look like what Tony posted [1]. However it would add stuff
> that isn't strictly needed after the cleanup. Doing it in one patch results
> in less churn. Think of it another way: it's a nice cleanup that just so
> happens to fix a regression.
>
> As for the fixes tag, it's there so other people potentially doing backports
> of the 8250 runtime PM work can spot this followup fix.
Tony's patch is recent enough to not have progressed beyond tty-next so
fixing it shouldn't really require paying that much attention to stable
rules wrt. Fixes tag and minimality.
As the target currently is tty-next, a cleanup which also happens to fix
the issue seems perfectly fine.
--
i.
On Tue, Jun 6, 2023 at 5:36 PM AngeloGioacchino Del Regno
<[email protected]> wrote:
>
> Il 06/06/23 11:17, Chen-Yu Tsai ha scritto:
> > The 8250_mtk driver's runtime PM support has some issues:
> >
> > - The bus clock is enabled (through runtime PM callback) later than a
> > register write
> > - runtime PM resume callback directly called in probe, but no
> > pm_runtime_set_active() call is present
> > - UART PM function calls the callbacks directly, _and_ calls runtime
> > PM API
> > - runtime PM callbacks try to do reference counting, adding yet another
> > count between runtime PM and clocks
> >
> > This fragile setup worked in a way, but broke recently with runtime PM
> > support added to the serial core. The system would hang when the UART
> > console was probed and brought up.
> >
> > Tony provided some potential fixes [1][2], though they were still a bit
> > complicated. The 8250_dw driver, which the 8250_mtk driver might have
> > been based on, has a similar structure but simpler runtime PM usage.
> >
> > Simplify clock sequencing and runtime PM support in the 8250_mtk driver.
> > Specifically, the clock is acquired enabled and assumed to be active,
> > unless toggled through runtime PM suspend/resume. Reference counting is
> > removed and left to the runtime PM core. The serial pm function now
> > only calls the runtime PM API.
> >
> > [1] https://lore.kernel.org/linux-serial/[email protected]/
> > [2] https://lore.kernel.org/linux-serial/[email protected]/
> >
> > Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
> > Suggested-by: Tony Lindgren <[email protected]>
> > Signed-off-by: Chen-Yu Tsai <[email protected]>
>
> You're both cleaning this up and solving a critical issue and I completely agree
> about doing that.
>
> I can imagine what actually fixes the driver, but still, is it possible to split
> this commit in two?
> One that solves the issue, one that performs the much needed cleanups.
>
> If it's not possible, then we can leave this commit as it is... and if the problem
> about splitting is the Fixes tag... well, we don't forcefully need it: after all,
> issues started arising after runtime PM support for 8250 landed and before that the
> driver technically worked, even though it was fragile.
The pure fix would look like what Tony posted [1]. However it would add stuff
that isn't strictly needed after the cleanup. Doing it in one patch results
in less churn. Think of it another way: it's a nice cleanup that just so
happens to fix a regression.
As for the fixes tag, it's there so other people potentially doing backports
of the 8250 runtime PM work can spot this followup fix.
ChenYu
[1] https://lore.kernel.org/linux-serial/[email protected]/
> Thanks,
> Angelo
>
> > ---
> > drivers/tty/serial/8250/8250_mtk.c | 50 ++++++------------------------
> > 1 file changed, 10 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
> > index aa8e98164d68..74da5676ce67 100644
> > --- a/drivers/tty/serial/8250/8250_mtk.c
> > +++ b/drivers/tty/serial/8250/8250_mtk.c
> > @@ -431,12 +431,7 @@ static int __maybe_unused mtk8250_runtime_suspend(struct device *dev)
> > while
> > (serial_in(up, MTK_UART_DEBUG0));
> >
> > - if (data->clk_count == 0U) {
> > - dev_dbg(dev, "%s clock count is 0\n", __func__);
> > - } else {
> > - clk_disable_unprepare(data->bus_clk);
> > - data->clk_count--;
> > - }
> > + clk_disable_unprepare(data->bus_clk);
> >
> > return 0;
> > }
> > @@ -444,19 +439,8 @@ static int __maybe_unused mtk8250_runtime_suspend(struct device *dev)
> > static int __maybe_unused mtk8250_runtime_resume(struct device *dev)
> > {
> > struct mtk8250_data *data = dev_get_drvdata(dev);
> > - int err;
> >
> > - if (data->clk_count > 0U) {
> > - dev_dbg(dev, "%s clock count is %d\n", __func__,
> > - data->clk_count);
> > - } else {
> > - err = clk_prepare_enable(data->bus_clk);
> > - if (err) {
> > - dev_warn(dev, "Can't enable bus clock\n");
> > - return err;
> > - }
> > - data->clk_count++;
> > - }
> > + clk_prepare_enable(data->bus_clk);
> >
> > return 0;
> > }
> > @@ -465,14 +449,12 @@ static void
> > mtk8250_do_pm(struct uart_port *port, unsigned int state, unsigned int old)
> > {
> > if (!state)
> > - if (!mtk8250_runtime_resume(port->dev))
> > - pm_runtime_get_sync(port->dev);
> > + pm_runtime_get_sync(port->dev);
> >
> > serial8250_do_pm(port, state, old);
> >
> > if (state)
> > - if (!pm_runtime_put_sync_suspend(port->dev))
> > - mtk8250_runtime_suspend(port->dev);
> > + pm_runtime_put_sync_suspend(port->dev);
> > }
> >
> > #ifdef CONFIG_SERIAL_8250_DMA
> > @@ -504,7 +486,7 @@ static int mtk8250_probe_of(struct platform_device *pdev, struct uart_port *p,
> > return 0;
> > }
> >
> > - data->bus_clk = devm_clk_get(&pdev->dev, "bus");
> > + data->bus_clk = devm_clk_get_enabled(&pdev->dev, "bus");
> > if (IS_ERR(data->bus_clk))
> > return PTR_ERR(data->bus_clk);
> >
> > @@ -587,25 +569,16 @@ static int mtk8250_probe(struct platform_device *pdev)
> >
> > platform_set_drvdata(pdev, data);
> >
> > - pm_runtime_enable(&pdev->dev);
> > - err = mtk8250_runtime_resume(&pdev->dev);
> > - if (err)
> > - goto err_pm_disable;
> > -
> > data->line = serial8250_register_8250_port(&uart);
> > - if (data->line < 0) {
> > - err = data->line;
> > - goto err_pm_disable;
> > - }
> > + if (data->line < 0)
> > + return data->line;
> >
> > data->rx_wakeup_irq = platform_get_irq_optional(pdev, 1);
> >
> > - return 0;
> > -
> > -err_pm_disable:
> > - pm_runtime_disable(&pdev->dev);
> > + pm_runtime_set_active(&pdev->dev);
> > + pm_runtime_enable(&pdev->dev);
> >
> > - return err;
> > + return 0;
> > }
> >
> > static int mtk8250_remove(struct platform_device *pdev)
> > @@ -619,9 +592,6 @@ static int mtk8250_remove(struct platform_device *pdev)
> > pm_runtime_disable(&pdev->dev);
> > pm_runtime_put_noidle(&pdev->dev);
> >
> > - if (!pm_runtime_status_suspended(&pdev->dev))
> > - mtk8250_runtime_suspend(&pdev->dev);
> > -
> > return 0;
> > }
> >
>
>
On Tue, 6 Jun 2023, Greg Kroah-Hartman wrote:
> On Tue, Jun 06, 2023 at 01:21:55PM +0300, Ilpo Järvinen wrote:
> > On Tue, 6 Jun 2023, Chen-Yu Tsai wrote:
> >
> > > On Tue, Jun 6, 2023 at 5:36 PM AngeloGioacchino Del Regno
> > > <[email protected]> wrote:
> > > >
> > > > Il 06/06/23 11:17, Chen-Yu Tsai ha scritto:
> > > > > The 8250_mtk driver's runtime PM support has some issues:
> > > > >
> > > > > - The bus clock is enabled (through runtime PM callback) later than a
> > > > > register write
> > > > > - runtime PM resume callback directly called in probe, but no
> > > > > pm_runtime_set_active() call is present
> > > > > - UART PM function calls the callbacks directly, _and_ calls runtime
> > > > > PM API
> > > > > - runtime PM callbacks try to do reference counting, adding yet another
> > > > > count between runtime PM and clocks
> > > > >
> > > > > This fragile setup worked in a way, but broke recently with runtime PM
> > > > > support added to the serial core. The system would hang when the UART
> > > > > console was probed and brought up.
> > > > >
> > > > > Tony provided some potential fixes [1][2], though they were still a bit
> > > > > complicated. The 8250_dw driver, which the 8250_mtk driver might have
> > > > > been based on, has a similar structure but simpler runtime PM usage.
> > > > >
> > > > > Simplify clock sequencing and runtime PM support in the 8250_mtk driver.
> > > > > Specifically, the clock is acquired enabled and assumed to be active,
> > > > > unless toggled through runtime PM suspend/resume. Reference counting is
> > > > > removed and left to the runtime PM core. The serial pm function now
> > > > > only calls the runtime PM API.
> > > > >
> > > > > [1] https://lore.kernel.org/linux-serial/[email protected]/
> > > > > [2] https://lore.kernel.org/linux-serial/[email protected]/
> > > > >
> > > > > Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
> > > > > Suggested-by: Tony Lindgren <[email protected]>
> > > > > Signed-off-by: Chen-Yu Tsai <[email protected]>
> > > >
> > > > You're both cleaning this up and solving a critical issue and I
> > > > completely agree about doing that.
> > > >
> > > > I can imagine what actually fixes the driver, but still, is it
> > > > possible to split this commit in two?
> > > > One that solves the issue, one that performs the much needed cleanups.
> > > >
> > > > If it's not possible, then we can leave this commit as it is... and if the problem
> > > > about splitting is the Fixes tag... well, we don't forcefully need it: after all,
> > > > issues started arising after runtime PM support for 8250 landed and before that the
> > > > driver technically worked, even though it was fragile.
> > >
> > > The pure fix would look like what Tony posted [1]. However it would add stuff
> > > that isn't strictly needed after the cleanup. Doing it in one patch results
> > > in less churn. Think of it another way: it's a nice cleanup that just so
> > > happens to fix a regression.
> > >
> > > As for the fixes tag, it's there so other people potentially doing backports
> > > of the 8250 runtime PM work can spot this followup fix.
> >
> > Tony's patch is recent enough to not have progressed beyond tty-next so
> > fixing it shouldn't really require paying that much attention to stable
> > rules wrt. Fixes tag and minimality.
> >
> > As the target currently is tty-next, a cleanup which also happens to fix
> > the issue seems perfectly fine.
>
> The Fixes: tag is relevant here, please don't dissuade people from using
> them.
Not including Fixes tag was definitely among the things I tried to
say.
What I meant was that this patch, given the current time frame,
likely gets included within the same "next cycle". So it fixes the issue
even before the commit it fixes ends up into any rc not to speak of
stables. As such, stable kernel rules don't seem relevant.
So creating a "fix patch" with extra stuff just for the sake of it and
then removing the extra stuff away in the cleanup change seems unnecessary
to me as the cleanup itself would fix the same issue. I'm not even sure if
it's proper to call this change a cleanup, it just takes a different
avenue to fix the problem by removing some complexity from old.
--
i.
On Tue, Jun 06, 2023 at 01:21:55PM +0300, Ilpo Järvinen wrote:
> On Tue, 6 Jun 2023, Chen-Yu Tsai wrote:
>
> > On Tue, Jun 6, 2023 at 5:36 PM AngeloGioacchino Del Regno
> > <[email protected]> wrote:
> > >
> > > Il 06/06/23 11:17, Chen-Yu Tsai ha scritto:
> > > > The 8250_mtk driver's runtime PM support has some issues:
> > > >
> > > > - The bus clock is enabled (through runtime PM callback) later than a
> > > > register write
> > > > - runtime PM resume callback directly called in probe, but no
> > > > pm_runtime_set_active() call is present
> > > > - UART PM function calls the callbacks directly, _and_ calls runtime
> > > > PM API
> > > > - runtime PM callbacks try to do reference counting, adding yet another
> > > > count between runtime PM and clocks
> > > >
> > > > This fragile setup worked in a way, but broke recently with runtime PM
> > > > support added to the serial core. The system would hang when the UART
> > > > console was probed and brought up.
> > > >
> > > > Tony provided some potential fixes [1][2], though they were still a bit
> > > > complicated. The 8250_dw driver, which the 8250_mtk driver might have
> > > > been based on, has a similar structure but simpler runtime PM usage.
> > > >
> > > > Simplify clock sequencing and runtime PM support in the 8250_mtk driver.
> > > > Specifically, the clock is acquired enabled and assumed to be active,
> > > > unless toggled through runtime PM suspend/resume. Reference counting is
> > > > removed and left to the runtime PM core. The serial pm function now
> > > > only calls the runtime PM API.
> > > >
> > > > [1] https://lore.kernel.org/linux-serial/[email protected]/
> > > > [2] https://lore.kernel.org/linux-serial/[email protected]/
> > > >
> > > > Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
> > > > Suggested-by: Tony Lindgren <[email protected]>
> > > > Signed-off-by: Chen-Yu Tsai <[email protected]>
> > >
> > > You're both cleaning this up and solving a critical issue and I
> > > completely agree about doing that.
> > >
> > > I can imagine what actually fixes the driver, but still, is it
> > > possible to split this commit in two?
> > > One that solves the issue, one that performs the much needed cleanups.
> > >
> > > If it's not possible, then we can leave this commit as it is... and if the problem
> > > about splitting is the Fixes tag... well, we don't forcefully need it: after all,
> > > issues started arising after runtime PM support for 8250 landed and before that the
> > > driver technically worked, even though it was fragile.
> >
> > The pure fix would look like what Tony posted [1]. However it would add stuff
> > that isn't strictly needed after the cleanup. Doing it in one patch results
> > in less churn. Think of it another way: it's a nice cleanup that just so
> > happens to fix a regression.
> >
> > As for the fixes tag, it's there so other people potentially doing backports
> > of the 8250 runtime PM work can spot this followup fix.
>
> Tony's patch is recent enough to not have progressed beyond tty-next so
> fixing it shouldn't really require paying that much attention to stable
> rules wrt. Fixes tag and minimality.
>
> As the target currently is tty-next, a cleanup which also happens to fix
> the issue seems perfectly fine.
The Fixes: tag is relevant here, please don't dissuade people from using
them.
greg k-h
Il 06/06/23 11:17, Chen-Yu Tsai ha scritto:
> The 8250_mtk driver's runtime PM support has some issues:
>
> - The bus clock is enabled (through runtime PM callback) later than a
> register write
> - runtime PM resume callback directly called in probe, but no
> pm_runtime_set_active() call is present
> - UART PM function calls the callbacks directly, _and_ calls runtime
> PM API
> - runtime PM callbacks try to do reference counting, adding yet another
> count between runtime PM and clocks
>
> This fragile setup worked in a way, but broke recently with runtime PM
> support added to the serial core. The system would hang when the UART
> console was probed and brought up.
>
> Tony provided some potential fixes [1][2], though they were still a bit
> complicated. The 8250_dw driver, which the 8250_mtk driver might have
> been based on, has a similar structure but simpler runtime PM usage.
>
> Simplify clock sequencing and runtime PM support in the 8250_mtk driver.
> Specifically, the clock is acquired enabled and assumed to be active,
> unless toggled through runtime PM suspend/resume. Reference counting is
> removed and left to the runtime PM core. The serial pm function now
> only calls the runtime PM API.
>
> [1] https://lore.kernel.org/linux-serial/[email protected]/
> [2] https://lore.kernel.org/linux-serial/[email protected]/
>
> Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
> Suggested-by: Tony Lindgren <[email protected]>
> Signed-off-by: Chen-Yu Tsai <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
> ---
> drivers/tty/serial/8250/8250_mtk.c | 50 ++++++------------------------
> 1 file changed, 10 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
> index aa8e98164d68..74da5676ce67 100644
> --- a/drivers/tty/serial/8250/8250_mtk.c
> +++ b/drivers/tty/serial/8250/8250_mtk.c
> @@ -431,12 +431,7 @@ static int __maybe_unused mtk8250_runtime_suspend(struct device *dev)
> while
> (serial_in(up, MTK_UART_DEBUG0));
>
> - if (data->clk_count == 0U) {
> - dev_dbg(dev, "%s clock count is 0\n", __func__);
> - } else {
> - clk_disable_unprepare(data->bus_clk);
> - data->clk_count--;
> - }
> + clk_disable_unprepare(data->bus_clk);
>
> return 0;
> }
> @@ -444,19 +439,8 @@ static int __maybe_unused mtk8250_runtime_suspend(struct device *dev)
> static int __maybe_unused mtk8250_runtime_resume(struct device *dev)
> {
> struct mtk8250_data *data = dev_get_drvdata(dev);
> - int err;
>
> - if (data->clk_count > 0U) {
> - dev_dbg(dev, "%s clock count is %d\n", __func__,
> - data->clk_count);
> - } else {
> - err = clk_prepare_enable(data->bus_clk);
> - if (err) {
> - dev_warn(dev, "Can't enable bus clock\n");
> - return err;
> - }
> - data->clk_count++;
> - }
> + clk_prepare_enable(data->bus_clk);
>
> return 0;
> }
> @@ -465,14 +449,12 @@ static void
> mtk8250_do_pm(struct uart_port *port, unsigned int state, unsigned int old)
> {
> if (!state)
> - if (!mtk8250_runtime_resume(port->dev))
> - pm_runtime_get_sync(port->dev);
> + pm_runtime_get_sync(port->dev);
>
> serial8250_do_pm(port, state, old);
>
> if (state)
> - if (!pm_runtime_put_sync_suspend(port->dev))
> - mtk8250_runtime_suspend(port->dev);
> + pm_runtime_put_sync_suspend(port->dev);
> }
>
> #ifdef CONFIG_SERIAL_8250_DMA
> @@ -504,7 +486,7 @@ static int mtk8250_probe_of(struct platform_device *pdev, struct uart_port *p,
> return 0;
> }
>
> - data->bus_clk = devm_clk_get(&pdev->dev, "bus");
> + data->bus_clk = devm_clk_get_enabled(&pdev->dev, "bus");
> if (IS_ERR(data->bus_clk))
> return PTR_ERR(data->bus_clk);
>
> @@ -587,25 +569,16 @@ static int mtk8250_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, data);
>
> - pm_runtime_enable(&pdev->dev);
> - err = mtk8250_runtime_resume(&pdev->dev);
> - if (err)
> - goto err_pm_disable;
> -
> data->line = serial8250_register_8250_port(&uart);
> - if (data->line < 0) {
> - err = data->line;
> - goto err_pm_disable;
> - }
> + if (data->line < 0)
> + return data->line;
>
> data->rx_wakeup_irq = platform_get_irq_optional(pdev, 1);
>
> - return 0;
> -
> -err_pm_disable:
> - pm_runtime_disable(&pdev->dev);
> + pm_runtime_set_active(&pdev->dev);
> + pm_runtime_enable(&pdev->dev);
>
> - return err;
> + return 0;
> }
>
> static int mtk8250_remove(struct platform_device *pdev)
> @@ -619,9 +592,6 @@ static int mtk8250_remove(struct platform_device *pdev)
> pm_runtime_disable(&pdev->dev);
> pm_runtime_put_noidle(&pdev->dev);
>
> - if (!pm_runtime_status_suspended(&pdev->dev))
> - mtk8250_runtime_suspend(&pdev->dev);
> -
> return 0;
> }
>
* Chen-Yu Tsai <[email protected]> [230606 09:19]:
> The 8250_mtk driver's runtime PM support has some issues:
>
> - The bus clock is enabled (through runtime PM callback) later than a
> register write
> - runtime PM resume callback directly called in probe, but no
> pm_runtime_set_active() call is present
> - UART PM function calls the callbacks directly, _and_ calls runtime
> PM API
> - runtime PM callbacks try to do reference counting, adding yet another
> count between runtime PM and clocks
>
> This fragile setup worked in a way, but broke recently with runtime PM
> support added to the serial core. The system would hang when the UART
> console was probed and brought up.
Great, looks like a good for Linux next to me:
Reviewed-by: Tony Lindgren <[email protected]>