On 9/13/21 2:36 AM, Xu Yilun wrote:
> On Mon, Sep 13, 2021 at 02:48:45PM +0800, Xu Yilun wrote:
>> On Fri, Sep 10, 2021 at 04:18:26PM -0700, Russ Weight wrote:
>>>
>>> On 9/10/21 1:22 AM, Xu Yilun wrote:
>>>> On Wed, Sep 08, 2021 at 07:18:42PM -0700, Russ Weight wrote:
>>>>> Extend the FPGA Image Load class driver to include IOCTL support
>>>>> (FPGA_IMAGE_LOAD_WRITE) for initiating an upload of an image to a device.
>>>>> The IOCTL will return immediately, and the update will begin in the
>>>>> context of a kernel worker thread.
>>>>>
>>>>> Signed-off-by: Russ Weight <russell.h.weight@xxxxxxxxx>
>>>>> ---
>>>>> v15:
>>>>> - Compare to previous patch:
>>>>> [PATCH v14 2/6] fpga: sec-mgr: enable secure updates
>>>>> - Changed file, symbol, and config names to reflect the new driver name
>>>>> - Removed update/filename sysfs file and added the FPGA_IMAGE_LOAD_WRITE
>>>>> IOCTL for writing the image data to the FPGA card. The driver no longer
>>>>> uses the request_firmware framework.
>>>>> - Fixed some error return values in fpga_image_load_register()
>>>>> - Removed signed-off/reviewed-by tags
>>>>> v14:
>>>>> - Added MAINTAINERS reference for
>>>>> Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
>>>>> - Updated ABI documentation date and kernel version
>>>>> - Updated copyright to 2021
>>>>> v13:
>>>>> - Change "if (count == 0 || " to "if (!count || "
>>>>> - Improve error message: "Attempt to register without all required ops\n"
>>>>> v12:
>>>>> - Updated Date and KernelVersion fields in ABI documentation
>>>>> - Removed size parameter from write_blk() op - it is now up to
>>>>> the lower-level driver to determine the appropriate size and
>>>>> to update smgr->remaining_size accordingly.
>>>>> v11:
>>>>> - Fixed a spelling error in a comment
>>>>> - Initialize smgr->err_code and smgr->progress explicitly in
>>>>> fpga_sec_mgr_create() instead of accepting the default 0 value.
>>>>> v10:
>>>>> - Rebased to 5.12-rc2 next
>>>>> - Updated Date and KernelVersion in ABI documentation
>>>>> v9:
>>>>> - Updated Date and KernelVersion in ABI documentation
>>>>> v8:
>>>>> - No change
>>>>> v7:
>>>>> - Changed Date in documentation file to December 2020
>>>>> - Changed filename_store() to use kmemdup_nul() instead of
>>>>> kstrndup() and changed the count to not assume a line-return.
>>>>> v6:
>>>>> - Changed "security update" to "secure update" in commit message
>>>>> v5:
>>>>> - When checking the return values for functions of type enum
>>>>> fpga_sec_err err_code, test for FPGA_SEC_ERR_NONE instead of 0
>>>>> 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:
>>>>> - Removed unnecessary "goto done"
>>>>> - Added a comment to explain imgr->driver_unload in
>>>>> ifpga_sec_mgr_unregister()
>>>>> v2:
>>>>> - Bumped documentation date and version
>>>>> - Removed explicit value assignments in enums
>>>>> - Other minor code cleanup per review comments
>>>>> ---
>>>>> Documentation/fpga/fpga-image-load.rst | 21 +++
>>>>> MAINTAINERS | 1 +
>>>>> drivers/fpga/fpga-image-load.c | 224 ++++++++++++++++++++++++-
>>>>> include/linux/fpga/fpga-image-load.h | 29 ++++
>>>>> include/uapi/linux/fpga-image-load.h | 58 +++++++
>>>>> 5 files changed, 329 insertions(+), 4 deletions(-)
>>>>> create mode 100644 include/uapi/linux/fpga-image-load.h
>>>>>
>>>>> diff --git a/Documentation/fpga/fpga-image-load.rst b/Documentation/fpga/fpga-image-load.rst
>>>>> index a6e53ac66026..2ca8d2f0212d 100644
>>>>> --- a/Documentation/fpga/fpga-image-load.rst
>>>>> +++ b/Documentation/fpga/fpga-image-load.rst
>>>>> @@ -8,3 +8,24 @@ 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.
>>>>> +
>>>>> +User API
>>>>> +========
>>>>> +
>>>>> +open
>>>>> +----
>>>>> +
>>>>> +An FPGA Image Load device is opened exclusively to control an image load.
>>>>> +Image loads are processed by a kernel worker thread. A user may choose
>>>>> +close the device while the upload continues.
>>>> Why we allow the user to close the dev while the uploading is ongoing?
>>>> Seems it introduces more checking effort when another user open the dev
>>>> again and try another uploading.
>>> A user could choose to write their own tools. How would we prevent
>>> them from closing the device while an update is in progress? Should
>>> we attempt to cancel the update if they close the device?
>> I think we could try to cancel the update when close(), if we cannot
>> cancel we block on close().
>>
>> The driver should be responsible for the integrity of the update flow,
>> it is not prefered a user starts an update and leaves, but canceled by
>> another user.
OK - I can make those changes for the next version.
>>> Concurrent updates are already prevented by returning EBUSY for the
>>> FPGA_IMAGE_LOAD_WRITE IOCTL if an update is already in progress.
>>>
>>> A single IOCTL is sufficient to do a full update. Maybe a user
>>> would want to have three small tools: update_start, update_status,
>>> update_cancel, each of which would open the device, do an IOCTL,
>>> and then close the device. This is analogous to the sysfs
>>> implementation (no open/close that bounds the entire sequence).
>>>
>>> With the current design, we do an exclusive open. As long as the user
>>> keeps the device open, no other process can open the device to start
>>> a new update, cancel, or collect status.
>>>
>>> Those are my thoughts. What do you think? Is it OK as is? Or should
>>> I make some changes here?
>>>
>>>>> +
>>>>> +ioctl
>>>>> +-----
>>>>> +
>>>>> +FPGA_IMAGE_LOAD_WRITE:
>>>>> +
>>>>> +Start an image load with the provided image buffer. This IOCTL returns
>>>>> +immediately after starting a kernel worker thread to process the image load
>>>>> +which could take as long a 40 minutes depending on the actual device being
>>>>> +updated. This is an exclusive operation; an attempt to start concurrent image
>>>>> +load for the same device will fail with EBUSY.
>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>> index 4e7f48fa7e5c..637bc003ca81 100644
>>>>> --- a/MAINTAINERS
>>>>> +++ b/MAINTAINERS
>>>>> @@ -7365,6 +7365,7 @@ S: Maintained
>>>>> F: Documentation/fpga/fpga-image-load.rst
>>>>> F: drivers/fpga/fpga-image-load.c
>>>>> F: include/linux/fpga/fpga-image-load.h
>>>>> +F: include/uapi/linux/fpga-image-load.h
>>>>>
>>>>> FPU EMULATOR
>>>>> M: Bill Metzenthen <billm@xxxxxxxxxxxxx>
>>>>> diff --git a/drivers/fpga/fpga-image-load.c b/drivers/fpga/fpga-image-load.c
>>>>> index 7d75bbcff541..f5ccfa9dd977 100644
>>>>> --- a/drivers/fpga/fpga-image-load.c
>>>>> +++ b/drivers/fpga/fpga-image-load.c
>>>>> @@ -5,18 +5,181 @@
>>>>> * Copyright (C) 2019-2021 Intel Corporation, Inc.
>>>>> */
>>>>>
>>>>> +#include <linux/delay.h>
>>>>> #include <linux/fpga/fpga-image-load.h>
>>>>> +#include <linux/fs.h>
>>>>> +#include <linux/kernel.h>
>>>>> #include <linux/module.h>
>>>>> #include <linux/slab.h>
>>>>> +#include <linux/uaccess.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;
>>>>> +static dev_t fpga_image_devt;
>>>>>
>>>>> #define to_image_load(d) container_of(d, struct fpga_image_load, dev)
>>>>>
>>>>> +static void fpga_image_dev_error(struct fpga_image_load *imgld,
>>>>> + enum fpga_image_err err_code)
>>>>> +{
>>>>> + imgld->err_code = err_code;
>>>>> + imgld->lops->cancel(imgld);
>>>>> +}
>>>>> +
>>>>> +static void fpga_image_prog_complete(struct fpga_image_load *imgld)
>>>>> +{
>>>>> + mutex_lock(&imgld->lock);
>>>>> + imgld->progress = FPGA_IMAGE_PROG_IDLE;
>>>>> + complete_all(&imgld->update_done);
>>>>> + mutex_unlock(&imgld->lock);
>>>>> +}
>>>>> +
>>>>> +static void fpga_image_do_load(struct work_struct *work)
>>>>> +{
>>>>> + struct fpga_image_load *imgld;
>>>>> + enum fpga_image_err ret;
>>>>> + u32 size, offset = 0;
>>>>> +
>>>>> + imgld = container_of(work, struct fpga_image_load, work);
>>>>> + size = imgld->remaining_size;
>>>>> +
>>>>> + get_device(&imgld->dev);
>>>>> + if (!try_module_get(imgld->dev.parent->driver->owner)) {
>>>>> + imgld->err_code = FPGA_IMAGE_ERR_BUSY;
>>>>> + goto idle_exit;
>>>>> + }
>>>>> +
>>>>> + imgld->progress = FPGA_IMAGE_PROG_PREPARING;
>>>>> + ret = imgld->lops->prepare(imgld);
>>>>> + if (ret != FPGA_IMAGE_ERR_NONE) {
>>>>> + fpga_image_dev_error(imgld, ret);
>>>>> + goto modput_exit;
>>>>> + }
>>>>> +
>>>>> + imgld->progress = FPGA_IMAGE_PROG_WRITING;
>>>>> + while (imgld->remaining_size) {
>>>>> + ret = imgld->lops->write_blk(imgld, offset);
>>>>> + if (ret != FPGA_IMAGE_ERR_NONE) {
>>>>> + fpga_image_dev_error(imgld, ret);
>>>>> + goto done;
>>>>> + }
>>>>> +
>>>>> + offset = size - imgld->remaining_size;
>>>> The low level driver is required to update the "remaining_size" in
>>>> write_blk ops?
>>>>
>>>> The API seems ambiguous. The framework asks for writing a block of data,
>>>> but no block size is specified.
>>> This change was made two or three iterations ago at Richard Gong's
>>> suggestion. He asserted that the lower level driver should be in
>>> control of the block size (based on write speeds). What do you
>> I think it is reasonable.
>>
>>> think? Should the class driver impose a fixed size? Or allow the
>> But the definition of the API is hard to comprehend, either we input the
>> offset & the blk size, or we record them all in imgld structure, is it
>> better.
>>
>> If the framework provides the blk size, the low level driver could
>> return the written size so it could still control its own batch.
I'll clean up the API for this function in the next version of
the patches.
>>
>>> lower-level driver to determine the size? Is it OK to update
>>> remaining_size in the lower-level driver? Or should there be
>>> another call-back to request the size?
>>>
>>>>> + }
>>>>> +
>>>>> + imgld->progress = FPGA_IMAGE_PROG_PROGRAMMING;
>>>>> + ret = imgld->lops->poll_complete(imgld);
>>>>> + if (ret != FPGA_IMAGE_ERR_NONE)
>>>>> + fpga_image_dev_error(imgld, ret);
>>>>> +
>>>>> +done:
>>>>> + if (imgld->lops->cleanup)
>>>>> + imgld->lops->cleanup(imgld);
>>>>> +
>>>>> +modput_exit:
>>>>> + module_put(imgld->dev.parent->driver->owner);
>>>>> +
>>>>> +idle_exit:
>>>>> + /*
>>>>> + * Note: imgld->remaining_size is left unmodified here to provide
>>>>> + * additional information on errors. It will be reinitialized when
>>>>> + * the next image load begins.
>>>>> + */
>>>>> + vfree(imgld->data);
>>>>> + imgld->data = NULL;
>>>>> + put_device(&imgld->dev);
>>>>> + fpga_image_prog_complete(imgld);
>>>>> +}
>>>>> +
>>>>> +static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
>>>>> + unsigned long arg)
>>>>> +{
>>>>> + struct fpga_image_write wb;
>>>>> + unsigned long minsz;
>>>>> + u8 *buf;
>>>>> +
>>>>> + if (imgld->driver_unload || imgld->progress != FPGA_IMAGE_PROG_IDLE)
>>>>> + return -EBUSY;
>>>>> +
>>>>> + minsz = offsetofend(struct fpga_image_write, buf);
>>>>> + if (copy_from_user(&wb, (void __user *)arg, minsz))
>>>>> + return -EFAULT;
>>>>> +
>>>>> + if (wb.flags)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + /* Enforce 32-bit alignment on the write data */
>>>>> + if (wb.size & 0x3)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + buf = vzalloc(wb.size);
>>>>> + if (!buf)
>>>>> + return -ENOMEM;
>>>>> +
>>>>> + if (copy_from_user(buf, u64_to_user_ptr(wb.buf), wb.size)) {
>>>>> + vfree(buf);
>>>>> + return -EFAULT;
>>>>> + }
>>>>> +
>>>>> + imgld->data = buf;
>>>>> + imgld->remaining_size = wb.size;
>>>>> + imgld->err_code = FPGA_IMAGE_ERR_NONE;
>>>>> + imgld->progress = FPGA_IMAGE_PROG_STARTING;
>>>>> + reinit_completion(&imgld->update_done);
>>>>> + schedule_work(&imgld->work);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static long fpga_image_load_ioctl(struct file *filp, unsigned int cmd,
>>>>> + unsigned long arg)
>>>>> +{
>>>>> + struct fpga_image_load *imgld = filp->private_data;
>>>>> + int ret = -ENOTTY;
>>>>> +
>>>>> + switch (cmd) {
>>>>> + case FPGA_IMAGE_LOAD_WRITE:
>>>>> + mutex_lock(&imgld->lock);
>>>>> + ret = fpga_image_load_ioctl_write(imgld, arg);
>>>>> + mutex_unlock(&imgld->lock);
>>>>> + break;
>>>>> + }
>>>>> +
>>>>> + return ret;
>>>>> +}
>>>>> +
>>>>> +static int fpga_image_load_open(struct inode *inode, struct file *filp)
>>>>> +{
>>>>> + struct fpga_image_load *imgld = container_of(inode->i_cdev,
>>>>> + struct fpga_image_load, cdev);
>>>>> +
>>>>> + if (test_and_set_bit(0, &imgld->opened))
>>>> Some more flags to add for "opened" field? But the field name indicates
>>>> it is a single flag.
>>> Can you explain your comment further? I'm not understanding. What
>>> "more flags" are you referring to?
>>>
>> The test_and_set_bit() makes me feel there are more bits to be
>> controlled in "opened" for other purposes. The name "opened" indicates
>> this is a single state, maybe you don't have to use bitops for it,
>> atomic ops for int maybe better.
OK - I'll use an atomic operation here.
>>
>>> This test_and_set_bit() operation and the "opened" structure member
>>> were borrowed from the production driver implementation. opened is a
>>> single value that is expected to be either 1 or 0.
>>>
>>>>> + return -EBUSY;
>>>>> +
>>>>> + filp->private_data = imgld;
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static int fpga_image_load_release(struct inode *inode, struct file *filp)
>>>>> +{
>>>>> + struct fpga_image_load *imgld = filp->private_data;
>>>>> +
>>>>> + clear_bit(0, &imgld->opened);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static const struct file_operations fpga_image_load_fops = {
>>>>> + .owner = THIS_MODULE,
>>>>> + .open = fpga_image_load_open,
>>>>> + .release = fpga_image_load_release,
>>>>> + .unlocked_ioctl = fpga_image_load_ioctl,
>>>>> +};
>>>>> +
>>>>> /**
>>>>> * fpga_image_load_register - create and register an FPGA Image Load Device
>>>>> *
>>>>> @@ -33,11 +196,17 @@ fpga_image_load_register(struct device *parent,
>>>>> const struct fpga_image_load_ops *lops, void *priv)
>>>>> {
>>>>> struct fpga_image_load *imgld;
>>>>> - int id, ret;
>>>>> + int ret;
>>>>> +
>>>>> + if (!lops || !lops->cancel || !lops->prepare ||
>>>>> + !lops->write_blk || !lops->poll_complete) {
>>>>> + dev_err(parent, "Attempt to register without all required ops\n");
>>>>> + return ERR_PTR(-ENOMEM);
>>>>> + }
>>>>>
>>>>> imgld = kzalloc(sizeof(*imgld), GFP_KERNEL);
>>>>> if (!imgld)
>>>>> - return NULL;
>>>>> + return ERR_PTR(-ENOMEM);
>>>> This is the fix for Patch #1? If yes please merge it to Patch #1.
>>> Good catch. Yes, I'll move it to patch #1.
>>>>>
>>>>> ret = xa_alloc(&fpga_image_load_xa, &imgld->dev.id, imgld, IMAGE_LOAD_XA_LIMIT,
>>>>> GFP_KERNEL);
>>>>> @@ -48,13 +217,19 @@ fpga_image_load_register(struct device *parent,
>>>>>
>>>>> imgld->priv = priv;
>>>>> imgld->lops = lops;
>>>>> + imgld->err_code = FPGA_IMAGE_ERR_NONE;
>>>>> + imgld->progress = FPGA_IMAGE_PROG_IDLE;
>>>>> + init_completion(&imgld->update_done);
>>>>> + INIT_WORK(&imgld->work, fpga_image_do_load);
>>>>>
>>>>> imgld->dev.class = fpga_image_load_class;
>>>>> imgld->dev.parent = parent;
>>>>> + imgld->dev.devt = MKDEV(MAJOR(fpga_image_devt), imgld->dev.id);
>>>>>
>>>>> - ret = dev_set_name(&imgld->dev, "fpga_image%d", id);
>>>>> + ret = dev_set_name(&imgld->dev, "fpga_image%d", imgld->dev.id);
>>>> Another fix? Please merge it to Patch #1.
>>> Yes.
>>>
>>>>> if (ret) {
>>>>> - dev_err(parent, "Failed to set device name: fpga_image%d\n", id);
>>>>> + dev_err(parent, "Failed to set device name: fpga_image%d\n",
>>>>> + imgld->dev.id);
>>>> Ditto
>>> Yes.
>>>>> goto error_device;
>>>>> }
>>>>>
>>>>> @@ -64,6 +239,16 @@ fpga_image_load_register(struct device *parent,
>>>>> return ERR_PTR(ret);
>>>>> }
>>>>>
>>>>> + cdev_init(&imgld->cdev, &fpga_image_load_fops);
>>>>> + imgld->cdev.owner = parent->driver->owner;
>>>>> + imgld->cdev.kobj.parent = &imgld->dev.kobj;
>>>>> +
>>>>> + ret = cdev_add(&imgld->cdev, imgld->dev.devt, 1);
>>>>> + if (ret) {
>>>>> + put_device(&imgld->dev);
>>>>> + return ERR_PTR(ret);
>>>>> + }
>>>>> +
>>>>> return imgld;
>>>>>
>>>>> error_device:
>>>>> @@ -83,9 +268,29 @@ EXPORT_SYMBOL_GPL(fpga_image_load_register);
>>>>> *
>>>>> * This function is intended for use in an FPGA Image Load driver's
>>>>> * remove() function.
>>>>> + *
>>>>> + * For some devices, once authentication of the uploaded image has begun,
>>>>> + * the hardware cannot be signaled to stop, and the driver will not exit
>>>>> + * until the hardware signals completion. This could be 30+ minutes of
>>>>> + * waiting. The driver_unload flag enables a force-unload of the driver
>>>>> + * (e.g. modprobe -r) by signaling the parent driver to exit even if the
>>>> How does the driver_unload enables the force unload of the parent
>>>> driver? I didn't find the code.
>>> The driver_unload field is tested in the lower-level (parent) driver in
>>> m10bmc_sec_poll_complete(), allowing the kernel worker thread to exit
>>> even if the firmware update is still in progress.
>> I didn't see the signaling implementation here. Maybe you need to move the
>> comments somewhere else, or in another patch.
> I have another concern. the design requires the low level driver poll
> for a field of the input parameter that the framework manages. It is
> unnormal, like an implicit cancel call to the driver.
I'm cleaning up the driver_unload. I'm also changing the release
(close) function to set the request_cancel flag when need. I think
I have addressed all of your concerns for the next version of the
patches.
Thanks,
- Russ
>
> How about the framework explicitly call cancel() when needed? Of cource
> we need to declare the cancel() could be called when other callback is
> in progress.
>
> Thanks,
> Yilun
>
>> Thanks,
>> Yilun
>>
>>>>> + * hardware update is incomplete. The driver_unload flag also prevents
>>>>> + * new updates from starting once the unregister process has begun.
>>>>> */
>>>>> void fpga_image_load_unregister(struct fpga_image_load *imgld)
>>>>> {
>>>>> + mutex_lock(&imgld->lock);
>>>>> + imgld->driver_unload = true;
>>>>> + if (imgld->progress == FPGA_IMAGE_PROG_IDLE) {
>>>>> + mutex_unlock(&imgld->lock);
>>>>> + goto unregister;
>>>>> + }
>>>>> +
>>>>> + mutex_unlock(&imgld->lock);
>>>>> + wait_for_completion(&imgld->update_done);
>>>>> +
>>>>> +unregister:
>>>>> + cdev_del(&imgld->cdev);
>>>>> device_unregister(&imgld->dev);
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(fpga_image_load_unregister);
>>>>> @@ -100,19 +305,30 @@ static void fpga_image_load_dev_release(struct device *dev)
>>>>>
>>>>> static int __init fpga_image_load_class_init(void)
>>>>> {
>>>>> + int ret;
>>>>> 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);
>>>>>
>>>>> + ret = alloc_chrdev_region(&fpga_image_devt, 0, MINORMASK,
>>>>> + "fpga_image_load");
>>>>> + if (ret)
>>>>> + goto exit_destroy_class;
>>>>> +
>>>>> fpga_image_load_class->dev_release = fpga_image_load_dev_release;
>>>>>
>>>>> return 0;
>>>>> +
>>>>> +exit_destroy_class:
>>>>> + class_destroy(fpga_image_load_class);
>>>>> + return ret;
>>>>> }
>>>>>
>>>>> static void __exit fpga_image_load_class_exit(void)
>>>>> {
>>>>> + unregister_chrdev_region(fpga_image_devt, MINORMASK);
>>>>> class_destroy(fpga_image_load_class);
>>>>> WARN_ON(!xa_empty(&fpga_image_load_xa));
>>>>> }
>>>>> diff --git a/include/linux/fpga/fpga-image-load.h b/include/linux/fpga/fpga-image-load.h
>>>>> index a9cef9e1056b..b3d790e5d943 100644
>>>>> --- a/include/linux/fpga/fpga-image-load.h
>>>>> +++ b/include/linux/fpga/fpga-image-load.h
>>>>> @@ -7,22 +7,51 @@
>>>>> #ifndef _LINUX_FPGA_IMAGE_LOAD_H
>>>>> #define _LINUX_FPGA_IMAGE_LOAD_H
>>>>>
>>>>> +#include <linux/cdev.h>
>>>>> +#include <linux/completion.h>
>>>>> #include <linux/device.h>
>>>>> #include <linux/mutex.h>
>>>>> #include <linux/types.h>
>>>>> +#include <uapi/linux/fpga-image-load.h>
>>>>>
>>>>> struct fpga_image_load;
>>>>>
>>>>> /**
>>>>> * struct fpga_image_load_ops - device specific operations
>>>>> + * @prepare: Required: Prepare secure update
>>>>> + * @write_blk: Required: Write a block of data
>>>>> + * @poll_complete: Required: Check for the completion of the
>>>>> + * HW authentication/programming process. This
>>>>> + * function should check for imgld->driver_unload
>>>>> + * and abort with FPGA_IMAGE_ERR_CANCELED when true.
>>>>> + * @cancel: Required: Signal HW to cancel update
>>>>> + * @cleanup: Optional: Complements the prepare()
>>>>> + * function and is called at the completion
>>>>> + * of the update, whether success or failure,
>>>>> + * if the prepare function succeeded.
>>>>> */
>>>>> struct fpga_image_load_ops {
>>>>> + enum fpga_image_err (*prepare)(struct fpga_image_load *imgld);
>>>>> + enum fpga_image_err (*write_blk)(struct fpga_image_load *imgld, u32 offset);
>>>>> + enum fpga_image_err (*poll_complete)(struct fpga_image_load *imgld);
>>>>> + enum fpga_image_err (*cancel)(struct fpga_image_load *imgld);
>>>>> + void (*cleanup)(struct fpga_image_load *imgld);
>>>>> };
>>>>>
>>>>> struct fpga_image_load {
>>>>> struct device dev;
>>>>> + struct cdev cdev;
>>>>> const struct fpga_image_load_ops *lops;
>>>>> struct mutex lock; /* protect data structure contents */
>>>>> + unsigned long opened;
>>>>> + struct work_struct work;
>>>>> + struct completion update_done;
>>>>> + const u8 *data; /* pointer to update data */
>>>>> + u32 remaining_size; /* size remaining to transfer */
>>>>> + enum fpga_image_prog progress;
>>>>> + enum fpga_image_prog err_progress; /* progress at time of failure */
>>>> This field is not used in this patch? So could you introduce it later?
>>> Yes - I'll move it. Thanks.
>>>>> + enum fpga_image_err err_code; /* image load error code */
>>>>> + bool driver_unload;
>>>>> void *priv;
>>>>> };
>>>>>
>>>>> diff --git a/include/uapi/linux/fpga-image-load.h b/include/uapi/linux/fpga-image-load.h
>>>>> new file mode 100644
>>>>> index 000000000000..4146a0a9e408
>>>>> --- /dev/null
>>>>> +++ b/include/uapi/linux/fpga-image-load.h
>>>>> @@ -0,0 +1,58 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>>>>> +/*
>>>>> + * Header File for FPGA Image Load User API
>>>>> + *
>>>>> + * Copyright (C) 2019-2021 Intel Corporation, Inc.
>>>>> + *
>>>>> + */
>>>>> +
>>>>> +#ifndef _UAPI_LINUX_FPGA_IMAGE_LOAD_H
>>>>> +#define _UAPI_LINUX_FPGA_IMAGE_LOAD_H
>>>>> +
>>>>> +#include <linux/types.h>
>>>>> +#include <linux/ioctl.h>
>>>>> +
>>>>> +#define FPGA_IMAGE_LOAD_MAGIC 0xB9
>>>>> +
>>>>> +/* Image load progress codes */
>>>>> +enum fpga_image_prog {
>>>>> + FPGA_IMAGE_PROG_IDLE,
>>>>> + FPGA_IMAGE_PROG_STARTING,
>>>>> + FPGA_IMAGE_PROG_PREPARING,
>>>>> + FPGA_IMAGE_PROG_WRITING,
>>>>> + FPGA_IMAGE_PROG_PROGRAMMING,
>>>>> + FPGA_IMAGE_PROG_MAX
>>>>> +};
>>>>> +
>>>>> +/* Image error progress codes */
>>>>> +enum fpga_image_err {
>>>>> + FPGA_IMAGE_ERR_NONE,
>>>>> + FPGA_IMAGE_ERR_HW_ERROR,
>>>>> + FPGA_IMAGE_ERR_TIMEOUT,
>>>>> + FPGA_IMAGE_ERR_CANCELED,
>>>>> + FPGA_IMAGE_ERR_BUSY,
>>>>> + FPGA_IMAGE_ERR_INVALID_SIZE,
>>>>> + FPGA_IMAGE_ERR_RW_ERROR,
>>>>> + FPGA_IMAGE_ERR_WEAROUT,
>>>>> + FPGA_IMAGE_ERR_MAX
>>>>> +};
>>>>> +
>>>>> +#define FPGA_IMAGE_LOAD_WRITE _IOW(FPGA_IMAGE_LOAD_MAGIC, 0, struct fpga_image_write)
>>>> Put the cmd word definition under the comments and parameter definition.
>>> OK
>>>>> +
>>>>> +/**
>>>>> + * FPGA_IMAGE_LOAD_WRITE - _IOW(FPGA_IMAGE_LOAD_MAGIC, 0,
>>>>> + * struct fpga_image_write)
>>>>> + *
>>>>> + * Upload a data buffer to the target device. The user must provide the
>>>>> + * data buffer, size, and an eventfd file descriptor.
>>>> I didn't find the eventfd.
>>> It is added in a later patch. I'll change the comment accordingly.
>>>
>>> Thanks,
>>> - Russ
>>>> Thanks,
>>>> Yilun
>>>>
>>>>> + *
>>>>> + * Return: 0 on success, -errno on failure.
>>>>> + */
>>>>> +struct fpga_image_write {
>>>>> + /* Input */
>>>>> + __u32 flags; /* Zero for now */
>>>>> + __u32 size; /* Data size (in bytes) to be written */
>>>>> + __u64 buf; /* User space address of source data */
>>>>> +};
>>>>> +
>>>>> +#endif /* _UAPI_LINUX_FPGA_IMAGE_LOAD_H */
>>>>> --
>>>>> 2.25.1
