Date:   Fri, 10 Sep 2021 13:47:43 -0700
From:   Russ Weight <russell.h.weight@...el.com>
To:     Xu Yilun <yilun.xu@...el.com>
CC:     <mdf@...nel.org>, <linux-fpga@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <trix@...hat.com>,
        <lgoncalv@...hat.com>, <hao.wu@...el.com>,
        <matthew.gerlach@...el.com>
Subject: Re: [PATCH v15 1/6] fpga: image-load: fpga image load class driver



On 9/9/21 11:46 PM, Xu Yilun wrote:
> On Wed, Sep 08, 2021 at 07:18:41PM -0700, Russ Weight wrote:
>> The FPGA Image Load class driver provides an API to transfer update
>> files to an FPGA device. Image files are self-describing. They could
>> contain FPGA images, BMC images, Root Entry Hashes, or other device
>> specific files. It is up to the device driver and the target device
>> to authenticate and disposition the file data.
>>
>> Signed-off-by: Russ Weight <russell.h.weight@...el.com>
>> ---
>> v15:
>>  - Compare to previous patch:
>>      [PATCH v14 1/6] fpga: sec-mgr: fpga security manager class driver 
>>  - Changed file, symbol, and config names to reflect the new driver name
>>  - Rewrote documentation. The documentation will be added to in later patches.
>>  - Removed signed-off/reviewed-by tags
>> v14:
>>  - Updated copyright to 2021
>>  - Removed the name sysfs entry
>>  - Removed MAINTAINERS reference to
>>    Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
>>  - Use xa_alloc() instead of ida_simple_get()
>>  - Rename dev to parent for parent devices
>>  - Remove fpga_sec_mgr_create(), devm_fpga_sec_mgr_create(), and
>>    fpga_sec_mgr_free() functions and update the fpga_sec_mgr_register()
>>    function to both create and register a new security manager.
>>  - Populate the fpga_sec_mgr_dev_release() function.
>> v13:
>>   - No change
>> v12:
>>   - Updated Date and KernelVersion fields in ABI documentation
>> v11:
>>   - No change
>> v10:
>>   - Rebased to 5.12-rc2 next
>>   - Updated Date and KernelVersion in ABI documentation
>> v9:
>>   - Updated Date and KernelVersion in ABI documentation
>> v8:
>>   - Fixed grammatical error in Documentation/fpga/fpga-sec-mgr.rst
>> v7:
>>   - Changed Date in documentation file to December 2020
>> v6:
>>   - Removed sysfs support and documentation for the display of the
>>     flash count, root entry hashes, and code-signing-key cancelation
>>     vectors.
>> v5:
>>   - Added the devm_fpga_sec_mgr_unregister() function, following recent
>>     changes to the fpga_manager() implementation.
>>   - Changed some *_show() functions to use sysfs_emit() instead of sprintf(
>> v4:
>>   - Changed from "Intel FPGA Security Manager" to FPGA Security Manager"
>>     and removed unnecessary references to "Intel".
>>   - Changed: iops -> sops, imgr -> smgr, IFPGA_ -> FPGA_, ifpga_ to fpga_
>> v3:
>>   - Modified sysfs handler check in check_sysfs_handler() to make
>>     it more readable.
>> v2:
>>   - Bumped documentation dates and versions
>>   - Added Documentation/fpga/ifpga-sec-mgr.rst
>>   - Removed references to bmc_flash_count & smbus_flash_count (not supported)
>>   - Split ifpga_sec_mgr_register() into create() and register() functions
>>   - Added devm_ifpga_sec_mgr_create()
>>   - Removed typedefs for imgr ops
>> ---
>> ---
>>  Documentation/fpga/fpga-image-load.rst |  10 ++
>>  Documentation/fpga/index.rst           |   1 +
>>  MAINTAINERS                            |   8 ++
>>  drivers/fpga/Kconfig                   |  10 ++
>>  drivers/fpga/Makefile                  |   3 +
>>  drivers/fpga/fpga-image-load.c         | 124 +++++++++++++++++++++++++
>>  include/linux/fpga/fpga-image-load.h   |  35 +++++++
>>  7 files changed, 191 insertions(+)
>>  create mode 100644 Documentation/fpga/fpga-image-load.rst
>>  create mode 100644 drivers/fpga/fpga-image-load.c
>>  create mode 100644 include/linux/fpga/fpga-image-load.h
>>
>> diff --git a/Documentation/fpga/fpga-image-load.rst b/Documentation/fpga/fpga-image-load.rst
>> new file mode 100644
>> index 000000000000..a6e53ac66026
>> --- /dev/null
>> +++ b/Documentation/fpga/fpga-image-load.rst
>> @@ -0,0 +1,10 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +============================
>> +FPGA Image Load Class Driver
>> +============================
>> +
>> +The FPGA Image Load class driver provides a common API for user-space
>> +tools to manage image uploads to FPGA devices. Device drivers that
>> +instantiate the FPGA Image Load class driver will interact with the
>> +target device to transfer and authenticate the image data.
>> diff --git a/Documentation/fpga/index.rst b/Documentation/fpga/index.rst
>> index f80f95667ca2..85d25fb22c08 100644
>> --- a/Documentation/fpga/index.rst
>> +++ b/Documentation/fpga/index.rst
>> @@ -8,6 +8,7 @@ fpga
>>      :maxdepth: 1
>>  
>>      dfl
>> +    fpga-image-load
>>  
>>  .. only::  subproject and html
>>  
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 6c63415d2ac2..4e7f48fa7e5c 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -7358,6 +7358,14 @@ F:	Documentation/fpga/
>>  F:	drivers/fpga/
>>  F:	include/linux/fpga/
>>  
>> +FPGA SECURITY MANAGER DRIVERS
>> +M:	Russ Weight <russell.h.weight@...el.com>
>> +L:	linux-fpga@...r.kernel.org
>> +S:	Maintained
>> +F:	Documentation/fpga/fpga-image-load.rst
>> +F:	drivers/fpga/fpga-image-load.c
>> +F:	include/linux/fpga/fpga-image-load.h
>> +
>>  FPU EMULATOR
>>  M:	Bill Metzenthen <billm@...bpc.org.au>
>>  S:	Maintained
>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>> index 991b3f361ec9..c12a14e62fff 100644
>> --- a/drivers/fpga/Kconfig
>> +++ b/drivers/fpga/Kconfig
>> @@ -243,4 +243,14 @@ config FPGA_MGR_VERSAL_FPGA
>>  	  configure the programmable logic(PL).
>>  
>>  	  To compile this as a module, choose M here.
>> +
>> +config FPGA_IMAGE_LOAD
>> +	tristate "FPGA Image Load Driver"
> Maybe we don't call it "Driver". A framework or "FPGA Image load support",
> is it better?
>
> There are more descriptions about "driver" below, maybe you need to change
> them all.

Yeah - I think that language sounds better.
>
>> +	help
>> +	  The FPGA Image Load class driver presents a common user API for
>> +	  uploading an image file to an FPGA device. The image file is
>> +	  expected to be self-describing. It is up to the device driver
>> +	  and/or the device itself to authenticate and disposition the
>> +	  image data.
>> +
>>  endif # FPGA
>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
>> index 0bff783d1b61..adf228ee4f5e 100644
>> --- a/drivers/fpga/Makefile
>> +++ b/drivers/fpga/Makefile
>> @@ -22,6 +22,9 @@ obj-$(CONFIG_FPGA_MGR_VERSAL_FPGA)      += versal-fpga.o
>>  obj-$(CONFIG_ALTERA_PR_IP_CORE)         += altera-pr-ip-core.o
>>  obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT)    += altera-pr-ip-core-plat.o
>>  
>> +# FPGA Image Load Framework
>> +obj-$(CONFIG_FPGA_IMAGE_LOAD)		+= fpga-image-load.o
>> +
>>  # FPGA Bridge Drivers
>>  obj-$(CONFIG_FPGA_BRIDGE)		+= fpga-bridge.o
>>  obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)	+= altera-hps2fpga.o altera-fpga2sdram.o
>> diff --git a/drivers/fpga/fpga-image-load.c b/drivers/fpga/fpga-image-load.c
>> new file mode 100644
>> index 000000000000..7d75bbcff541
>> --- /dev/null
>> +++ b/drivers/fpga/fpga-image-load.c
>> @@ -0,0 +1,124 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * FPGA Image Load Class Driver
>> + *
>> + * Copyright (C) 2019-2021 Intel Corporation, Inc.
>> + */
>> +
>> +#include <linux/fpga/fpga-image-load.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include <linux/vmalloc.h>
>> +
>> +#define IMAGE_LOAD_XA_LIMIT	XA_LIMIT(0, INT_MAX)
>> +static DEFINE_XARRAY_ALLOC(fpga_image_load_xa);
>> +
>> +static struct class *fpga_image_load_class;
>> +
>> +#define to_image_load(d) container_of(d, struct fpga_image_load, dev)
>> +
>> +/**
>> + * fpga_image_load_register - create and register an FPGA Image Load Device
>> + *
>> + * @parent: fpga image load device from pdev
>> + * @lops:   pointer to a structure of image load callback functions
> Maybe "ops" is just good, some more below.

OK
>
>> + * @priv:   fpga image load private data
>> + *
>> + * Returns a struct fpga_image_load pointer on success, or ERR_PTR() on
>> + * error. The caller of this function is responsible for calling
>> + * fpga_image_load_unregister().
>> + */
>> +struct fpga_image_load *
>> +fpga_image_load_register(struct device *parent,
>> +			 const struct fpga_image_load_ops *lops, void *priv)
>> +{
>> +	struct fpga_image_load *imgld;
>> +	int id, ret;
>> +
>> +	imgld = kzalloc(sizeof(*imgld), GFP_KERNEL);
>> +	if (!imgld)
>> +		return NULL;
>> +
>> +	ret = xa_alloc(&fpga_image_load_xa, &imgld->dev.id, imgld, IMAGE_LOAD_XA_LIMIT,
>> +		       GFP_KERNEL);
>> +	if (ret)
>> +		goto error_kfree;
>> +
>> +	mutex_init(&imgld->lock);
>> +
>> +	imgld->priv = priv;
>> +	imgld->lops = lops;
>> +
>> +	imgld->dev.class = fpga_image_load_class;
>> +	imgld->dev.parent = parent;
>> +
>> +	ret = dev_set_name(&imgld->dev, "fpga_image%d", id);
> Is it better "fpga_image_load%d"?
OK

Thanks for the comments,
- Russ
>
>> +	if (ret) {
>> +		dev_err(parent, "Failed to set device name: fpga_image%d\n", id);
>> +		goto error_device;
>> +	}
>> +
>> +	ret = device_register(&imgld->dev);
>> +	if (ret) {
>> +		put_device(&imgld->dev);
>> +		return ERR_PTR(ret);
>> +	}
>> +
>> +	return imgld;
>> +
>> +error_device:
>> +	xa_erase(&fpga_image_load_xa, imgld->dev.id);
>> +
>> +error_kfree:
>> +	kfree(imgld);
>> +
>> +	return ERR_PTR(ret);
>> +}
>> +EXPORT_SYMBOL_GPL(fpga_image_load_register);
>> +
>> +/**
>> + * fpga_image_load_unregister - unregister an FPGA image load device
>> + *
>> + * @imgld: pointer to struct fpga_image_load
>> + *
>> + * This function is intended for use in an FPGA Image Load driver's
>> + * remove() function.
>> + */
>> +void fpga_image_load_unregister(struct fpga_image_load *imgld)
>> +{
>> +	device_unregister(&imgld->dev);
>> +}
>> +EXPORT_SYMBOL_GPL(fpga_image_load_unregister);
>> +
>> +static void fpga_image_load_dev_release(struct device *dev)
>> +{
>> +	struct fpga_image_load *imgld = to_image_load(dev);
>> +
>> +	xa_erase(&fpga_image_load_xa, imgld->dev.id);
>> +	kfree(imgld);
>> +}
>> +
>> +static int __init fpga_image_load_class_init(void)
>> +{
>> +	pr_info("FPGA Image Load Driver\n");
>> +
>> +	fpga_image_load_class = class_create(THIS_MODULE, "fpga_image_load");
>> +	if (IS_ERR(fpga_image_load_class))
>> +		return PTR_ERR(fpga_image_load_class);
>> +
>> +	fpga_image_load_class->dev_release = fpga_image_load_dev_release;
>> +
>> +	return 0;
>> +}
>> +
>> +static void __exit fpga_image_load_class_exit(void)
>> +{
>> +	class_destroy(fpga_image_load_class);
>> +	WARN_ON(!xa_empty(&fpga_image_load_xa));
>> +}
>> +
>> +MODULE_DESCRIPTION("FPGA Image Load Driver");
>> +MODULE_LICENSE("GPL v2");
>> +
>> +subsys_initcall(fpga_image_load_class_init);
>> +module_exit(fpga_image_load_class_exit)
>> diff --git a/include/linux/fpga/fpga-image-load.h b/include/linux/fpga/fpga-image-load.h
>> new file mode 100644
>> index 000000000000..a9cef9e1056b
>> --- /dev/null
>> +++ b/include/linux/fpga/fpga-image-load.h
>> @@ -0,0 +1,35 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Header file for FPGA Image Load Driver
>> + *
>> + * Copyright (C) 2019-2021 Intel Corporation, Inc.
>> + */
>> +#ifndef _LINUX_FPGA_IMAGE_LOAD_H
>> +#define _LINUX_FPGA_IMAGE_LOAD_H
>> +
>> +#include <linux/device.h>
>> +#include <linux/mutex.h>
>> +#include <linux/types.h>
>> +
>> +struct fpga_image_load;
>> +
>> +/**
>> + * struct fpga_image_load_ops - device specific operations
>> + */
>> +struct fpga_image_load_ops {
>> +};
>> +
>> +struct fpga_image_load {
>> +	struct device dev;
>> +	const struct fpga_image_load_ops *lops;
>> +	struct mutex lock;		/* protect data structure contents */
>> +	void *priv;
>> +};
>> +
>> +struct fpga_image_load *
>> +fpga_image_load_register(struct device *dev,
>> +			 const struct fpga_image_load_ops *lops, void *priv);
>> +
>> +void fpga_image_load_unregister(struct fpga_image_load *imgld);
>> +
>> +#endif
>> -- 
>> 2.25.1
> Thanks,
> Yilun

Powered by blists - more mailing lists