Message-ID: <20210910064658.GA754505@yilunxu-OptiPlex-7050>
Date:   Fri, 10 Sep 2021 14:46:58 +0800
From:   Xu Yilun <yilun.xu@...el.com>
To:     Russ Weight <russell.h.weight@...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 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.

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

> + * @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"?

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