Re: [PATCH v17 4/5] fpga: image-load: add status ioctl

From: Russ Weight
Date: Wed Oct 20 2021 - 17:43:09 EST




On 10/15/21 1:22 PM, Lizhi Hou wrote:
>
> On 9/29/21 4:00 PM, Russ Weight wrote:
>> Extend the FPGA Image Load framework to include an FPGA_IMAGE_LOAD_STATUS
>> IOCTL that can be used to monitor the progress of an ongoing image upload.
>> The status returned includes how much data remains to be transferred, the
>> progress of the image upload, and error information in the case of a
>> failure.
>>
>> Signed-off-by: Russ Weight <russell.h.weight@xxxxxxxxx>
>> ---
>> v17:
>>   - Rebased for changes to earlier patches.
>> v16:
>>   - Minor changes to adapt in changes in prevoius patches.
>> v15:
>>   - This patch is new to the patchset and provides an FPGA_IMAGE_LOAD_STATUS
>>     IOCTL to return the current values for: remaining_size, progress,
>>     err_progress, and err_code.
>>   - This patch has elements of the following three patches from the previous
>>     patch-set:
>>       [PATCH v14 3/6] fpga: sec-mgr: expose sec-mgr update status
>>       [PATCH v14 4/6] fpga: sec-mgr: expose sec-mgr update errors
>>       [PATCH v14 5/6] fpga: sec-mgr: expose sec-mgr update size
>>   - Changed file, symbol, and config names to reflect the new driver name
>>   - There are some minor changes to locking to enable this ioctl to return
>>     coherent data.
>> ---
>>   Documentation/fpga/fpga-image-load.rst |  6 +++
>>   drivers/fpga/fpga-image-load.c         | 58 +++++++++++++++++++++-----
>>   include/linux/fpga/fpga-image-load.h   |  1 +
>>   include/uapi/linux/fpga-image-load.h   | 18 ++++++++
>>   4 files changed, 73 insertions(+), 10 deletions(-)
>>
>> diff --git a/Documentation/fpga/fpga-image-load.rst b/Documentation/fpga/fpga-image-load.rst
>> index 487b5466f67c..f64f5ee473b8 100644
>> --- a/Documentation/fpga/fpga-image-load.rst
>> +++ b/Documentation/fpga/fpga-image-load.rst
>> @@ -33,3 +33,9 @@ being updated. This is an exclusive operation; an attempt to start
>>   concurrent image uploads for the same device will fail with EBUSY. An
>>   eventfd file descriptor parameter is provided to this IOCTL. It will be
>>   signalled at the completion of the image upload.
>> +
>> +FPGA_IMAGE_LOAD_STATUS:
>> +
>> +Collect status for an on-going image upload. The status returned includes
>> +how much data remains to be transferred, the progress of the image upload,
>> +and error information in the case of a failure.
>> diff --git a/drivers/fpga/fpga-image-load.c b/drivers/fpga/fpga-image-load.c
>> index f04dfc71c190..58373b9e8c02 100644
>> --- a/drivers/fpga/fpga-image-load.c
>> +++ b/drivers/fpga/fpga-image-load.c
>> @@ -22,6 +22,22 @@ static dev_t fpga_image_devt;
>>
>>   #define to_image_load(d) container_of(d, struct fpga_image_load, dev)
>>
>> +static void fpga_image_update_progress(struct fpga_image_load *imgld,
>> +                                      u32 new_progress)
>> +{
>> +       mutex_lock(&imgld->lock);
>> +       imgld->progress = new_progress;
>> +       mutex_unlock(&imgld->lock);
>> +}
>> +
>> +static void fpga_image_set_error(struct fpga_image_load *imgld, u32 err_code)
>> +{
>> +       mutex_lock(&imgld->lock);
>> +       imgld->err_progress = imgld->progress;
>> +       imgld->err_code = err_code;
>> +       mutex_unlock(&imgld->lock);
>> +}
>> +
>>   static void fpga_image_prog_complete(struct fpga_image_load *imgld)
>>   {
>>          mutex_lock(&imgld->lock);
>> @@ -38,24 +54,24 @@ static void fpga_image_do_load(struct work_struct *work)
>>          imgld = container_of(work, struct fpga_image_load, work);
>>
>>          if (imgld->driver_unload) {
>> -               imgld->err_code = FPGA_IMAGE_ERR_CANCELED;
>> +               fpga_image_set_error(imgld, FPGA_IMAGE_ERR_CANCELED);
>>                  goto idle_exit;
>>          }
>>
>>          get_device(&imgld->dev);
>>          if (!try_module_get(imgld->dev.parent->driver->owner)) {
>> -               imgld->err_code = FPGA_IMAGE_ERR_BUSY;
>> +               fpga_image_set_error(imgld, FPGA_IMAGE_ERR_BUSY);
>>                  goto putdev_exit;
>>          }
>>
>> -       imgld->progress = FPGA_IMAGE_PROG_PREPARING;
>> +       fpga_image_update_progress(imgld, FPGA_IMAGE_PROG_PREPARING);
>>          ret = imgld->ops->prepare(imgld, imgld->data, imgld->remaining_size);
>>          if (ret) {
>> -               imgld->err_code = ret;
>> +               fpga_image_set_error(imgld, ret);
>>                  goto modput_exit;
>>          }
>>
>> -       imgld->progress = FPGA_IMAGE_PROG_WRITING;
>> +       fpga_image_update_progress(imgld, FPGA_IMAGE_PROG_WRITING);
>>          while (imgld->remaining_size) {
>>                  ret = imgld->ops->write(imgld, imgld->data, offset,
>>                                          imgld->remaining_size);
>> @@ -65,7 +81,7 @@ static void fpga_image_do_load(struct work_struct *work)
>>                                           "write-op wrote zero data\n");
>>                                  ret = -FPGA_IMAGE_ERR_RW_ERROR;
>>                          }
>> -                       imgld->err_code = -ret;
>> +                       fpga_image_set_error(imgld, -ret);
>>                          goto done;
>>                  }
>>
>> @@ -73,10 +89,10 @@ static void fpga_image_do_load(struct work_struct *work)
>>                  offset += ret;
>>          }
>>
>> -       imgld->progress = FPGA_IMAGE_PROG_PROGRAMMING;
>> +       fpga_image_update_progress(imgld, FPGA_IMAGE_PROG_PROGRAMMING);
>>          ret = imgld->ops->poll_complete(imgld);
>>          if (ret)
>> -               imgld->err_code = ret;
>> +               fpga_image_set_error(imgld, ret);
>>
>>   done:
>>          if (imgld->ops->cleanup)
>> @@ -151,20 +167,42 @@ static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
>>          return ret;
>>   }
>>
>> +static int fpga_image_load_ioctl_status(struct fpga_image_load *imgld,
>> +                                       unsigned long arg)
>> +{
>> +       struct fpga_image_status status;
>> +
>> +       memset(&status, 0, sizeof(status));
>> +       status.progress = imgld->progress;
>> +       status.remaining_size = imgld->remaining_size;
>> +       status.err_progress = imgld->err_progress;
>> +       status.err_code = imgld->err_code;
>> +
>> +       if (copy_to_user((void __user *)arg, &status, sizeof(status)))
>> +               return -EFAULT;
>> +
>> +       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;
>>
>> +       mutex_lock(&imgld->lock);
>> +
>>          switch (cmd) {
>>          case FPGA_IMAGE_LOAD_WRITE:
>> -               mutex_lock(&imgld->lock);
>>                  ret = fpga_image_load_ioctl_write(imgld, arg);
>> -               mutex_unlock(&imgld->lock);
>> +               break;
>> +       case FPGA_IMAGE_LOAD_STATUS:
>> +               ret = fpga_image_load_ioctl_status(imgld, arg);
>>                  break;
>>          }
>>
>> +       mutex_unlock(&imgld->lock);
>> +
>>          return ret;
>>   }
>>
>> diff --git a/include/linux/fpga/fpga-image-load.h b/include/linux/fpga/fpga-image-load.h
>> index 77b3c91ce073..366111d090fb 100644
>> --- a/include/linux/fpga/fpga-image-load.h
>> +++ b/include/linux/fpga/fpga-image-load.h
>> @@ -49,6 +49,7 @@ struct fpga_image_load {
>>          const u8 *data;                 /* pointer to update data */
>>          u32 remaining_size;             /* size remaining to transfer */
>>          u32 progress;
>> +       u32 err_progress;               /* progress at time of error */
>>          u32 err_code;                   /* image load error code */
>>          bool driver_unload;
>>          struct eventfd_ctx *finished;
>> diff --git a/include/uapi/linux/fpga-image-load.h b/include/uapi/linux/fpga-image-load.h
>> index 8d2d3db92e87..1b91343961df 100644
>> --- a/include/uapi/linux/fpga-image-load.h
>> +++ b/include/uapi/linux/fpga-image-load.h
>> @@ -51,4 +51,22 @@ struct fpga_image_write {
>>
>>   #define FPGA_IMAGE_LOAD_WRITE  _IOW(FPGA_IMAGE_LOAD_MAGIC, 0, struct fpga_image_write)
>>
>> +/**
>> + * FPGA_IMAGE_LOAD_STATUS - _IOR(FPGA_IMAGE_LOAD_MAGIC, 1,
>> + *                              struct fpga_image_status)
>> + *
>> + * Request status information for an ongoing update.
>> + *
>> + * Return: 0 on success, -errno on failure.
>> + */
>> +struct fpga_image_status {
>> +       /* Output */
>> +       __u32 remaining_size;   /* size remaining to transfer */
>> +       __u32 progress;         /* current progress of image load */
>> +       __u32 err_progress;     /* progress at time of error */
>> +       __u32 err_code;         /* error code */
>> +};
>
> Could this be extended to also collect the image detail?
>
>         Image version, name, etc been successfully written to device (flash).
>
>         Image version, name, etc is currently running on the device.
>
> Or maybe add another query command to do these?
>
> So the userland utility will be able to show what image is running and what image is going to run with next cold reboot.

This status call is intended specifically to show the progress and
status of the image load. Yilun's comments on patch 5 suggest that
some of these types of operations could be handled as part of the
fpga-region class driver

I have patches that have not been submitted yet that contain some
of these types of features, although not in the class driver (yet).
Here is a link to documentation for what I have now. Take a look
at the sysfs node descriptions that are in the control subdirectory.
These are listed at the bottom of the file.

https://github.com/OPAE/linux-dfl/blob/fpga-ofs-dev/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-sec-update

- Russ


>
>
> Thanks,
>
> Lizhi
>
>> +
>> +#define FPGA_IMAGE_LOAD_STATUS _IOR(FPGA_IMAGE_LOAD_MAGIC, 1, struct fpga_image_status)
>> +
>>   #endif /* _UAPI_LINUX_FPGA_IMAGE_LOAD_H */
>> --
>> 2.25.1
>>