Re: [PATCH v2] serial: 8250_uniphier: Use devm_clk_get_enabled()

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Mon, Sep 28, 2026 at 03:51:09PM +0900, Kunihiko Hayashi wrote:
> On 2026/09/25 22:37, Andy Shevchenko wrote:
> > On Fri, Sep 25, 2026 at 08:32:20PM +0900, Kunihiko Hayashi wrote:
> > > On 2026/09/24 23:09, Andy Shevchenko wrote:
> > > > On Thu, Sep 24, 2026 at 06:07:37PM +0900, Kunihiko Hayashi wrote:
> > > > > On 2026/09/17 13:05, Malathi A wrote:

...

> > > > > I'm a bit concerned about the error path in uniphier_uart_resume().
> > > > > The clock may have been disabled in uniphier_uart_suspend(), and
> > > > > clk_prepare_enable() can fail when trying to enable it again.
> > > > > 
> > > > > In that case, wouldn't devres try to disable an already disabled
> > > > > clock on detach?
> > > > 
> > > > Isn't there is a guarantee that the .remove() is called with PM
> > runtime on
> > 
> > After reading a code of driver core I see that this is the opposite
> > actually.
> > The PM runtime might be off.
> > 
> > > > and get, meaning it's called with the enabled clock? But if not the
> > case,
> > > > we might use PM runtime force operations (resume and suspend in the
> > > > respective cases.
> > > 
> > > My concern was just the case where clk_prepare_enable() in the system
> > > resume callback fails, leaving the clock disabled before devres cleanup.
> > > 
> > > This driver currently only uses SET_SYSTEM_SLEEP_PM_OPS(), so I wasn't
> > > considering runtime PM here.
> > 
> > So, in such a case how do you see the scenario when system is resumed
> > (Right?
> > Otherwise we can't do anything, like detaching driver from the device.)
> > and
> > clock is disabled?
> 
> Ah, I understand your point. I was assuming that the driver could later
> be detached after uniphier_uart_resume() failed, leaving the clock disabled.

I'm not sure, I don't know if I was right. Can you confirm that this scenario
is not possible? So, it might look like CPU is resumed, some of the devices
were resumed, but this particular UART failed to resume, and now we want to
detach it. If this case is possible, I believe tons of the device drivers as
of today may be affected by the same issue (it doesn't mean that the issue
is impossible to happen, one needs to investigate deeper).

> If that sequence cannot happen after a failed system resume, then my concern
> doesn't apply.

As pointed out I'm not sure. Last time I experimented with failed resume was
long time ago.

> Thanks for pointing this out.

> In that case, I have no further concerns about the use of
> devm_clk_get_enabled() here.

-- 
With Best Regards,
Andy Shevchenko