Re: [PATCH] serial: 8250_mtk: Simplify clock sequencing and runtime PM

[Ilpo Järvinen]

On Tue, 6 Jun 2023, Chen-Yu Tsai wrote:

> On Tue, Jun 6, 2023 at 5:36 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@xxxxxxxxxxxxx> 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/20230602092701.GP14287@xxxxxxxxxxx/
> > > [2] https://lore.kernel.org/linux-serial/20230605061511.GW14287@xxxxxxxxxxx/
> > >
> > > Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
> > > Suggested-by: Tony Lindgren <tony@xxxxxxxxxxx>
> > > Signed-off-by: Chen-Yu Tsai <wenst@xxxxxxxxxxxx>
> >
> > 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.