linux-kernel - Re: [PATCH RESEND] Do not mark ACPI devices as irq safe

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240820035719.GA5105@atomide.com>
Date: Tue, 20 Aug 2024 06:57:19 +0300
From: Tony Lindgren <tony@...mide.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: Dmitry Osipenko <digetx@...il.com>, Breno Leitao <leitao@...ian.org>,
	dmitry.osipenko@...labora.com, Andi Shyti <andi.shyti@...nel.org>,
	Laxman Dewangan <ldewangan@...dia.com>,
	Thierry Reding <thierry.reding@...il.com>,
	Jonathan Hunter <jonathanh@...dia.com>, leit@...a.com,
	Michael van der Westhuizen <rmikey@...a.com>,
	"open list:I2C SUBSYSTEM HOST DRIVERS" <linux-i2c@...r.kernel.org>,
	"open list:TEGRA ARCHITECTURE SUPPORT" <linux-tegra@...r.kernel.org>,
	open list <linux-kernel@...r.kernel.org>,
	Kevin Hilman <khilman@...libre.com>
Subject: Re: [PATCH RESEND] Do not mark ACPI devices as irq safe

* Andy Shevchenko <andy.shevchenko@...il.com> [240819 09:21]:
> +Cc: Tony
> 
> On Thu, Aug 15, 2024 at 5:48 AM Dmitry Osipenko <digetx@...il.com> wrote:
> > 13.08.2024 18:52, Andy Shevchenko пишет:
> > ...
> > >>> but somewhere in the replies
> > >>> here I would like to hear about roadmap to get rid of the
> > >>> pm_runtime_irq_safe() in all Tegra related code.
> > >>
> > >> What is the problem with pm_runtime_irq_safe()?
> > >
> > > It's a hack. It has no reasons to stay in the kernel. It also prevents
> > > PM from working properly (in some cases, not Tegra).
> >
> > Why is it a hack? Why it can't be made to work properly for all cases?
> 
> Because it messes up with the proper power transitions of the parent
> devices. Refer to the initial commit c7b61de5b7b1 ("PM / Runtime: Add
> synchronous runtime interface for interrupt handlers (v3)") that
> pretty much explains the constraints of it. Also note, it was added
> quite a while after the main PM machinery had been introduced.
> 
> What you have to use is device links to make sure the parent (PM
> speaking) may not go away.
> FWIW, if I am not mistaken the whole reconsideration of
> pm_runtime_irq_safe() had been started with this [1] thread.
> 
> If you want to dive more into the history of this API, run `git log -S
> pm_runtime_irq_safe`. It gives you also interesting facts of how it
> was started being used and in many cases reverted or reworked for a
> reason.

Yeah we should remove pm_runtime_irq_safe() completely. Fixing the use
of it in a driver afterwards is always a pain. And so far there has
always been a better solution available for the use cases I've seen.

> > >> There were multiple
> > >> problems with RPM for this driver in the past, it wasn't trivial to make
> > >> it work for all Tegra HW generations. Don't expect anyone would want to
> > >> invest time into doing it all over again.
> > >
> > > You may always refer to the OMAP case, which used to have 12 (IIRC,
> > > but definitely several) calls to this API and now 0. Taking the OMAP
> > > case into consideration I believe it's quite possible to get rid of
> > > this hack and retire the API completely. Yes, this may take months or
> > > even years. But I would like to have this roadmap be documented.
> >
> > There should be alternative to the removed API. Otherwise drivers will
> > have to have own hacks to work around the RPM limitation, re-invent own
> > PM, or not do RPM at all.
> >
> > Looking at the i2c-omap.c, I see it's doing pm_runtime_get_sync() in the
> > atomic transfer, which should cause a lockup without IRQ-safe RPM,
> > AFAICT. The OMAP example doesn't look great so far.
> 
> Bugs may still appear, but it's not a point. I can easily find a
> better example with a hint why it's bad to call that API [2][3][4] and
> so on.

Adding Kevin for the i2c-omap.c, sounds like it might depend on the
autosuspend timeout for runtime PM.

For issues where the controller may wake to an interrupt while runtime
idle, there's pm_runtime_get_noresume().

Regards,

Tony

> [1]: https://lore.kernel.org/all/20180515183409.78046-1-andriy.shevchenko@linux.intel.com/T/#u
> [2]: https://lore.kernel.org/all/20191114101718.20619-1-peter.ujfalusi@ti.com/
> [3]: https://lore.kernel.org/all/20180920193532.7714-1-tony@atomide.com/
> [4]: https://lore.kernel.org/all/1463014396-4095-1-git-send-email-tony@atomide.com/