Re: [PATCH 01/12] fpga: sec-mgr: fpga security manager class driver

[Russ Weight <russell.h.weight@intel.com>]

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

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