| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CANk1AXSki1s2aKn9Xk2yVa-V6oHJQqYBbs4XbVHiYiwkzNbxcg@mail.gmail.com>
Date: Mon, 13 Mar 2017 13:00:49 -0500
From: Alan Tull <delicious.quinoa@...il.com>
To: "Li, Yi" <yi1.li@...ux.intel.com>
Cc: ming.lei@...onical.com, mcgrof@...nel.org,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
atull <atull@...nsource.altera.com>,
Moritz Fischer <moritz.fischer@...us.com>,
linux-kernel <linux-kernel@...r.kernel.org>,
linux-fpga@...r.kernel.org
Subject: Re: [RFC 2/2] fpga manager: Add fpga_mgr_firmware_stream API
On Thu, Mar 9, 2017 at 6:18 PM, <yi1.li@...ux.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@...ux.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@...ux.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
>
Powered by blists - more mailing lists