Re: [RFC 2/2] fpga manager: Add fpga_mgr_firmware_stream API

[Alan Tull <delicious.quinoa@gmail.com>]

On Thu, Mar 9, 2017 at 6:18 PM,  <yi1.li@linux.intel.com> wrote:

Hi Yi,

Thanks for your RFC.  I believe this functionality is badly needed.

I had a few comments about the chunk size and some nits about comments below...

> From: Yi Li <yi1.li@linux.intel.com>
>
> Add fpga_mgr_firmware_stream API, which can load and program firmware
>
> in trucks to FPGA instead of the whole file.
>
> Signed-off-by: Yi Li <yi1.li@linux.intel.com>
> ---
>  drivers/fpga/fpga-mgr.c       | 77 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/fpga/fpga-mgr.h |  4 +++
>  2 files changed, 81 insertions(+)
>
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index 3206a53..bb55b80 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -27,10 +27,15 @@
>  #include <linux/slab.h>
>  #include <linux/scatterlist.h>
>  #include <linux/highmem.h>
> +#include <linux/sizes.h>
>
>  static DEFINE_IDA(fpga_mgr_ida);
>  static struct class *fpga_mgr_class;
>
> +static int streamsize = SZ_4K;
> +module_param(streamsize, int, 0664);
> +MODULE_PARM_DESC(streamsize, "buffer size for firmware streaming");

I think we could fix the chunk size at PAGE_SIZE unless someone can
state a  reason for needing something different.  Below I have one
reason why we will sometimes need > PAGE_SIZE.

> +
>  /*
>   * Call the low level driver's write_init function.  This will do the
>   * device-specific things to get the FPGA into the state where it is ready to
> @@ -309,6 +314,78 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr,
>  }
>  EXPORT_SYMBOL_GPL(fpga_mgr_firmware_load);
>
> +/**
> + * fpga_mgr_firmware_load - request firmware and load to fpga
> + * @mgr:       fpga manager
> + * @info:      fpga image specific information
> + * @image_name:        name of image file on the firmware search path
> + *
> + * Request an FPGA image using the firmware class, then write out to the FPGA.
> + * Update the state before each step to provide info on what step failed if
> + * there is a failure.  This code assumes the caller got the mgr pointer
> + * from of_fpga_mgr_get() or fpga_mgr_get() and checked that it is not an error
> + * code.
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +int fpga_mgr_firmware_stream(struct fpga_manager *mgr,
> +                          struct fpga_image_info *info,
> +                          const char *image_name)

No need for both fpga_mgr_firmware_load and fpga_mgr_firmware_stream.
Just replace the old fpga_mgr_firmware_load with this function.

> +{
> +       struct device *dev = &mgr->dev;
> +       const struct firmware *fw = NULL;
> +       int ret;
> +       size_t size = INT_MAX, offset = 0;
> +       bool start_flag = 1;
> +
> +       dev_info(dev, "writing %s to %s with buffer size %d\n",
> +                       image_name, mgr->name, streamsize);
> +
> +       mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ;
> +
> +       while (size > 0) {
> +               ret = stream_firmware(&fw, image_name, dev, offset, streamsize);
> +               if (ret) {
> +                       dev_err(dev, "Error reading firmware %d\n", ret);
> +                       mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR;
> +                       break;
> +               }
> +
> +               /*
> +                * init.
> +                */

We don't really need this comment.

> +               if (start_flag) {
> +                       ret = fpga_mgr_write_init_buf(mgr, info, fw->data,
> +                                                     fw->size);

When the fpga manager is registered, one of the ops is
initial_header_size, which specifies how much buffer the write_init
needs.  So this call could fail if initial_header_size < streamsize.

I suggest that you make the streamsize equal to the smallest multiple
of PAGE_SIZE that is >= initial_header_size.

> +                       start_flag = 0;
> +                       if (ret)
> +                               break;
> +               }
> +
> +               /*
> +                * Write the FPGA image to the FPGA.
> +                */

Or this comment.

> +               mgr->state = FPGA_MGR_STATE_WRITE;
> +               ret = mgr->mops->write(mgr, fw->data, fw->size);
> +               if (ret) {
> +                       dev_err(dev, "Error while writing image data to FPGA\n");
> +                       mgr->state = FPGA_MGR_STATE_WRITE_ERR;
> +                       break;
> +               }
> +
> +               offset += fw->size;
> +               size -= fw->size;
> +               if (fw->size < streamsize)
> +                       break;
> +       }
> +
> +       ret = fpga_mgr_write_complete(mgr, info);
> +
> +       release_firmware(fw);
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(fpga_mgr_firmware_stream);
> +
>  int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info)
>  {
>         if (info->firmware_name)
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index 0f5072c..a25362e 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -151,6 +151,10 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr,
>                            struct fpga_image_info *info,
>                            const char *image_name);
>
> +int fpga_mgr_firmware_stream(struct fpga_manager *mgr,
> +                          struct fpga_image_info *info,
> +                          const char *image_name);

Don't need both fpga_mgr_firmware_load and fpga_mgr_firmware_stream,
so no need to change the header.

> +
>  int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info);
>
>  int fpga_mgr_lock(struct fpga_manager *mgr);
> --
> 2.7.4
>