linux-kernel - Re: [PATCH v3 3/5] drivers/acpi: Introduce Platform Firmware Runtime Update device driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YUtnuW22MH97p7+G@kroah.com>
Date:   Wed, 22 Sep 2021 19:28:25 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Chen Yu <yu.c.chen@...el.com>
Cc:     linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Len Brown <len.brown@...el.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Andy Shevchenko <andriy.shevchenko@...el.com>,
        Aubrey Li <aubrey.li@...el.com>,
        Ashok Raj <ashok.raj@...el.com>,
        Mike Rapoport <rppt@...nel.org>,
        Ard Biesheuvel <ardb@...nel.org>,
        Jonathan Corbet <corbet@....net>,
        Hans de Goede <hdegoede@...hat.com>,
        Ben Widawsky <ben.widawsky@...el.com>,
        linux-doc@...r.kernel.org
Subject: Re: [PATCH v3 3/5] drivers/acpi: Introduce Platform Firmware Runtime
 Update device driver

On Thu, Sep 23, 2021 at 12:33:21AM +0800, Chen Yu wrote:
> On Wed, Sep 22, 2021 at 11:10:02AM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Sep 22, 2021 at 05:04:42PM +0800, Chen Yu wrote:
> > > Hi Greg,
> > > On Tue, Sep 21, 2021 at 05:59:05PM +0200, Greg Kroah-Hartman wrote:
> > > > On Fri, Sep 17, 2021 at 12:02:18AM +0800, Chen Yu wrote:
> > > > > Introduce the pfru_update driver which can be used for Platform Firmware
> > > > > Runtime code injection and driver update. 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.
> > > > > 
> > > > > Signed-off-by: Chen Yu <yu.c.chen@...el.com>
> > > > 
> > > > Where is the userspace code that uses this ioctl and has tested it out
> > > > to verify it works properly?  A link to that in the changelog would be
> > > > great to have.
> > > > 
> > > The patch [5/5] is a self testing tool to test the whole feature. I'll send a
> > > new version and Cc you too.
> > 
> > That tests it, but does not answer the question of who will actually use
> > this.  What userspace tool needs this new api?
> >
> One end user is the cloud user.

What exactly do you mean by "cloud user"?

> Currently there is no dedicated userspace tool developed to use this
> feature AFAIK.

Wonderful, then it is not needed to be added to the kernel :)

> It was expected that the end users
> could refer to the self test tool to customize their tools. I'm not sure if
> this is the proper way to propose the feature, may I have your suggestion on
> this, should I create a separate git repository for this tool, or put it in
> tools/selftestings as it is now?

No, do not add this to the kernel unless you have a real need and user
for this.


> > > > > +static struct miscdevice pfru_misc_dev = {
> > > > > +	.minor = MISC_DYNAMIC_MINOR,
> > > > > +	.name = "pfru_update",
> > > > > +	.nodename = "pfru/update",
> > > > 
> > > > Why is this in a subdirectory?  What requires this?  Why not just
> > > > "pfru"?
> > > > 
> > > The pfru directory might be reused for pfru_telemetry device, whose driver
> > > is in 4/5 patch, I'll Cc you with the whole patch set in next version.
> > 
> > "might be" is not a valid reason.  Why does this simple driver deserve a
> > whole /dev/ subdirectory?
> > 
> There are pfru_update and pfru_telemetry in the patch, and there is plan to
> add a pfru_prm device in the future, which stands for "Platform Runtime Mechanism".
> I'll move them to /dev/ in next version.

That is a very generic name for a very platform specific and arch
specific interface.  As this is an ACPI interface, why not use that name
prefix?

thanks,

greg k-h