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

[Rafael J. Wysocki]

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?