Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <cover.1634310710.git.yu.c.chen@intel.com> <eadc25933400c71a52e7d176880df09a147a39b9.1634310710.git.yu.c.chen@intel.com>
 <YWrswQ+ETMthZZVk@kroah.com>
In-Reply-To: <YWrswQ+ETMthZZVk@kroah.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Tue, 19 Oct 2021 18:33:08 +0200
Message-ID: <CAJZ5v0gvLDu8xGUVR0kATko8M37RiYg63t+1BWFox499QJxSpg@mail.gmail.com>
Subject: Re: [PATCH v4 2/4] drivers/acpi: Introduce Platform Firmware Runtime
 Update device driver
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     Chen Yu <yu.c.chen@intel.com>,
        ACPI Devel Maling List <linux-acpi@vger.kernel.org>,
        Ard Biesheuvel <ardb@kernel.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Len Brown <lenb@kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Ashok Raj <ashok.raj@intel.com>,
        Andy Shevchenko <andriy.shevchenko@intel.com>,
        Mike Rapoport <rppt@kernel.org>,
        Aubrey Li <aubrey.li@intel.com>,
        Jonathan Corbet <corbet@lwn.net>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, Oct 16, 2021 at 5:16 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Sat, Oct 16, 2021 at 06:40:51PM +0800, Chen Yu wrote:
> > Introduce the pfru_update driver which can be used for Platform Firmware
> > Runtime code injection and driver update[1]. The user is expected to
> > provide the update firmware in the form of capsule file, and pass it to
> > the driver via ioctl. Then the driver would hand this capsule file to the
> > Platform Firmware Runtime Update via the ACPI device _DSM method. At last
> > the low level Management Mode would do the firmware update.
> >
> > [1] https://uefi.org/sites/default/files/resources/Intel_MM_OS_Interface_Spec_Rev100.pdf
> >
> > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > ---
> > v4: Add Documentation/ABI/testing/pfru (Rafael J. Wysocki)
> >     Change all pr_debug() to dev_dbg() (Greg Kroah-Hartman,
> >     Rafael J. Wysocki)
> >     Change the error code ENOIOCTLCMD to ENOTTY in ioctl.
> >     (Greg Kroah-Hartman)
> >     Remove compat ioctl. (Greg Kroah-Hartman)
> >     Change /dev/pfru/update to /dev/acpi_pfru (Greg Kroah-Hartman)
> >     Remove valid_cap_type() and do sanity check in query_capability().
> >     (Rafael J. Wysocki)
> >     Remove the loop in query_capability().
> >     (Rafael J. Wysocki)
> >     Do not fail if the package has more elements than expected,
> >     and return error if the number of package elements is too
> >     small. (Rafael J. Wysocki)
> >     Return the type or a negative error code in get_image_type()
> >     (Rafael J. Wysocki)
> >     Put the comment inside the function rather than outside.
> >     (Rafael J. Wysocki)
> >     Return the size or a negative error code adjust_efi_size()
> >     (Rafael J. Wysocki)
> >     Return -EINVAL rather than -EFAULT if revison id is incorrect.
> >     (Rafael J. Wysocki)
> >     Move the an read() of pfru into ioctl(), and using read() for
> >     the telemetry retrieval. So as to avoid the telemetry device
> >     file, the write() will be the code injection/update, the read()
> >     will be telemetry retrieval and all of the rest can be ioctl()s
> >     under one special device file.
> >     (Rafael J. Wysocki)
> > v3: Use __u32 instead of int and __64 instead of unsigned long
> >     in include/uapi/linux/pfru.h (Greg Kroah-Hartman)
> >     Rename the structure in uapi to start with a prefix pfru so as
> >     to avoid confusing in the global namespace. (Greg Kroah-Hartman)
> > v2: Add sanity check for duplicated instance of ACPI device.
> >     Update the driver to work with allocated pfru_device objects.
> >     (Mike Rapoport)
> >     For each switch case pair, get rid of the magic case numbers
> >     and add a default clause with the error handling.
> >     (Mike Rapoport)
> >     Move the obj->type checks outside the switch to reduce redundancy.
> >     (Mike Rapoport)
> >     Parse the code_inj_id and drv_update_id at driver initialization time
> >     to reduce the re-parsing at runtime.(Mike Rapoport)
> >     Explain in detail how the size needs to be adjusted when doing
> >     version check.(Mike Rapoport)
> >     Rename parse_update_result() to dump_update_result()(Mike Rapoport)
> >     Remove redundant return.(Mike Rapoport)
> >     Do not expose struct capsulate_buf_info to uapi, since it is
> >     not needed in userspace.(Mike Rapoport)
> > ---
> >  Documentation/ABI/testing/pfru                |  41 ++
> >  .../userspace-api/ioctl/ioctl-number.rst      |   1 +
> >  drivers/acpi/Kconfig                          |   1 +
> >  drivers/acpi/Makefile                         |   1 +
> >  drivers/acpi/pfru/Kconfig                     |  16 +
> >  drivers/acpi/pfru/Makefile                    |   2 +
> >  drivers/acpi/pfru/pfru_update.c               | 567 ++++++++++++++++++
> >  include/uapi/linux/pfru.h                     | 102 ++++
> >  8 files changed, 731 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/pfru
> >  create mode 100644 drivers/acpi/pfru/Kconfig
> >  create mode 100644 drivers/acpi/pfru/Makefile
> >  create mode 100644 drivers/acpi/pfru/pfru_update.c
> >  create mode 100644 include/uapi/linux/pfru.h
> >
> > diff --git a/Documentation/ABI/testing/pfru b/Documentation/ABI/testing/pfru
> > new file mode 100644
> > index 000000000000..b8bc81703f46
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/pfru
> > @@ -0,0 +1,41 @@
> > +What:                /dev/acpi_pfru
> > +Date:                October 2021
> > +KernelVersion:       5.15
> > +Contact:     Chen Yu <yu.c.chen@intel.com>
> > +Description:
> > +             The ioctl interface to drivers for platform firmware runtime
> > +             update(PFRU). Following actions are supported:
> > +
> > +             * PFRU_IOC_QUERY_CAP: Read the PFRU Runtime Update capability.
> > +               The value is a structure of pfru_update_cap_info.
> > +               See include/uapi/linux/pfru.h for definition.
> > +
> > +             * PFRU_SET_REV: Set the Revision ID for PFRU Runtime Update.
> > +               It could be either 1 or 2.
> > +
> > +             * PFRU_IOC_STAGE: Stage a capsule image from communication
> > +               buffer and perform authentication.
> > +
> > +             * PFRU_IOC_ACTIVATE: Activate a previous staged capsule image.
> > +
> > +             * PFRU_IOC_STAGE_ACTIVATE: Perform both stage and activation
> > +               actions.
> > +
> > +             * PFRU_LOG_IOC_SET_INFO: set log information in Telemetry
> > +               Service. The input is a structure of pfru_log_info.
> > +               This structure includes log revision id(1 or 2),
> > +               log level(0 : Error Message, 1 : Warning Message,
> > +               2 : Informational Message, 4 : Verbose), log data type
> > +               (0 : Execution Log, 1 : History Information).
> > +               See include/uapi/linux/pfru.h for definition.
> > +
> > +             * PFRU_LOG_IOC_GET_INFO: get log information in Telemetry.
> > +               The output is a structure of pfru_log_info.
> > +
> > +             * PFRU_LOG_IOC_GET_DATA_INFO: get log data information in
> > +               Telemetry. The output is a structure of pfru_log_data_info.
> > +               See include/uapi/linux/pfru.h for definition.
> > +
> > +             Besides ioctl interface, write() and read() are supported on
> > +             /dev/acpi_pfru. The write() will be the code injection/update,
> > +             and the read() will be telemetry retrieval.
>
> Do we normally describe ioctl interfaces in Documentation/ABI/?  Why not
> just add this to the kernel doc with the structures you are creating?
> Wouldn't that be easier?

It would work I suppose.

> Or are other acpi ioctl interfaces documented here already?

No, they aren't, but there is Documentation/ABI/testing/gpio-cdev, for
example, so there is some practice there.  Whether it is good or bad
is a separate topic though.