linux-kernel - Re: [PATCH v7 3/4] drivers/acpi: Introduce Platform Firmware Runtime Update Telemetry

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: <20211101101641.GA20219@chenyu5-mobl1>
Date:   Mon, 1 Nov 2021 18:16:41 +0800
From:   Chen Yu <yu.c.chen@...el.com>
To:     Andy Shevchenko <andriy.shevchenko@...el.com>
Cc:     linux-acpi@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Ard Biesheuvel <ardb@...nel.org>, Len Brown <lenb@...nel.org>,
        Ashok Raj <ashok.raj@...el.com>,
        Mike Rapoport <rppt@...nel.org>,
        Aubrey Li <aubrey.li@...el.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 3/4] drivers/acpi: Introduce Platform Firmware Runtime
 Update Telemetry

On Wed, Oct 27, 2021 at 01:45:00PM +0300, Andy Shevchenko wrote:
> On Wed, Oct 27, 2021 at 03:08:05PM +0800, Chen Yu wrote:
> > Platform Firmware Runtime Update(PFRU) Telemetry Service is part of RoT
> > (Root of Trust), which allows PFRU handler and other PFRU drivers to
> > produce telemetry data to upper layer OS consumer at runtime.
> > 
> > The linux provides interfaces for the user to query the parameters of
> 
> Linux kernel
>
Ok. 
> > telemetry data, and the user could read out the telemetry data
> > accordingly.
> > 
> > The corresponding userspace tool and man page will be introduced at
> > tools/power/acpi/pfru.
> 
> ...
> 
> > +#include <linux/acpi.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/errno.h>
> > +#include <linux/file.h>
> > +#include <linux/fs.h>
> > +#include <linux/miscdevice.h>
> > +#include <linux/module.h>
> > +#include <linux/mm.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/string.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/uio.h>
> > +#include <linux/uuid.h>
> 
> + blank line?
> 
Ok.
> > +#include <uapi/linux/pfru.h>
> 
> ...
> 
> > +static DEFINE_IDA(pfru_log_ida);
> 
> Do you need any mutex against operations on IDA? (I don't remember
> if it incorporates any synchronization primitives).
> 
The IDA uses a spinlock_irqsave() to protect the bitmap.
> ...
> 
> Looking into the code I have feelings of déjà-vu. Has it really had
> nothing in common with the previous patch?
> 
They both invokes _DSM to trigger the low level actions. However the input
parameters and return ACPI package as well as the functions are different
and hard to extract the common code between them.
> ...
> 
> > +static int valid_log_level(int level)
> > +{
> > +	return level == LOG_ERR || level == LOG_WARN ||
> > +		level == LOG_INFO || level == LOG_VERB;
> 
> Indentation.
> 
Ok, will add.
> > +}
> 
> ...
> 
> 
> This ordering in ->probe() is not okay:
> 	devm_*()
> 	non-devm_*()
> 	devm_*()
> 	non-devm_*()
> 
> One mustn't interleave these. The allowed are:
> 
> Case 1:
> 	non-devm_*()
> 
> Case 2:
> 	devm_*()
> 
> Case 3:
> 	devm_*()
> 	non-devm_*()
> 
> Otherwise in ->remove() you have wrong release ordering which may hide
> subtle bugs.
> 
Got it. I'll fix it in next version.
> Above comment is applicable to the other patch as well as some comments
> from there are applicable here.
> 
Ok.

Thanks,
Chenyu
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>