On Thu, May 20, 2021 at 06:10:17PM -0700, Russ Weight wrote:
>
> On 5/19/21 1:42 PM, Tom Rix wrote:
> >
> > 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
>
> FYI: I just posted patches that remove the managed resource functions and
> populate the class dev_release functions for fpga_mgr.c, fpga_region.c,
> and fpga_bridge.c.
>
> https://marc.info/?l=linux-fpga&m=162155904229400&w=2
> https://marc.info/?l=linux-fpga&m=162155904329404&w=2
> https://marc.info/?l=linux-fpga&m=162155904529409&w=2
> https://marc.info/?l=linux-fpga&m=162155904529412&w=2
You forgot to cc: me :(
I guess you didn't want me to point out the fact that you forgot to go
through the proper internal Intel patch review process first, before
asking others to review your changes?
{sigh}
greg k-h
