Date:   Fri, 3 Feb 2023 07:42:27 +0100
From:   Sebastian Grzywna <swiftgeek@gmail.com>
To:     "Rafael J. Wysocki" <rafael@kernel.org>
Cc:     Pedro Falcato <pedro.falcato@gmail.com>,
        linux-acpi@vger.kernel.org, Len Brown <lenb@kernel.org>,
        linux-kernel@vger.kernel.org, rui.zhang@intel.com,
        Hang Zhang <zh.nvgt@gmail.com>
Subject: Re: [PATCH] ACPI: Make custom_method use per-open state
Message-ID: <20230203074227.47c3e363@gmail.com>
In-Reply-To: <CAJZ5v0iwO=xJ8A=vv4Khm6Z+Lb9hpZsZmyCjMeSHutMWRcp78g@mail.gmail.com>
References: <20230131233755.58942-1-pedro.falcato@gmail.com>
        <CAJZ5v0iXcRFamA+mE837=zHReBT-+8WmMeRDR7L9R+FVpLr25A@mail.gmail.com>
        <20230202084953.3035c6e3@gmail.com>
        <CAJZ5v0iwO=xJ8A=vv4Khm6Z+Lb9hpZsZmyCjMeSHutMWRcp78g@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, 2 Feb 2023 11:03:47 +0100
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> On Thu, Feb 2, 2023 at 8:50 AM Sebastian Grzywna
> <swiftgeek@gmail.com> wrote:
> >
> > Dnia 2023-02-01, o godz. 19:34:48
> > "Rafael J. Wysocki" <rafael@kernel.org> napisa=C5=82(a):
> > =20
> > > On Wed, Feb 1, 2023 at 12:38 AM Pedro Falcato
> > > <pedro.falcato@gmail.com> wrote: =20
> > > >
> > > > 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@g=
mail.com/
> > > > # [1] Reported-by: Hang Zhang <zh.nvgt@gmail.com> Cc: Swift Geek
> > > > <swiftgeek@gmail.com> Signed-off-by: Pedro Falcato
> > > > <pedro.falcato@gmail.com> ---
> > > >  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. =20
> > >
> > > Well, thanks for the patch, but yes, they have.  Sorry. =20
> >
> > Hi Rafael,
> > Can you please explain why you don't want to keep it, given there's
> > a patch? =20
>=20
> Because this interface was a bad idea to start with and its
> implementation is questionable at the design level.
>=20
> 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.
>=20
> If the AML debugger has problems, I would very much prefer fixing them
> to the perpetual maintenance of custom_method.
>=20
> > 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. =20
>=20
> It's been always conceptually questionable, problematic from the
> security standpoint and implemented poorly.  Also its documentation is
> outdated.
>=20
> The patches fixing its most apparent functional issues don't actually
> address much of the above.
>=20
> The AML debugger should really be used for debug rather than
> custom_method and honestly, what's the purpose of it beyond debug?

I tried to investigate how to get my workflow going with
tools/power/acpi/acpidbg , and after looking at
drivers/acpi/acpica/dbinput.c I am a bit confused as to how should I
enable CMD_LOAD properly, and if it's not in Kconfig then how
supported/tested is this feature.

acpica-reference_19.pdf is also confusing me a bit with mention of
"Unloading the DSDT is not allowed", making me worry that live
reloading of DSDT would not be possible (which would be superset of
overriding just a single method).

I want to be able to swap a particular method, and have it run as close
to normally as possible, sometimes for prolonged period of time, but
most of the time I will fail at writing a patch and will need to swap
it again with another attempted fix.