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

[Greg Kroah-Hartman]

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
> > <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.

The Fixes: tag is relevant here, please don't dissuade people from using
them.

greg k-h