| 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: <8a6cd982-cb8f-41df-f108-ca07b6fe48a5@linux.intel.com>
Date: Mon, 13 Mar 2017 14:04:06 -0500
From: "Li, Yi" <yi1.li@...ux.intel.com>
To: Alan Tull <delicious.quinoa@...il.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
Thanks Alan for the comments.
On 3/13/2017 1:00 PM, Alan Tull wrote:
> 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.
Sure, will do.
>
>> +{
>> + 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.
Agree, we can fix the streamsize to something bigger than
initial_header_size for the bitstream file. Another reason to have the
variable streamsize is for the performance, although with Altera Cyclone
V FPGA CvP I did not see much difference when increasing the buffer
size, since the bottleneck is with programming through PCIe and the time
spent with firmware file reading is negligible.
>
>> + 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
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists