All of lore.kernel.org
 help / color / mirror / Atom feed
From: Greg KH <gregkh@linuxfoundation.org>
To: Russ Weight <russell.h.weight@intel.com>
Cc: Tom Rix <trix@redhat.com>, Xu Yilun <yilun.xu@intel.com>,
	Moritz Fischer <mdf@kernel.org>,
	linux-fpga@vger.kernel.org, moritzf@google.com
Subject: Re: [PATCH 01/12] fpga: sec-mgr: fpga security manager class driver
Date: Fri, 21 May 2021 06:58:40 +0200	[thread overview]
Message-ID: <YKc+ADoYScqJAqI6@kroah.com> (raw)
In-Reply-To: <af35f2cb-4f69-77e5-7add-d7f337a9dac7@intel.com>

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

  reply	other threads:[~2021-05-21  4:58 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-17  2:31 [PATCH 00/12] FPGA Security Manager for 5.14 Moritz Fischer
2021-05-17  2:31 ` [PATCH 01/12] fpga: sec-mgr: fpga security manager class driver Moritz Fischer
2021-05-17  5:18   ` Greg KH
2021-05-17 17:45     ` Russ Weight
2021-05-17 17:55       ` Greg KH
2021-05-17 18:25         ` Russ Weight
2021-05-19 20:42           ` Tom Rix
2021-05-21  1:10             ` Russ Weight
2021-05-21  4:58               ` Greg KH [this message]
2021-05-21 15:15                 ` Russ Weight
2021-05-17  2:31 ` [PATCH 02/12] fpga: sec-mgr: enable secure updates Moritz Fischer
2021-05-17  5:32   ` Greg KH
2021-05-17 19:37     ` Russ Weight
2021-07-30  1:23       ` Russ Weight
2021-07-30 11:18         ` Greg KH
2021-08-02 18:31           ` Russ Weight
2021-08-03  5:49             ` Greg KH
2021-08-03 19:02               ` Russ Weight
2021-08-04  7:37                 ` Greg KH
2021-08-04 14:58                   ` Moritz Fischer
2021-08-04 15:12                     ` Greg KH
2021-08-04 19:47                       ` Moritz Fischer
2021-11-02 16:25                       ` Russ Weight
2021-11-02 17:06                         ` Greg KH
2021-05-17  2:31 ` [PATCH 03/12] fpga: sec-mgr: expose sec-mgr update status Moritz Fischer
2021-05-17  2:31 ` [PATCH 04/12] fpga: sec-mgr: expose sec-mgr update errors Moritz Fischer
2021-05-17  2:31 ` [PATCH 05/12] fpga: sec-mgr: expose sec-mgr update size Moritz Fischer
2021-05-17  2:31 ` [PATCH 06/12] fpga: sec-mgr: enable cancel of secure update Moritz Fischer
2021-05-17  2:31 ` [PATCH 07/12] fpga: sec-mgr: expose hardware error info Moritz Fischer
2021-05-17  7:10   ` Greg KH
2021-05-17 19:49     ` Russ Weight
2021-05-17  2:31 ` [PATCH 08/12] fpga: m10bmc-sec: create max10 bmc secure update driver Moritz Fischer
2021-05-17  5:30   ` Greg KH
2021-05-17 20:09     ` Russ Weight
2021-05-17  2:31 ` [PATCH 09/12] fpga: m10bmc-sec: expose max10 flash update count Moritz Fischer
2021-05-17  2:31 ` [PATCH 10/12] fpga: m10bmc-sec: expose max10 canceled keys in sysfs Moritz Fischer
2021-05-17  2:31 ` [PATCH 11/12] fpga: m10bmc-sec: add max10 secure update functions Moritz Fischer
2021-05-17  2:32 ` [PATCH 12/12] fpga: m10bmc-sec: add max10 get_hw_errinfo callback func Moritz Fischer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YKc+ADoYScqJAqI6@kroah.com \
    --to=gregkh@linuxfoundation.org \
    --cc=linux-fpga@vger.kernel.org \
    --cc=mdf@kernel.org \
    --cc=moritzf@google.com \
    --cc=russell.h.weight@intel.com \
    --cc=trix@redhat.com \
    --cc=yilun.xu@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.