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