Re: [PATCHv2 3/3] fpga_mgr: Add streaming support through the new driver_data API

From: Alan Tull
Date: Tue May 23 2017 - 11:22:30 EST

On Mon, May 22, 2017 at 11:11 PM, Li, Yi <yi1.li@xxxxxxxxxxxxxxx> wrote:
> hi Alan
>
>
> On 5/22/2017 4:09 PM, Alan Tull wrote:
>>
>> On Sat, May 20, 2017 at 1:49 AM, <yi1.li@xxxxxxxxxxxxxxx> wrote:
>>
>> Hi Yi,
>>
>>> From: Yi Li <yi1.li@xxxxxxxxxxxxxxx>
>>>
>>> Since the FPGA image are getting bigger in size, this add an new API
>>> fpga_mgr_firmware_stream
>>
>> You could replace the guts of the current fpga_mgr_firmware_load()
>> with this new API (keep the old name). Unless anyone can think of a
>> reason to keep my old memory hog version. Assuming this all works :)
>>
>>> in FPGA manager, which will stream FPGA image in
>>> 4KB trunks.
>>
>> chunks?
>
> Oops.
>>
>>
>>> Signed-off-by: Yi Li <yi1.li@xxxxxxxxxxxxxxx>
>>> ---
>>> drivers/fpga/fpga-mgr.c | 111
>>> ++++++++++++++++++++++++++++++++++++++++++
>>> include/linux/fpga/fpga-mgr.h | 4 ++
>>> 2 files changed, 115 insertions(+)
>>>
>>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
>>> index 188ffef..420ee38 100644
>>> --- a/drivers/fpga/fpga-mgr.c
>>> +++ b/drivers/fpga/fpga-mgr.c
>>> @@ -27,6 +27,8 @@
>>> #include <linux/slab.h>
>>> #include <linux/scatterlist.h>
>>> #include <linux/highmem.h>
>>> +#include <linux/sizes.h>
>>> +#include <linux/driver_data.h>
>>>
>>> static DEFINE_IDA(fpga_mgr_ida);
>>> static struct class *fpga_mgr_class;
>>> @@ -196,6 +198,115 @@ static int fpga_mgr_buf_load_mapped(struct
>>> fpga_manager *mgr,
>>> return fpga_mgr_write_complete(mgr, info);
>>> }
>>>
>>> +struct fpga_mgr_streaming_priv_params {
>>
>> This name is boggling my mind. :-) Something can be private or it can
>> be params, but it seems like params aren't really private. And later
>> it's referred to as 'params' although it's really private data. Could
>> you rename and take off the _params? That way when I'm reading
>> through this, I'll know that this is something that this code is
>> really passing to itself and I won't mistakenly think that this is a
>> struct that is being passed for the firmware code to consume.
>
>
> Okay, will rename the structure to fpga_mgr_streaming_context?

That sounds good.

>>
>>
>>> + struct fpga_image_info *info;
>>> + struct fpga_manager *mgr;
>>> + loff_t offset;
>>> + loff_t fw_size;
>>> +};
>>> +
>>
>> Please add a comment here explaining that this is the callback
>> function, called back from the firmware code for each 4K chunk of FPGA
>> image.
>
>
> Sure, will do.
>>>
>>> +static int fpga_mgr_streaming_fw_cb(void *context, const struct firmware
>>> *fw,
>>> + int err)
>>> +{
>>> + int ret = -EINVAL;
>>> + struct fpga_mgr_streaming_priv_params *params =
>>
>> And please call this 'priv' or something with 'priv' in the name?
>
>
> Okay, struct fpga_mgr_streaming_context *streaming_context?

That's fine, especially since the firmware layer is calling it "context".

>>
>>
>>> + (struct fpga_mgr_streaming_priv_params *)context;
>>> + struct fpga_image_info *info = params->info;
>>> + struct fpga_manager *mgr = params->mgr;
>>> + struct device *dev = &mgr->dev;
>>> +
>>> + params->fw_size = fw->size;
>>
>> Please skip a line here.
>
> Thanks for catching that.
>>
>>
>>> + /*
>>> + * init.
>>> + */
>>
>> /* A one line comment should be like this, not 3 lines. */
>
> Thanks for catching that.
>>
>>
>>> + if (params->offset == 0) {
>>> + ret = fpga_mgr_write_init_buf(mgr, info, fw->data,
>>> fw->size);
>>> + if (ret)
>>> + return ret;
>>> + }
>>> +
>>> + /*
>>> + * Write the FPGA image to the FPGA.
>>> + */
>>
>> /* Write the FPGA image to the FPGA. */
>>
>>> + 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;
>>> + return ret;
>>> + }
>>> +
>>> + if (fw->size < SZ_4K)
>>
>> Define a macro before the function and use it everywhere SZ_4K means
>> the same thing here (3 occurrences), such as
>>
>> #define FW_CHUNK_SZ SZ_4K
>
> Okay.
>>
>>
>>> + ret = fpga_mgr_write_complete(mgr, info);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +/**
>>> + * fpga_mgr_firmware_stream - streaming firmware and load to fpga
>>> + * @mgr: fpga manager
>>> + * @info: fpga image specific information
>>> + * @image_name: name of image file on the firmware search path
>>> + *
>>> + * Streaming 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)
>>> +{
>>> + int ret;
>>> + char *path = NULL;
>>> + void *buf;
>>> + size_t length = INT_MAX;
>>
>> I guess the firmware layer doesn't give us the size of the whole
>> firmware, so length is used for the while() below, right? Too bad
>> since everybody using streaming firmware is going to be writing this
>> same type of while statement.
>
> Yes, with the non-streaming new driver_data_request_xx case, it will give
> the firmware structure with the size of image length. But with the
> streaming case, we will not know the size of the image file until it reach
> to the end of the file.
>
>>
>>> + struct device *dev = &mgr->dev;
>>> + struct fpga_mgr_streaming_priv_params params = {
>>
>> priv
>>
>>> + .info = info,
>>> + .mgr = mgr,
>>> + .fw_size = 0,
>>> + .offset = 0,
>>> + };
>>> +
>>> + const struct driver_data_req_params req_params = {
>>> + DRIVER_DATA_DEFAULT_SYNC(fpga_mgr_streaming_fw_cb,
>>> &params),
>>> + .reqs = DRIVER_DATA_REQ_NO_CACHE |
>>> DRIVER_DATA_REQ_STREAMING,
>>> + .alloc_buf_size = SZ_4K,
>>
>> FW_CHUNK_SZ or whatever name is appropriate.
>>
>>> + .alloc_buf = &buf,
>>> + .img_offset = &params.offset,
>>> + .path = &path,
>>> + };
>>> +
>>> + buf = kmalloc(SZ_4K, GFP_KERNEL);
>>> + if (!buf) {
>>> + dev_err(dev, "%s: kmalloc buf failed\n", __func__);
>>
>> This message isn't needed.
>
> Got it
>>
>>
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ;
>>> + while (length > 0) {
>>
>> This could be "do { ... } while ((params.fw_size >= FIRMWARE_CHUNK_SZ)
>> && (length > 0));" since that's what it's really doing.
>
> Yes, this is better, thanks.
>
>>
>>> + ret = driver_data_request_sync(image_name, &req_params,
>>> dev);
>>> + if (ret) {
>>> + dev_err(dev, "Error reading firmware %d\n", ret);
>>> + mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR;

Also need: kfree(buf);

>>> + return ret;
>>> + }
>>> +
>>> + length -= params.fw_size;
>>> + params.offset += params.fw_size;
>>> + if (params.fw_size < SZ_4K)
>>> + break;
>>> + }
>>> +
>>> + kfree(buf);
>>
>> Please skip a line before return.
>>
>>> + return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(fpga_mgr_firmware_stream);
>>
>> Thanks for working on this!
>>
>> Alan
>>
>>> +
>>> /**
>>> * fpga_mgr_buf_load - load fpga from image in buffer
>>> * @mgr: fpga manager
>>> diff --git a/include/linux/fpga/fpga-mgr.h
>>> b/include/linux/fpga/fpga-mgr.h
>>> index b4ac24c..083e091 100644
>>> --- a/include/linux/fpga/fpga-mgr.h
>>> +++ b/include/linux/fpga/fpga-mgr.h
>>> @@ -143,6 +143,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);
>>> +
>>> struct fpga_manager *of_fpga_mgr_get(struct device_node *node);
>>>
>>> struct fpga_manager *fpga_mgr_get(struct device *dev);
>>> --
>>> 2.7.4
>>>
>