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

Commit Message

Li, Yi March 10, 2017, 12:18 a.m. UTC

From: Yi Li <[e-mail]>

Add function to load firmware in multiple chucks instead of

loading the whole big firmware file at once.

Signed-off-by: Yi Li <[e-mail]>
---
 drivers/base/firmware_class.c | 128 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/firmware.h      |   2 +
 2 files changed, 130 insertions(+)

Comments

Matthew Gerlach March 10, 2017, 5:44 p.m. UTC | #1

On Thu, 9 Mar 2017, [e-mail] wrote:

> From: Yi Li <[e-mail]>


Hi Yi,

Just one question below.

Matthew Gerlach


> Add function to load firmware in multiple chucks instead of
>
> loading the whole big firmware file at once.
>
> Signed-off-by: Yi Li <[e-mail]>
> ---
> 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);

I'm probably being paranoid, but is it safe to assume the length of the 
buffer returned by __getname() is at least PATH_MAX?  It seems like
the length should be pagesize.

> +		if (len >= PATH_MAX) {
> +			rc = -ENAMETOOLONG;
> +			break;
> +		}
> +
> +		if (!path || !*path)
> +			continue;
> +
> +		if (!buf->data) {
> +			buf->data = vmalloc(length);
> +			if (!buf->data) {
> +				rc = -ENOMEM;
> +				break;
> +			}
> +		}
> +
> +		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 [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Li, Yi March 10, 2017, 7:25 p.m. UTC | #2

Hi Matthew


On 3/10/2017 11:44 AM, [e-mail] wrote:
>
>
> On Thu, 9 Mar 2017, [e-mail] wrote:
>
>> From: Yi Li <[e-mail]>
>
>
> Hi Yi,
>
> Just one question below.
>
> Matthew Gerlach
>
>
>> Add function to load firmware in multiple chucks instead of
>>
>> loading the whole big firmware file at once.
>>
>> Signed-off-by: Yi Li <[e-mail]>
>> ---
>> 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);
>
> I'm probably being paranoid, but is it safe to assume the length of 
> the buffer returned by __getname() is at least PATH_MAX?  It seems like
> the length should be pagesize.

The size should be the maximum number of char of the string be produced, 
not the input size.
According to 
https://www.gnu.org/software/libc/manual/html_node/Formatted-Output-Functions.html
Function:/int/*snprintf*/(char *s, size_tsize, const char *template, …)
/The|snprintf|function is similar to|sprintf|, except that 
thesizeargument specifies the maximum number of characters to produce. 
The trailing null character is counted towards this limit, so you should 
allocate at leastsizecharacters for the strings. Ifsizeis zero, nothing, 
not even the null byte, shall be written andsmay be a null pointer.
The return value is the number of characters which would be generated 
for the given input, excluding the trailing null. If this value is 
greater than or equal tosize, not all characters from the result have 
been stored ins

>
>> +        if (len >= PATH_MAX) {
>> +            rc = -ENAMETOOLONG;
>> +            break;
>> +        }
>> +
>> +        if (!path || !*path)
>> +            continue;
>> +
>> +        if (!buf->data) {
>> +            buf->data = vmalloc(length);
>> +            if (!buf->data) {
>> +                rc = -ENOMEM;
>> +                break;
>> +            }
>> +        }
>> +
>> +        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 [e-mail]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Matthew Gerlach March 13, 2017, 9:09 p.m. UTC | #3

On Fri, 10 Mar 2017, Li, Yi wrote:

> Hi Matthew
>
Hi Yi,


>
> On 3/10/2017 11:44 AM, [e-mail] wrote:
>> 
>> 
>> On Thu, 9 Mar 2017, [e-mail] wrote:
>> 
>>> From: Yi Li <[e-mail]>
>> 
>> 
>> Hi Yi,
>> 
>> Just one question below.
>> 
>> Matthew Gerlach
>> 
>> 
>>> Add function to load firmware in multiple chucks instead of
>>> 
>>> loading the whole big firmware file at once.
>>> 
>>> Signed-off-by: Yi Li <[e-mail]>
>>> ---
>>> 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);
>> 
>> I'm probably being paranoid, but is it safe to assume the length of the 
>> buffer returned by __getname() is at least PATH_MAX?  It seems like
>> the length should be pagesize.
>
> The size should be the maximum number of char of the string be produced, not 
> the input size.
> According to 
> https://www.gnu.org/software/libc/manual/html_node/Formatted-Output-Functions.html
> Function:/int/*snprintf*/(char *s, size_tsize, const char *template, …)
> /The|snprintf|function is similar to|sprintf|, except that thesizeargument 
> specifies the maximum number of characters to produce. The trailing null 
> character is counted towards this limit, so you should allocate at 
> leastsizecharacters for the strings. Ifsizeis zero, nothing, not even the 
> null byte, shall be written andsmay be a null pointer.
> The return value is the number of characters which would be generated for the 
> given input, excluding the trailing null. If this value is greater than or 
> equal tosize, not all characters from the result have been stored ins
>

I am familiar with the functionality of snprintf versus sprintf.  In the
snprintf call above, you are saying that memory pointed to by the variable 
path, has at least PATH_MAX number of bytes.  My question is how can you 
know that the memory returned by __getname() has PATH_MAX number of bytes?


>> 
>>> +        if (len >= PATH_MAX) {
>>> +            rc = -ENAMETOOLONG;
>>> +            break;
>>> +        }
>>> +
>>> +        if (!path || !*path)
>>> +            continue;
>>> +
>>> +        if (!buf->data) {
>>> +            buf->data = vmalloc(length);
>>> +            if (!buf->data) {
>>> +                rc = -ENOMEM;
>>> +                break;
>>> +            }
>>> +        }
>>> +
>>> +        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 [e-mail]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> 
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Li, Yi March 14, 2017, 4:10 p.m. UTC | #4

hi Matthew,


On 3/13/2017 4:09 PM, [e-mail] wrote:
>
>
> On Fri, 10 Mar 2017, Li, Yi wrote:
>
>> Hi Matthew
>>
> Hi Yi,
>
>
>>
>> On 3/10/2017 11:44 AM, [e-mail] wrote:
>>>
>>>
>>> On Thu, 9 Mar 2017, [e-mail] wrote:
>>>
>>>> From: Yi Li <[e-mail]>
>>>
>>>
>>> Hi Yi,
>>>
>>> Just one question below.
>>>
>>> Matthew Gerlach
>>>
>>>
>>>> Add function to load firmware in multiple chucks instead of
>>>>
>>>> loading the whole big firmware file at once.
>>>>
>>>> Signed-off-by: Yi Li <[e-mail]>
>>>> ---
>>>> 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);
>>>
>>> I'm probably being paranoid, but is it safe to assume the length of 
>>> the buffer returned by __getname() is at least PATH_MAX?  It seems like
>>> the length should be pagesize.
>>
>> The size should be the maximum number of char of the string be 
>> produced, not the input size.
>> According to 
>> https://www.gnu.org/software/libc/manual/html_node/Formatted-Output-Functions.html
>> Function:/int/*snprintf*/(char *s, size_tsize, const char *template, …)
>> /The|snprintf|function is similar to|sprintf|, except that 
>> thesizeargument specifies the maximum number of characters to 
>> produce. The trailing null character is counted towards this limit, 
>> so you should allocate at leastsizecharacters for the strings. 
>> Ifsizeis zero, nothing, not even the null byte, shall be written 
>> andsmay be a null pointer.
>> The return value is the number of characters which would be generated 
>> for the given input, excluding the trailing null. If this value is 
>> greater than or equal tosize, not all characters from the result have 
>> been stored ins
>>
>
> I am familiar with the functionality of snprintf versus sprintf. In the
> snprintf call above, you are saying that memory pointed to by the 
> variable path, has at least PATH_MAX number of bytes.  My question is 
> how can you know that the memory returned by __getname() has PATH_MAX 
> number of bytes?
>
>

Ah, now I understand the concerns.

The __getname() will allocate an buffer object from names_cachep
extern struct kmem_cache 
<http://lxr.free-electrons.com/ident?i=kmem_cache> *names_cachep 
<http://lxr.free-electrons.com/ident?i=names_cachep>;
#define __getname <http://lxr.free-electrons.com/ident?i=__getname>() 
kmem_cache_alloc 
<http://lxr.free-electrons.com/ident?i=kmem_cache_alloc>(names_cachep 
<http://lxr.free-electrons.com/ident?i=names_cachep>, GFP_KERNEL 
<http://lxr.free-electrons.com/ident?i=GFP_KERNEL>)

names_cachep is created in fs/dcaches.c vfs_caches_init function with 
object size equal to PATH_MAX.
names_cachep <http://lxr.free-electrons.com/ident?i=names_cachep> = 
kmem_cache_create 
<http://lxr.free-electrons.com/ident?i=kmem_cache_create>(/"names_cache"/, 
PATH_MAX <http://lxr.free-electrons.com/ident?i=PATH_MAX>, 
0,SLAB_HWCACHE_ALIGN 
<http://lxr.free-electrons.com/ident?i=SLAB_HWCACHE_ALIGN>|SLAB_PANIC 
<http://lxr.free-electrons.com/ident?i=SLAB_PANIC>, NULL 
<http://lxr.free-electrons.com/ident?i=NULL>);

so __getname() should allocate buffer with size of PATH_MAX.

The code is borrowed from fw_get_filesystem_firmware function, which 
should be reviewed and safe to use. :)

Thanks,
Yi


>>>
>>>> +        if (len >= PATH_MAX) {
>>>> +            rc = -ENAMETOOLONG;
>>>> +            break;
>>>> +        }
>>>> +
>>>> +        if (!path || !*path)
>>>> +            continue;
>>>> +
>>>> +        if (!buf->data) {
>>>> +            buf->data = vmalloc(length);
>>>> +            if (!buf->data) {
>>>> +                rc = -ENOMEM;
>>>> +                break;
>>>> +            }
>>>> +        }
>>>> +
>>>> +        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 [e-mail]
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>
>>
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
>> the body of a message to [e-mail]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Matthew Gerlach March 14, 2017, 4:55 p.m. UTC | #5

On Tue, 14 Mar 2017, Li, Yi wrote:

> hi Matthew,
>
Hi Yi,

>
> On 3/13/2017 4:09 PM, [e-mail] wrote:
>> 
>> 
>> On Fri, 10 Mar 2017, Li, Yi wrote:
>> 
>>> Hi Matthew
>>> 
>> Hi Yi,
>> 
>> 
>>> 
>>> On 3/10/2017 11:44 AM, [e-mail] wrote:
>>>> 
>>>> 
>>>> On Thu, 9 Mar 2017, [e-mail] wrote:
>>>> 
>>>>> From: Yi Li <[e-mail]>
>>>> 
>>>> 
>>>> Hi Yi,
>>>> 
>>>> Just one question below.
>>>> 
>>>> Matthew Gerlach
>>>> 
>>>> 
>>>>> Add function to load firmware in multiple chucks instead of
>>>>> 
>>>>> loading the whole big firmware file at once.
>>>>> 
>>>>> Signed-off-by: Yi Li <[e-mail]>
>>>>> ---
>>>>> 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);
>>>> 
>>>> I'm probably being paranoid, but is it safe to assume the length of the 
>>>> buffer returned by __getname() is at least PATH_MAX?  It seems like
>>>> the length should be pagesize.
>>> 
>>> The size should be the maximum number of char of the string be produced, 
>>> not the input size.
>>> According to 
>>> https://www.gnu.org/software/libc/manual/html_node/Formatted-Output-Functions.html
>>> Function:/int/*snprintf*/(char *s, size_tsize, const char *template, …)
>>> /The|snprintf|function is similar to|sprintf|, except that thesizeargument 
>>> specifies the maximum number of characters to produce. The trailing null 
>>> character is counted towards this limit, so you should allocate at 
>>> leastsizecharacters for the strings. Ifsizeis zero, nothing, not even the 
>>> null byte, shall be written andsmay be a null pointer.
>>> The return value is the number of characters which would be generated for 
>>> the given input, excluding the trailing null. If this value is greater 
>>> than or equal tosize, not all characters from the result have been stored 
>>> ins
>>> 
>> 
>> I am familiar with the functionality of snprintf versus sprintf. In the
>> snprintf call above, you are saying that memory pointed to by the variable 
>> path, has at least PATH_MAX number of bytes.  My question is how can you 
>> know that the memory returned by __getname() has PATH_MAX number of bytes?
>> 
>> 
>
> Ah, now I understand the concerns.
>
> The __getname() will allocate an buffer object from names_cachep
> extern struct kmem_cache <http://lxr.free-electrons.com/ident?i=kmem_cache> 
> *names_cachep <http://lxr.free-electrons.com/ident?i=names_cachep>;
> #define __getname <http://lxr.free-electrons.com/ident?i=__getname>() 
> kmem_cache_alloc 
> <http://lxr.free-electrons.com/ident?i=kmem_cache_alloc>(names_cachep 
> <http://lxr.free-electrons.com/ident?i=names_cachep>, GFP_KERNEL 
> <http://lxr.free-electrons.com/ident?i=GFP_KERNEL>)
>
> names_cachep is created in fs/dcaches.c vfs_caches_init function with object 
> size equal to PATH_MAX.
> names_cachep <http://lxr.free-electrons.com/ident?i=names_cachep> = 
> kmem_cache_create 
> <http://lxr.free-electrons.com/ident?i=kmem_cache_create>(/"names_cache"/, 
> PATH_MAX <http://lxr.free-electrons.com/ident?i=PATH_MAX>, 
> 0,SLAB_HWCACHE_ALIGN 
> <http://lxr.free-electrons.com/ident?i=SLAB_HWCACHE_ALIGN>|SLAB_PANIC 
> <http://lxr.free-electrons.com/ident?i=SLAB_PANIC>, NULL 
> <http://lxr.free-electrons.com/ident?i=NULL>);
>
> so __getname() should allocate buffer with size of PATH_MAX.
>
> The code is borrowed from fw_get_filesystem_firmware function, which should 
> be reviewed and safe to use. :)

Thanks for following up. I am no longer paranoid.

Matthew Gerlach
>
> Thanks,
> Yi
>
>
>>>> 
>>>>> +        if (len >= PATH_MAX) {
>>>>> +            rc = -ENAMETOOLONG;
>>>>> +            break;
>>>>> +        }
>>>>> +
>>>>> +        if (!path || !*path)
>>>>> +            continue;
>>>>> +
>>>>> +        if (!buf->data) {
>>>>> +            buf->data = vmalloc(length);
>>>>> +            if (!buf->data) {
>>>>> +                rc = -ENOMEM;
>>>>> +                break;
>>>>> +            }
>>>>> +        }
>>>>> +
>>>>> +        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 [e-mail]
>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>> 
>>> 
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
>>> the body of a message to [e-mail]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> 
>
>

Alan Tull March 20, 2017, 6 p.m. UTC | #6

On Thu, Mar 9, 2017 at 6:18 PM,  <[e-mail]> 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 <[e-mail]>
>
> Add function to load firmware in multiple chucks instead of
>
> loading the whole big firmware file at once.
>
> Signed-off-by: Yi Li <[e-mail]>
> ---
>  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 [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alan Tull March 20, 2017, 6:34 p.m. UTC | #7

On Mon, Mar 20, 2017 at 1:00 PM, Alan Tull <[e-mail]> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Li, Yi March 22, 2017, 10:05 p.m. UTC | #8

Alan


On 3/20/2017 1:34 PM, Alan Tull wrote:
> On Mon, Mar 20, 2017 at 1:00 PM, Alan Tull <[e-mail]> 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 [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alan Tull March 23, 2017, 12:34 a.m. UTC | #9

On Wed, Mar 22, 2017 at 5:05 PM, Li, Yi <[e-mail]> wrote:
> Alan
>
>
> On 3/20/2017 1:34 PM, Alan Tull wrote:
>>
>> On Mon, Mar 20, 2017 at 1:00 PM, Alan Tull <[e-mail]>
>> 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 [e-mail]
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Luis R. Rodriguez March 27, 2017, 7:36 p.m. UTC | #10

On Thu, Mar 09, 2017 at 06:18:09PM -0600, [e-mail] wrote:
> From: Yi Li <[e-mail]>
> 
> Add function to load firmware in multiple chucks instead of
> 
> loading the whole big firmware file at once.
> 
> Signed-off-by: Yi Li <[e-mail]>
> ---
>  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;
> +			}
> +		}
> +
> +		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;
> +}

Yet another API call to read files form the fs seems rather odd, are you sure
nothing can be done to re-purpose the existing call ?

> +
> +EXPORT_SYMBOL(stream_firmware);

New functionality should be EXPORT_SYMBOL_GPL().

> 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);

Have you looked at the rather new request_firmware_into_buf() ? I hated that
as it did not get any proper code *review* but its there now... If we can
leverage off of it to give us something useful for actual upstream drivers
rather than cruft outside of the kernel I'd be a bit happier.

Also, please note that I had been noting we keep extending the firmware API
with loose APIs, I've been consolidating a bit of this into a newer API which
provides a flexible API for us. Since this is not upstream I don't expect
you to work off of that, but I will Cc you on some updated patches which
will fold in this work, I am expecting that the API we will ultimately use for
this feature you are preoposing could be folded there.

So for now, please consider the review notes above, and we can later see how
we fold this into a new set of APIs which I hope to bake this week.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Li, Yi March 27, 2017, 9:20 p.m. UTC | #11

hi Luis,


On 3/27/2017 2:36 PM, Luis R. Rodriguez wrote:
> On Thu, Mar 09, 2017 at 06:18:09PM -0600, [e-mail] wrote:
>> From: Yi Li <[e-mail]>
>>
>> Add function to load firmware in multiple chucks instead of
>>
>> loading the whole big firmware file at once.
>>
>> Signed-off-by: Yi Li <[e-mail]>
>> ---
>>   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;
>> +			}
>> +		}
>> +
>> +		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;
>> +}
> Yet another API call to read files form the fs seems rather odd, are you sure
> nothing can be done to re-purpose the existing call ?

Agree, I also hated to duplicate the read files from fs, what we are 
missing in existing API is an "offset", which will enable loading the 
firmware in pieces.

>
>> +
>> +EXPORT_SYMBOL(stream_firmware);
> New functionality should be EXPORT_SYMBOL_GPL().

Got it.

>
>> 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);
> Have you looked at the rather new request_firmware_into_buf() ? I hated that
> as it did not get any proper code *review* but its there now... If we can
> leverage off of it to give us something useful for actual upstream drivers
> rather than cruft outside of the kernel I'd be a bit happier.

Yes, I looked request_firmware_into_buf API, it still load the whole 
firmware image into a big memory buffer (but allocated outside of 
firmware class).

>
> Also, please note that I had been noting we keep extending the firmware API
> with loose APIs, I've been consolidating a bit of this into a newer API which
> provides a flexible API for us. Since this is not upstream I don't expect
> you to work off of that, but I will Cc you on some updated patches which
> will fold in this work, I am expecting that the API we will ultimately use for
> this feature you are preoposing could be folded there.
>
> So for now, please consider the review notes above, and we can later see how
> we fold this into a new set of APIs which I hope to bake this week.

Cool, copy me on the updated patches. I am also working on the patch 
based on Alan Tull's comments. Pardon me if I am slow this 2 weeks due 
to family emergency.

>
>    Luis
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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;
+			}
+		}
+
+		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,