Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 6 Jun 2023 12:31:31 +0200
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Ilpo =?iso-8859-1?Q?J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
Cc:     Chen-Yu Tsai <wenst@chromium.org>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>,
        Jiri Slaby <jirislaby@kernel.org>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        John Ogness <john.ogness@linutronix.de>,
        linux-serial <linux-serial@vger.kernel.org>,
        linux-mediatek@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org,
        LKML <linux-kernel@vger.kernel.org>,
        Tony Lindgren <tony@atomide.com>
Subject: Re: [PATCH] serial: 8250_mtk: Simplify clock sequencing and runtime
 PM
Message-ID: <2023060608-bucket-footsore-9e43@gregkh>
References: <20230606091747.2031168-1-wenst@chromium.org>
 <58949bbd-1506-90a0-7154-e6e57d8ddf70@collabora.com>
 <CAGXv+5GRBOWFMw+BmkSpczHcm_R6=mvL2GSKnWWyhpng1xw21w@mail.gmail.com>
 <61ec34d0-6cc2-951e-c990-fdfd57381d7@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <61ec34d0-6cc2-951e-c990-fdfd57381d7@linux.intel.com>
Precedence: bulk

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@collabora.com> 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@atomide.com/
> > > > [2] https://lore.kernel.org/linux-serial/20230605061511.GW14287@atomide.com/
> > > >
> > > > Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
> > > > Suggested-by: Tony Lindgren <tony@atomide.com>
> > > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > >
> > > 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