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

[Tom Rix <trix@redhat.com>]

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

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