Re: [PATCH] ACPI: Make custom_method use per-open state

[Rafael J. Wysocki]

On Thu, Feb 2, 2023 at 11:03 AM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>
> On Thu, Feb 2, 2023 at 8:50 AM Sebastian Grzywna <swiftgeek@xxxxxxxxx> wrote:
> >
> > Dnia 2023-02-01, o godz. 19:34:48
> > "Rafael J. Wysocki" <rafael@xxxxxxxxxx> napisał(a):
> >
> > > On Wed, Feb 1, 2023 at 12:38 AM Pedro Falcato
> > > <pedro.falcato@xxxxxxxxx> wrote:
> > > >
> > > > Make custom_method keep its own per-file-open state instead of
> > > > global state in order to avoid race conditions[1] and other
> > > > possible conflicts with other concurrent users.
> > > >
> > > > Link:
> > > > https://lore.kernel.org/linux-acpi/20221227063335.61474-1-zh.nvgt@xxxxxxxxx/
> > > > # [1] Reported-by: Hang Zhang <zh.nvgt@xxxxxxxxx> Cc: Swift Geek
> > > > <swiftgeek@xxxxxxxxx> Signed-off-by: Pedro Falcato
> > > > <pedro.falcato@xxxxxxxxx> ---
> > > >  This patch addresses Hang's problems plus the ones raised by
> > > > Rafael in his review (see link above).
> > > > https://lore.kernel.org/lkml/2667007.mvXUDI8C0e@kreacher/ was
> > > > submitted but since there were still people that wanted this
> > > > feature, I took my time to write up a patch that should fix the
> > > > issues. Hopefully the linux-acpi maintainers have not decided to
> > > > remove custom_method just yet.
> > >
> > > Well, thanks for the patch, but yes, they have.  Sorry.
> >
> > Hi Rafael,
> > Can you please explain why you don't want to keep it, given there's a
> > patch?
>
> Because this interface was a bad idea to start with and its
> implementation is questionable at the design level.
>
> Granted, at the time it was introduced, there was no alternative, but
> there is the AML debugger in the kernel now and as far as debugging is
> concerned, it is actually more powerful than custom_metod AFAICS.  See
> Documentation/firmware-guide/acpi/aml-debugger.rst.
>
> If the AML debugger has problems, I would very much prefer fixing them
> to the perpetual maintenance of custom_method.
>
> > I find it really useful in my day-to-day as a firmware engineer.
> > I don't see much happening in git history of
> > drivers/acpi/custom_method.c , and I don't see anything that was
> > specifically changed in it in past 10 years to keep it being
> > functional. Without your more detailed explanation I have hard time
> > understanding your decision to remove it, since I'm not a kernel
> > developer myself.
>
> It's been always conceptually questionable, problematic from the
> security standpoint and implemented poorly.  Also its documentation is
> outdated.
>
> The patches fixing its most apparent functional issues don't actually
> address much of the above.
>
> The AML debugger should really be used for debug rather than
> custom_method and honestly, what's the purpose of it beyond debug?

The above said, if people really do care about custom_method, it can
be retained, but its documentation needs to be updated to cover the
current requirements (according to Rui, they have changed after some
upstream ACPICA changes).

Also note that the upstream ACPICA may not be guaranteed to avoid
breaking this interface in the future, as it depends on
acpi_install_method() that is provided by ACPICA specifically for the
use in the AML debugger.