On 5/17/21 11:25 AM, Russ Weight wrote:
On 5/17/21 10:55 AM, Greg KH wrote:
On Mon, May 17, 2021 at 10:45:40AM -0700, Russ Weight wrote:
Hi Greg,
On 5/16/21 10:18 PM, Greg KH wrote:
On Sun, May 16, 2021 at 07:31:49PM -0700, Moritz Fischer wrote:
From: Russ Weight <russell.h.weight@xxxxxxxxx>
Create the FPGA Security Manager class driver. The security
manager provides interfaces to manage secure updates for the
FPGA and BMC images that are stored in FLASH. The driver can
also be used to update root entry hashes and to cancel code
signing keys. The image type is encoded in the image file
and is decoded by the HW/FW secure update engine.
Signed-off-by: Russ Weight <russell.h.weight@xxxxxxxxx>
Russ, you know the Intel rules here, why did you not get someone who has
knowledge of the kernel's driver model to review your patches before
sending them out?
Basic driver model review comments below, I'm stopping after reviewing
this one as there's some big failures here...
+++ b/drivers/fpga/fpga-sec-mgr.c
@@ -0,0 +1,296 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * FPGA Security Manager
+ *
+ * Copyright (C) 2019-2020 Intel Corporation, Inc.
What year is it? :(
Thanks - I'll fix the copyright dates.
+ */
+
+#include <linux/fpga/fpga-sec-mgr.h>
+#include <linux/idr.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/vmalloc.h>
+
+static DEFINE_IDA(fpga_sec_mgr_ida);
+static struct class *fpga_sec_mgr_class;
+
+struct fpga_sec_mgr_devres {
+ struct fpga_sec_mgr *smgr;
+};
+
+#define to_sec_mgr(d) container_of(d, struct fpga_sec_mgr, dev)
+
+static ssize_t name_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct fpga_sec_mgr *smgr = to_sec_mgr(dev);
+
+ return sysfs_emit(buf, "%s\n", smgr->name);
+}
+static DEVICE_ATTR_RO(name);
What is wrong with the name of the device? Please just use that and do
not have a "second name" of the thing.
The purpose was to display the name of the parent driver. Should I change
"name" to "parent"? Or drop this altogether?
How is "name" a "parent"? To find the parent, just walk up the sysfs
tree.
Please note that in this and other cases, I have been conforming to
conventions already used in FPGA Manager class driver:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/fpga/fpga-mgr.c#n397
Maybe that needs to be fixed as well :)
But, why re-implement the same thing and not just use the existing class
framework and code?
I did the exercise of trying to merge the new functionality into the
fpga-mgr.c code, but there was so little commonality that it was beginning
to look like a dual-personality driver. The only thing that could be shared
was the registration/unregistration of the driver. It seemed cleaner to
have it as a separate class driver.
- Russ
I'll post a patch in a bit that does nothing new but refactor fpga-mgr's
ops into 'partial update' and 'full update'
existing stuff in partial
security update stuff in full
Tom
+EXPORT_SYMBOL_GPL(fpga_sec_mgr_create);
Why did you not register the device here.
My original implementation created and registered the device in a single function:
https://marc.info/?l=linux-fpga&m=159926365226264&w=2
It was split up to conform to the conventions used by other class drivers in the FPGA
framework: fpga-mgr.c, fpga-bridge.c, fpga-region.c
If you don't need things to be split, don't split it. Or better yet,
use the existing code.
There used to be some lovely documentation in the kernel that said I was
allowed to yell at anyone who did something like this. But that's
removed, so I'll just be quiet and ask you to think about why you would
ever want to provide an empty function, just to make the kernel core "be
quiet". Did you perhaps think you were smarter than the kobject core
and this was the proper solution to make it "shut up" with it's crazy
warning that some over-eager developer added? Or perhaps, that warning
was there on purpose, lovingly hand-added to help provide a HUGE HINT
that not providing a REAL release function was wrong.
In my original submission, this function was populated.
https://marc.info/?l=linux-fpga&m=159926365226264&w=2
Again, I was conforming to conventions used in the other class drivers in
the FPGA framework, all of which have an empty *_dev_release()
function:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/fpga/fpga-mgr.c#n782
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/fpga/fpga-bridge.c#n476
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/fpga/fpga-region.c#n317
Oh wow, that's totally wrong and broken, thanks for pointing it out.
Please fix that up first.
thanks,
greg k-h
