MIME-Version: 1.0
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>
 <CAJZ5v0hbFNGugDJ3PGLzfNm7h7f8vTesUOZ0R_vkYGaxBWFCdQ@mail.gmail.com> <CAKbZUD3UPMuH-NHXL093FaaFtEaeC295RemSeVMC8jLr6zbghA@mail.gmail.com>
In-Reply-To: <CAKbZUD3UPMuH-NHXL093FaaFtEaeC295RemSeVMC8jLr6zbghA@mail.gmail.com>
From:   Pedro Falcato <pedro.falcato@gmail.com>
Date:   Mon, 13 Mar 2023 23:45:30 +0000
Message-ID: <CAKbZUD0AmxKhMkaO1MwkV_0TAna=Vq2oapxG5QTCwrUeOhzXwQ@mail.gmail.com>
Subject: Re: [PATCH] ACPI: Make custom_method use per-open state
To:     "Rafael J. Wysocki" <rafael@kernel.org>
Cc:     "Marty E. Plummer" <hanetzer@startmail.com>,
        Sebastian Grzywna <swiftgeek@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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Feb 2, 2023 at 3:48=E2=80=AFPM Pedro Falcato <pedro.falcato@gmail.c=
om> wrote:
>
> On Thu, Feb 2, 2023 at 10:45 AM Rafael J. Wysocki <rafael@kernel.org> wro=
te:
> >
> > On Thu, Feb 2, 2023 at 11:03 AM Rafael J. Wysocki <rafael@kernel.org> w=
rote:
> > >
> > > 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):
> > > >
> > > > > On Wed, Feb 1, 2023 at 12:38 AM Pedro Falcato
> > > > > <pedro.falcato@gmail.com> 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.nv=
gt@gmail.com/
> > > > > > # [1] Reported-by: Hang Zhang <zh.nvgt@gmail.com> Cc: Swift Gee=
k
> > > > > > <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 t=
o
> > > > > > 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 i=
s
> > > concerned, it is actually more powerful than custom_metod AFAICS.  Se=
e
> > > Documentation/firmware-guide/acpi/aml-debugger.rst.
> > >
> > > If the AML debugger has problems, I would very much prefer fixing the=
m
> > > 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 i=
s
> > > 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.
>
> Just to be clear, I have no stake in this matter and wrote this patch bec=
ause
> someone complained on #coreboot about the removal. This patch was mostly
> trivial to write and if you decide there really is no value in keeping
> this, I'm a-OK.
>
> From a maintainers' PoV, if the AML debugger completely encompasses this
> from a functional PoV with a good UX, I would probably encourage you to
> remove this once and for all (what's the value in having 2 of the same
> functionality?).
>
> Hopefully the FW folks can give more context and feedback.
>
> --
> Pedro

Hi Rafael and other stakeholders,

Gentle ping. Are we axing the feature? Are we reviewing/merging this patch?

Having a straight up racy/broken feature in the kernel (even if only
under debugfs) is not ideal.

--=20
Pedro