Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1604915 > unrolled thread

Re: [RFC 1/2] firmware class: Add stream_firmware API.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC 1/2] firmware class: Add stream_firmware API. Alan Tull <delicious.quinoa@gmail.com> - 2017-03-20 19:10 +0100
    Re: [RFC 1/2] firmware class: Add stream_firmware API. Alan Tull <delicious.quinoa@gmail.com> - 2017-03-20 19:40 +0100
      Re: [RFC 1/2] firmware class: Add stream_firmware API. "Li, Yi" <yi1.li@linux.intel.com> - 2017-03-22 23:10 +0100
        Re: [RFC 1/2] firmware class: Add stream_firmware API. Alan Tull <delicious.quinoa@gmail.com> - 2017-03-23 01:40 +0100

#1604915 — Re: [RFC 1/2] firmware class: Add stream_firmware API.

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

Hi Yi,

As FPGA image sizes are increasing, this change can be really helpful.
I have one comment below.

Alan Tull

> From: Yi Li <yi1.li@linux.intel.com>
>
> Add function to load firmware in multiple chucks instead of
>
> loading the whole big firmware file at once.
>
> Signed-off-by: Yi Li <yi1.li@linux.intel.com>
> ---
>  drivers/base/firmware_class.c | 128 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/firmware.h      |   2 +
>  2 files changed, 130 insertions(+)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index ac350c5..44fddff 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -436,6 +436,62 @@ fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
>         return rc;
>  }
>
> +static int
> +fw_stream_filesystem_firmware(struct device *device, struct firmware_buf *buf,
> +                       size_t offset, size_t length)
> +{
> +       int i, len;
> +       char *path;
> +       int rc = 0;
> +       struct file *file;
> +
> +       buf->size = 0;
> +
> +       path = __getname();
> +       if (!path)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
> +               /* skip the unset customized path */
> +               if (!fw_path[i][0])
> +                       continue;
> +
> +               len = snprintf(path, PATH_MAX, "%s/%s",
> +                              fw_path[i], buf->fw_id);
> +               if (len >= PATH_MAX) {
> +                       rc = -ENAMETOOLONG;
> +                       break;
> +               }
> +
> +               if (!path || !*path)
> +                       continue;
> +
> +               if (!buf->data) {
> +                       buf->data = vmalloc(length);
> +                       if (!buf->data) {
> +                               rc = -ENOMEM;
> +                               break;
> +                       }
> +               }

Your implementation is pretty straightforward.  My one complaint is
that this searches for the file on the firmware path each time we need
to get the next chunk of the firmware.  Could you change this to find
the file in the firmware path once and save that path somewhere?
Possibly added to struct firmware_buf since that's the priv.

> +
> +               file = filp_open(path, O_RDONLY, 0);
> +               if (IS_ERR(file))
> +                       continue;
> +
> +               buf->size = kernel_read(file, offset, (char *) buf->data,
> +                                       length);
> +               fput(file);
> +               break;
> +       }
> +
> +       __putname(path);
> +
> +       if (rc)
> +               dev_err(device, "loading %s failed with error %d\n",
> +                        path, rc);
> +       return rc;
> +}
> +
>  /* firmware holds the ownership of pages */
>  static void firmware_free_data(const struct firmware *fw)
>  {
> @@ -1267,6 +1323,78 @@ request_firmware(const struct firmware **firmware_p, const char *name,
>  }
>  EXPORT_SYMBOL(request_firmware);
>
> +static int
> +_stream_firmware(const struct firmware **firmware_p, const char *name,
> +                 struct device *device, void *buf, size_t size,
> +                 unsigned int opt_flags, size_t offset, size_t length)
> +{
> +       int ret;
> +       struct firmware *fw = NULL;
> +       struct firmware_buf *fbuf;
> +
> +       if ((!firmware_p) || (!name || name[0] == '\0')) {
> +               dev_err(device, "invalid firmware pointer or file name\n");
> +               return -EINVAL;
> +       }
> +
> +       if (!*firmware_p) {
> +               ret = _request_firmware_prepare(&fw, name, device, buf, size);
> +               if (ret <= 0) {
> +                       dev_err(device, "%s: _request_firmware_prepare failed %d\n",
> +                               __func__, ret);
> +               }
> +       } else {
> +               fw = (struct firmware *) *firmware_p;
> +       }
> +
> +       fbuf = (struct firmware_buf *) fw->priv;
> +       ret = fw_stream_filesystem_firmware(device, fbuf, offset, length);
> +       fw->size = fbuf->size;
> +       fw->data = fbuf->data;
> +       *firmware_p = fw;
> +
> +       if (ret)
> +               dev_err(device, "streaming with error %d\n", ret);
> +       return ret;
> +}
> +
> +/**
> + * stream_firmware: - send firmware request and wait for it
> + * @firmware_p: pointer to firmware image
> + * @name: name of firmware file
> + * @device: device for which firmware is being loaded
> + * @offset: offset of the file to read from
> + * @length: length in bytes to read
> + *
> + *      @firmware_p will be used to return a firmware image by the name
> + *      of @name for device @device.
> + *
> + *      Should be called from user context where sleeping is allowed.
> + *
> + *      @name will be used as $FIRMWARE in the uevent environment and
> + *      should be distinctive enough not to be confused with any other
> + *      firmware image for this or any other device.
> + *
> + *     Caller must hold the reference count of @device.
> + *
> + *     The function can be called safely inside device's suspend and
> + *     resume callback.
> + **/
> +int
> +stream_firmware(const struct firmware **firmware_p, const char *name,
> +                struct device *device, size_t offset, size_t length)
> +{
> +       size_t ret;
> +
> +       /* Need to pin this module until return */
> +       __module_get(THIS_MODULE);
> +       ret = _stream_firmware(firmware_p, name, device, NULL, 0,
> +                               FW_OPT_UEVENT | FW_OPT_NO_WARN, offset, length);
> +       module_put(THIS_MODULE);
> +       return ret;
> +}
> +EXPORT_SYMBOL(stream_firmware);
> +
>  /**
>   * request_firmware_direct: - load firmware directly without usermode helper
>   * @firmware_p: pointer to firmware image
> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index b1f9f0c..accd7f6 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -41,6 +41,8 @@ struct builtin_fw {
>  #if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && defined(MODULE))
>  int request_firmware(const struct firmware **fw, const char *name,
>                      struct device *device);
> +int stream_firmware(const struct firmware **fw, const char *name,
> +                   struct device *device, size_t offset, size_t length);
>  int request_firmware_nowait(
>         struct module *module, bool uevent,
>         const char *name, struct device *device, gfp_t gfp, void *context,
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [next] | [standalone]

#1605047

On Mon, Mar 20, 2017 at 1:00 PM, Alan Tull <delicious.quinoa@gmail.com> wrote:

>> +int
>> +stream_firmware(const struct firmware **firmware_p, const char *name,
>> +                struct device *device, size_t offset, size_t length)
>> +{
>> +       size_t ret;
>> +
>> +       /* Need to pin this module until return */
>> +       __module_get(THIS_MODULE);
>> +       ret = _stream_firmware(firmware_p, name, device, NULL, 0,
>> +                               FW_OPT_UEVENT | FW_OPT_NO_WARN, offset, length);

IIUC, here you are setting size == 0 and buf == NULL  to prevent
_request_firmware_prepare from attempting to load from built in
firmware.

So three of the parameters buf, size, and opt_flags are fixed and
don't need to be passed to _stream_firmware().

Alternatively, I wonder how hard it would be to code this so that the
streaming interface will fall back and successfully get the built in
or cached firmware if it exists and stream it out in PAGE_SIZE chunks.

Alan Tull

[toc] | [prev] | [next] | [standalone]

#1607057

Alan


On 3/20/2017 1:34 PM, Alan Tull wrote:
> On Mon, Mar 20, 2017 at 1:00 PM, Alan Tull <delicious.quinoa@gmail.com> wrote:
>
>>> +int
>>> +stream_firmware(const struct firmware **firmware_p, const char *name,
>>> +                struct device *device, size_t offset, size_t length)
>>> +{
>>> +       size_t ret;
>>> +
>>> +       /* Need to pin this module until return */
>>> +       __module_get(THIS_MODULE);
>>> +       ret = _stream_firmware(firmware_p, name, device, NULL, 0,
>>> +                               FW_OPT_UEVENT | FW_OPT_NO_WARN, offset, length);
> IIUC, here you are setting size == 0 and buf == NULL  to prevent
> _request_firmware_prepare from attempting to load from built in
> firmware.
>
> So three of the parameters buf, size, and opt_flags are fixed and
> don't need to be passed to _stream_firmware().

Sure.

> Alternatively, I wonder how hard it would be to code this so that the
> streaming interface will fall back and successfully get the built in
> or cached firmware if it exists and stream it out in PAGE_SIZE chunks.

That's an interesting idea, I will try it out and submit patch for 
review later. On another hand, if the kernel already cache the whole 
firmware image, why should we use streaming instead of regular 
request_firmware?

>
> Alan Tull
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1607098

On Wed, Mar 22, 2017 at 5:05 PM, Li, Yi <yi1.li@linux.intel.com> wrote:
> Alan
>
>
> On 3/20/2017 1:34 PM, Alan Tull wrote:
>>
>> On Mon, Mar 20, 2017 at 1:00 PM, Alan Tull <delicious.quinoa@gmail.com>
>> wrote:
>>
>>>> +int
>>>> +stream_firmware(const struct firmware **firmware_p, const char *name,
>>>> +                struct device *device, size_t offset, size_t length)
>>>> +{
>>>> +       size_t ret;
>>>> +
>>>> +       /* Need to pin this module until return */
>>>> +       __module_get(THIS_MODULE);
>>>> +       ret = _stream_firmware(firmware_p, name, device, NULL, 0,
>>>> +                               FW_OPT_UEVENT | FW_OPT_NO_WARN, offset,
>>>> length);
>>
>> IIUC, here you are setting size == 0 and buf == NULL  to prevent
>> _request_firmware_prepare from attempting to load from built in
>> firmware.
>>
>> So three of the parameters buf, size, and opt_flags are fixed and
>> don't need to be passed to _stream_firmware().
>
>
> Sure.
>
>> Alternatively, I wonder how hard it would be to code this so that the
>> streaming interface will fall back and successfully get the built in
>> or cached firmware if it exists and stream it out in PAGE_SIZE chunks.
>
>
> That's an interesting idea, I will try it out and submit patch for review
> later. On another hand, if the kernel already cache the whole firmware
> image, why should we use streaming instead of regular request_firmware?

The caller doesn't know whether the firmware is cached or not.  The
caller just wants the firmware if it exists, wherever it is.  If that can be
made automatic then the caller doesn't have to first attempt stream_firmware
and then if that fails fall back to calling request_firmware.

>
>>
>> Alan Tull
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web