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
