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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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@intel.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.

> +static void dump_update_result(struct pfru_updated_result *result)
> +{
> +	pr_debug("Update result:\n");
> +	pr_debug("Status:%d\n", result->status);
> +	pr_debug("Extended Status:%d\n", result->ext_status);
> +	pr_debug("Authentication Time Low:%lld\n", result->low_auth_time);
> +	pr_debug("Authentication Time High:%lld\n", result->high_auth_time);
> +	pr_debug("Execution Time Low:%lld\n", result->low_exec_time);
> +	pr_debug("Execution Time High:%lld\n", result->high_exec_time);

Why not dev_dbg()?  Same for all pr_* calls in this "driver".


> +static long pfru_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> +	void __user *p;
> +	int ret = 0, rev;
> +
> +	p = (void __user *)arg;
> +
> +	switch (cmd) {
> +	case PFRU_IOC_SET_REV:
> +		if (copy_from_user(&rev, p, sizeof(unsigned int)))
> +			return -EFAULT;
> +		if (!pfru_valid_revid(rev))
> +			return -EFAULT;
> +		pfru_dev->rev_id = rev;
> +		break;
> +	case PFRU_IOC_STAGE:
> +		ret = start_acpi_update(START_STAGE);
> +		break;
> +	case PFRU_IOC_ACTIVATE:
> +		ret = start_acpi_update(START_ACTIVATE);
> +		break;
> +	case PFRU_IOC_STAGE_ACTIVATE:
> +		ret = start_acpi_update(START_STAGE_ACTIVATE);
> +		break;
> +	default:
> +		ret = -ENOIOCTLCMD;

Wrong value :(



> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +#ifdef CONFIG_COMPAT
> +static long compat_pfru_ioctl(struct file *filep, unsigned int cmd,
> +			      unsigned long arg)
> +{
> +	return pfru_ioctl(filep, cmd, arg);
> +}
> +#endif

Why is this compat ioctl needed at all?

> +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"?

thanks,

greg k-h