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@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxx>
>> +L: linux-fpga@xxxxxxxxxxxxxxx
>> +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@xxxxxxxxxxxxx>
>> 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
