From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-20.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id BFDCCC433ED for ; Mon, 17 May 2021 18:25:53 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 9AD5261350 for ; Mon, 17 May 2021 18:25:53 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233749AbhEQS1J (ORCPT ); Mon, 17 May 2021 14:27:09 -0400 Received: from mga06.intel.com ([134.134.136.31]:49139 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233592AbhEQS1J (ORCPT ); Mon, 17 May 2021 14:27:09 -0400 IronPort-SDR: evh7ngWzKox5AtPMDfhroOF4MLdSlIVeGxcV3W9BaHc7OjjAbW8emARfOP458kVPtnW92h2WV2 VqjRIVABMT2A== X-IronPort-AV: E=McAfee;i="6200,9189,9987"; a="261766326" X-IronPort-AV: E=Sophos;i="5.82,307,1613462400"; d="scan'208";a="261766326" Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 May 2021 11:25:52 -0700 IronPort-SDR: IzcdCDipCTwPS4N41r5ij2PLkJkochm/BYHVOhxxb5EHSvC+oz9RXnuzDnKuOhASURLnDI46cU Pb2MiWJpKOHg== X-IronPort-AV: E=Sophos;i="5.82,307,1613462400"; d="scan'208";a="543798192" Received: from rhweight-mobl2.amr.corp.intel.com (HELO [10.0.2.4]) ([10.212.243.163]) by orsmga004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 May 2021 11:25:51 -0700 Subject: Re: [PATCH 01/12] fpga: sec-mgr: fpga security manager class driver To: Greg KH Cc: Xu Yilun , Tom Rix , Moritz Fischer , linux-fpga@vger.kernel.org, moritzf@google.com References: <20210517023200.52707-1-mdf@kernel.org> <20210517023200.52707-2-mdf@kernel.org> <0c54779e-4ac6-e816-e290-f613cfe1fff3@intel.com> From: Russ Weight Message-ID: Date: Mon, 17 May 2021 11:25:48 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-Language: en-US Precedence: bulk List-ID: X-Mailing-List: linux-fpga@vger.kernel.org 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 >>>> >>>> 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 >>> 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 >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> + >>>> +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