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

[Lizhi Hou]

On 10/20/21 2:42 PM, Russ Weight wrote:
> 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

Thanks a lot. This is very helpful.

Lizhi

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