On Sat, Oct 16, 2021 at 05:16:17PM +0200, Greg Kroah-Hartman 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 > > [snip...] > > 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? > Ok, will move these comments into kernel doc in pfru.h. > Or are other acpi ioctl interfaces documented here already? > No other acpi ioctl interfaces, but there are some non-acpi ioctl interfaces, such as rtc-cdev. > > diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst > > index 2e8134059c87..6e5a82fff408 100644 > > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst > > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst > > @@ -365,6 +365,7 @@ Code Seq# Include File Comments > > <mailto:aherrman@xxxxxxxxxx> > > 0xE5 00-3F linux/fuse.h > > 0xEC 00-01 drivers/platform/chrome/cros_ec_dev.h ChromeOS EC driver > > +0xEE 00-1F uapi/linux/pfru.h Platform Firmware Runtime Update and Telemetry > > You are not using all of those values, right? > Not using all of them, will shrink the range to 8 in next version. > > 0xF3 00-3F drivers/usb/misc/sisusbvga/sisusb.h sisfb (in development) > > [snip...] <mailto:thomas@xxxxxxxxxxxxxxxx> > > + > > +struct pfru_device { > > + guid_t uuid, code_uuid, drv_uuid; > > + int rev_id; > > + struct device *dev; > > +}; > > + > > +static struct pfru_device *pfru_dev; > > Why is this a single variable? Shouldn't this be per-device as the bus > provides it to you? > [snip...] > > + > > +static int acpi_pfru_probe(struct platform_device *pdev) > > +{ > > + acpi_handle handle; > > + int ret; > > + > > + /* Only one instance is allowed. */ > > + if (pfru_dev) > > + return 0; > > Why is only one instance allowed? Why add extra work to do this when it > really is not needed at all? It is simpler and less code to make it so > that there is no restriction like this at all. > > Also, the return value is incorrect, so your implementaion of trying to > keep only one instance does not work properly :( > Ok, I'll change it to per-device in next version. And the motivation of using a single variable was that: There would be only one instance of PFRU ACPI object and one PFRU Telemetry ACPI object provided by BIOS, otherwise it is regarded as a BIOS bug for now. But since per-device variable is more acceptable and scalable, will change it to per-device in next version. [snip...] > > +}; > > + [snip...] > > +static int __init pfru_init(void) > > +{ > > + int ret; > > + > > + ret = misc_register(&pfru_misc_dev); > > + if (ret) > > + return ret; > > + > > Why register this here, BEFORE you have a real device? That looks like > a big race condition here :( > > Register it per device you have in the system please. > Ok. Previously the pfru_misc_dev is shared between the PFRU device and PFRU Telemetry device, so that the PFRU device is accessed via pfru_misc_dev.write() and PFRU device is accessed via pfru_misc_dev.read(). The benefit of doing this is that, the user only deals with one misc_dev node rather than two. Changing this to per-device scope would generate two misc_dev nodes, and the user needs to deal with them respectively, but with better scalability and less race condition. I'll revise it in next version. Thanks, Chenyu > thanks, > > greg k-h
