Subject: Re: [RFC 1/2] firmware class: Add stream_firmware API.
To: "Luis R. Rodriguez" <mcgrof@kernel.org>
References: <1489105090-4996-1-git-send-email-yi1.li@linux.intel.com>
 <1489105090-4996-2-git-send-email-yi1.li@linux.intel.com>
 <20170327193638.GN28800@wotan.suse.de>
Cc: ming.lei@canonical.com, gregkh@linuxfoundation.org,
        atull@opensource.altera.com, moritz.fischer@ettus.com,
        linux-kernel@vger.kernel.org, linux-fpga@vger.kernel.org
From: "Li, Yi" <yi1.li@linux.intel.com>
Message-ID: <e9438625-22a8-a438-c187-986b29be4297@linux.intel.com>
Date: Mon, 27 Mar 2017 16:20:13 -0500
User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <20170327193638.GN28800@wotan.suse.de>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4386
Lines: 135

hi Luis,


On 3/27/2017 2:36 PM, Luis R. Rodriguez wrote:
> On Thu, Mar 09, 2017 at 06:18:09PM -0600, yi1.li@linux.intel.com wrote:
>> 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;
>> +			}
>> +		}
>> +
>> +		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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html