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

[Chen Yu]

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
> 
>