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

[Ilpo Järvinen]

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

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.