Date:   Tue, 23 May 2017 10:21:42 -0500
From:   Alan Tull <atull@...nel.org>
To:     "Li, Yi" <yi1.li@...ux.intel.com>
Cc:     mcgrof@...nel.org, Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        wagi@...om.org, David Woodhouse <dwmw2@...radead.org>,
        rafal@...ecki.pl, arend.vanspriel@...adcom.com, rjw@...ysocki.net,
        Moritz Fischer <moritz.fischer@...us.com>, pmladek@...e.com,
        johannes.berg@...el.com, emmanuel.grumbach@...el.com,
        Luca Coelho <luciano.coelho@...el.com>, kvalo@...eaurora.org,
        luto@...nel.org, Takahiro Akashi <takahiro.akashi@...aro.org>,
        dhowells@...hat.com, pjones@...hat.com,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-fpga@...r.kernel.org
Subject: Re: [PATCHv2 3/3] fpga_mgr: Add streaming support through the new
 driver_data API

On Mon, May 22, 2017 at 11:11 PM, Li, Yi <yi1.li@...ux.intel.com> wrote:
> hi Alan
>
>
> On 5/22/2017 4:09 PM, Alan Tull wrote:
>>
>> On Sat, May 20, 2017 at 1:49 AM,  <yi1.li@...ux.intel.com> wrote:
>>
>> Hi Yi,
>>
>>> From: Yi Li <yi1.li@...ux.intel.com>
>>>
>>> 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@...ux.intel.com>
>>> ---
>>>   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
>>>
>

Powered by blists - more mailing lists