diff mbox series

[v5,2/7] firmware: add offset to request_firmware_into_buf

Message ID 20200508002739.19360-3-scott.branden@broadcom.com (mailing list archive)
State New, archived
Headers show
Series firmware: add partial read support in request_firmware_into_buf | expand

Commit Message

Scott Branden May 8, 2020, 12:27 a.m. UTC
Add offset to request_firmware_into_buf to allow for portions
of firmware file to be read into a buffer.  Necessary where firmware
needs to be loaded in portions from file in memory constrained systems.

Signed-off-by: Scott Branden <scott.branden@broadcom.com>
---
 drivers/base/firmware_loader/firmware.h |  5 +++
 drivers/base/firmware_loader/main.c     | 52 +++++++++++++++++--------
 drivers/soc/qcom/mdt_loader.c           |  7 +++-
 include/linux/firmware.h                |  8 +++-
 lib/test_firmware.c                     |  4 +-
 5 files changed, 55 insertions(+), 21 deletions(-)

Comments

Luis Chamberlain May 13, 2020, 12:33 a.m. UTC | #1
On Thu, May 07, 2020 at 05:27:34PM -0700, Scott Branden wrote:
> Add offset to request_firmware_into_buf to allow for portions
> of firmware file to be read into a buffer.  Necessary where firmware
> needs to be loaded in portions from file in memory constrained systems.
> 
> Signed-off-by: Scott Branden <scott.branden@broadcom.com>
> ---
>  drivers/base/firmware_loader/firmware.h |  5 +++
>  drivers/base/firmware_loader/main.c     | 52 +++++++++++++++++--------
>  drivers/soc/qcom/mdt_loader.c           |  7 +++-
>  include/linux/firmware.h                |  8 +++-
>  lib/test_firmware.c                     |  4 +-
>  5 files changed, 55 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h
> index 25836a6afc9f..1147dae01148 100644
> --- a/drivers/base/firmware_loader/firmware.h
> +++ b/drivers/base/firmware_loader/firmware.h
> @@ -32,6 +32,8 @@
>   * @FW_OPT_FALLBACK_PLATFORM: Enable fallback to device fw copy embedded in
>   *	the platform's main firmware. If both this fallback and the sysfs
>   *      fallback are enabled, then this fallback will be tried first.
> + * @FW_OPT_PARTIAL: Allow partial read of firmware instead of needing to read
> + *	entire file.

See, this allows us use kdoc to document his nicely. Do that with the
kernel pread stuff.

> @@ -68,6 +71,8 @@ struct fw_priv {
>  	void *data;
>  	size_t size;
>  	size_t allocated_size;
> +	size_t offset;
> +	unsigned int flags;

But flags is a misnomer, you just do two operations, just juse an enum
here to classify the read operation.

> index 76f79913916d..4552b7bb819f 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -167,7 +167,8 @@ static int fw_cache_piggyback_on_request(const char *name);
>  
>  static struct fw_priv *__allocate_fw_priv(const char *fw_name,
>  					  struct firmware_cache *fwc,
> -					  void *dbuf, size_t size)
> +					  void *dbuf, size_t size,
> +					  size_t offset, unsigned int flags)

And likewise just use an enum here too.

> @@ -210,9 +213,11 @@ static struct fw_priv *__lookup_fw_priv(const char *fw_name)
>  static int alloc_lookup_fw_priv(const char *fw_name,
>  				struct firmware_cache *fwc,
>  				struct fw_priv **fw_priv, void *dbuf,
> -				size_t size, enum fw_opt opt_flags)
> +				size_t size, enum fw_opt opt_flags,
> +				size_t offset)

flags? But its a single variable enum!

>  {
>  	struct fw_priv *tmp;
> +	unsigned int pread_flags;
>  
>  	spin_lock(&fwc->lock);
>  	if (!(opt_flags & FW_OPT_NOCACHE)) {
> @@ -226,7 +231,12 @@ static int alloc_lookup_fw_priv(const char *fw_name,
>  		}
>  	}
>  
> -	tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size);
> +	if (opt_flags & FW_OPT_PARTIAL)
> +		pread_flags = KERNEL_PREAD_FLAG_PART;
> +	else
> +		pread_flags = KERNEL_PREAD_FLAG_WHOLE;
> +
> +	tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size, offset, pread_flags);

One of the advantages of using an enum is that you can then use a switch
here, and the compiler will warn if you haven't handled all the cases.

>  		/* load firmware files from the mount namespace of init */
> -		rc = kernel_read_file_from_path_initns(path, &buffer,
> -						       &size, msize, id);
> +		rc = kernel_pread_file_from_path_initns(path, &buffer,
> +							&size, fw_priv->offset,
> +							msize,
> +							fw_priv->flags, id);

And here you'd just pass the kernel enum.

You get the idea, I stopped reviewing after this.

  Luis
Scott Branden May 13, 2020, 6:35 p.m. UTC | #2
Hi Luis,

Another question.

On 2020-05-12 5:33 p.m., Luis Chamberlain wrote:
> On Thu, May 07, 2020 at 05:27:34PM -0700, Scott Branden wrote:
>> Add offset to request_firmware_into_buf to allow for portions
>> of firmware file to be read into a buffer.  Necessary where firmware
>> needs to be loaded in portions from file in memory constrained systems.
>>
>> Signed-off-by: Scott Branden <scott.branden@broadcom.com>
>> ---
>>   drivers/base/firmware_loader/firmware.h |  5 +++
>>   drivers/base/firmware_loader/main.c     | 52 +++++++++++++++++--------
>>   drivers/soc/qcom/mdt_loader.c           |  7 +++-
>>   include/linux/firmware.h                |  8 +++-
>>   lib/test_firmware.c                     |  4 +-
>>   5 files changed, 55 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h
>> index 25836a6afc9f..1147dae01148 100644
>> --- a/drivers/base/firmware_loader/firmware.h
>> +++ b/drivers/base/firmware_loader/firmware.h
>> @@ -32,6 +32,8 @@
>>    * @FW_OPT_FALLBACK_PLATFORM: Enable fallback to device fw copy embedded in
>>    *	the platform's main firmware. If both this fallback and the sysfs
>>    *      fallback are enabled, then this fallback will be tried first.
>> + * @FW_OPT_PARTIAL: Allow partial read of firmware instead of needing to read
>> + *	entire file.
> See, this allows us use kdoc to document his nicely. Do that with the
> kernel pread stuff.
>
>> @@ -68,6 +71,8 @@ struct fw_priv {
>>   	void *data;
>>   	size_t size;
>>   	size_t allocated_size;
>> +	size_t offset;
>> +	unsigned int flags;
> But flags is a misnomer, you just do two operations, just juse an enum
> here to classify the read operation.
>
>> index 76f79913916d..4552b7bb819f 100644
>> --- a/drivers/base/firmware_loader/main.c
>> +++ b/drivers/base/firmware_loader/main.c
>> @@ -167,7 +167,8 @@ static int fw_cache_piggyback_on_request(const char *name);
>>   
>>   static struct fw_priv *__allocate_fw_priv(const char *fw_name,
>>   					  struct firmware_cache *fwc,
>> -					  void *dbuf, size_t size)
>> +					  void *dbuf, size_t size,
>> +					  size_t offset, unsigned int flags)
> And likewise just use an enum here too.
>
>> @@ -210,9 +213,11 @@ static struct fw_priv *__lookup_fw_priv(const char *fw_name)
>>   static int alloc_lookup_fw_priv(const char *fw_name,
>>   				struct firmware_cache *fwc,
>>   				struct fw_priv **fw_priv, void *dbuf,
>> -				size_t size, enum fw_opt opt_flags)
>> +				size_t size, enum fw_opt opt_flags,
>> +				size_t offset)
> flags? But its a single variable enum!
fw_opt is an existing enum which doesn't really act like an enum.
It is a series of BIT defines in an enum that are then OR'd together in 
the (existing) code?
>
>>   {
>>   	struct fw_priv *tmp;
>> +	unsigned int pread_flags;
>>   
>>   	spin_lock(&fwc->lock);
>>   	if (!(opt_flags & FW_OPT_NOCACHE)) {
Have a look here - enum used as series of flags.
>> @@ -226,7 +231,12 @@ static int alloc_lookup_fw_priv(const char *fw_name,
>>   		}
>>   	}
>>   
>> -	tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size);
>> +	if (opt_flags & FW_OPT_PARTIAL)
>> +		pread_flags = KERNEL_PREAD_FLAG_PART;
>> +	else
>> +		pread_flags = KERNEL_PREAD_FLAG_WHOLE;
>> +
>> +	tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size, offset, pread_flags);
> One of the advantages of using an enum is that you can then use a switch
> here, and the compiler will warn if you haven't handled all the cases.
pread_flags is currently such.  I will change to enum pread_opt.
>
>>   		/* load firmware files from the mount namespace of init */
>> -		rc = kernel_read_file_from_path_initns(path, &buffer,
>> -						       &size, msize, id);
>> +		rc = kernel_pread_file_from_path_initns(path, &buffer,
>> +							&size, fw_priv->offset,
>> +							msize,
>> +							fw_priv->flags, id);
> And here you'd just pass the kernel enum.
Yes, will change to fw_priv->opt and use enum for this one.
>
> You get the idea, I stopped reviewing after this.
>
>    Luis
Luis Chamberlain May 15, 2020, 8:44 p.m. UTC | #3
On Wed, May 13, 2020 at 11:35:06AM -0700, Scott Branden wrote:
> On 2020-05-12 5:33 p.m., Luis Chamberlain wrote:
> > On Thu, May 07, 2020 at 05:27:34PM -0700, Scott Branden wrote:
> > flags? But its a single variable enum!
> fw_opt is an existing enum which doesn't really act like an enum.
> It is a series of BIT defines in an enum that are then OR'd together in the
> (existing) code?

Indeed, in retrospect that is odd, it should be a u32 then. Please feel
free to fix.
 
  Luis
diff mbox series

Patch

diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h
index 25836a6afc9f..1147dae01148 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -32,6 +32,8 @@ 
  * @FW_OPT_FALLBACK_PLATFORM: Enable fallback to device fw copy embedded in
  *	the platform's main firmware. If both this fallback and the sysfs
  *      fallback are enabled, then this fallback will be tried first.
+ * @FW_OPT_PARTIAL: Allow partial read of firmware instead of needing to read
+ *	entire file.
  */
 enum fw_opt {
 	FW_OPT_UEVENT			= BIT(0),
@@ -41,6 +43,7 @@  enum fw_opt {
 	FW_OPT_NOCACHE			= BIT(4),
 	FW_OPT_NOFALLBACK_SYSFS		= BIT(5),
 	FW_OPT_FALLBACK_PLATFORM	= BIT(6),
+	FW_OPT_PARTIAL			= BIT(7),
 };
 
 enum fw_status {
@@ -68,6 +71,8 @@  struct fw_priv {
 	void *data;
 	size_t size;
 	size_t allocated_size;
+	size_t offset;
+	unsigned int flags;
 #ifdef CONFIG_FW_LOADER_PAGED_BUF
 	bool is_paged_buf;
 	struct page **pages;
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index 76f79913916d..4552b7bb819f 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -167,7 +167,8 @@  static int fw_cache_piggyback_on_request(const char *name);
 
 static struct fw_priv *__allocate_fw_priv(const char *fw_name,
 					  struct firmware_cache *fwc,
-					  void *dbuf, size_t size)
+					  void *dbuf, size_t size,
+					  size_t offset, unsigned int flags)
 {
 	struct fw_priv *fw_priv;
 
@@ -185,6 +186,8 @@  static struct fw_priv *__allocate_fw_priv(const char *fw_name,
 	fw_priv->fwc = fwc;
 	fw_priv->data = dbuf;
 	fw_priv->allocated_size = size;
+	fw_priv->offset = offset;
+	fw_priv->flags = flags;
 	fw_state_init(fw_priv);
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 	INIT_LIST_HEAD(&fw_priv->pending_list);
@@ -210,9 +213,11 @@  static struct fw_priv *__lookup_fw_priv(const char *fw_name)
 static int alloc_lookup_fw_priv(const char *fw_name,
 				struct firmware_cache *fwc,
 				struct fw_priv **fw_priv, void *dbuf,
-				size_t size, enum fw_opt opt_flags)
+				size_t size, enum fw_opt opt_flags,
+				size_t offset)
 {
 	struct fw_priv *tmp;
+	unsigned int pread_flags;
 
 	spin_lock(&fwc->lock);
 	if (!(opt_flags & FW_OPT_NOCACHE)) {
@@ -226,7 +231,12 @@  static int alloc_lookup_fw_priv(const char *fw_name,
 		}
 	}
 
-	tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size);
+	if (opt_flags & FW_OPT_PARTIAL)
+		pread_flags = KERNEL_PREAD_FLAG_PART;
+	else
+		pread_flags = KERNEL_PREAD_FLAG_WHOLE;
+
+	tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size, offset, pread_flags);
 	if (tmp) {
 		INIT_LIST_HEAD(&tmp->list);
 		if (!(opt_flags & FW_OPT_NOCACHE))
@@ -495,8 +505,10 @@  fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
 		fw_priv->size = 0;
 
 		/* load firmware files from the mount namespace of init */
-		rc = kernel_read_file_from_path_initns(path, &buffer,
-						       &size, msize, id);
+		rc = kernel_pread_file_from_path_initns(path, &buffer,
+							&size, fw_priv->offset,
+							msize,
+							fw_priv->flags, id);
 		if (rc) {
 			if (rc != -ENOENT)
 				dev_warn(device, "loading %s failed with error %d\n",
@@ -687,7 +699,7 @@  int assign_fw(struct firmware *fw, struct device *device,
 static int
 _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 			  struct device *device, void *dbuf, size_t size,
-			  enum fw_opt opt_flags)
+			  enum fw_opt opt_flags, size_t offset)
 {
 	struct firmware *firmware;
 	struct fw_priv *fw_priv;
@@ -706,7 +718,7 @@  _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 	}
 
 	ret = alloc_lookup_fw_priv(name, &fw_cache, &fw_priv, dbuf, size,
-				  opt_flags);
+				   opt_flags, offset);
 
 	/*
 	 * bind with 'priv' now to avoid warning in failure path
@@ -753,7 +765,7 @@  static void fw_abort_batch_reqs(struct firmware *fw)
 static int
 _request_firmware(const struct firmware **firmware_p, const char *name,
 		  struct device *device, void *buf, size_t size,
-		  enum fw_opt opt_flags)
+		  enum fw_opt opt_flags, size_t offset)
 {
 	struct firmware *fw = NULL;
 	int ret;
@@ -767,7 +779,7 @@  _request_firmware(const struct firmware **firmware_p, const char *name,
 	}
 
 	ret = _request_firmware_prepare(&fw, name, device, buf, size,
-					opt_flags);
+					opt_flags, offset);
 	if (ret <= 0) /* error or already assigned */
 		goto out;
 
@@ -830,7 +842,7 @@  request_firmware(const struct firmware **firmware_p, const char *name,
 	/* Need to pin this module until return */
 	__module_get(THIS_MODULE);
 	ret = _request_firmware(firmware_p, name, device, NULL, 0,
-				FW_OPT_UEVENT);
+				FW_OPT_UEVENT, 0);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -857,7 +869,7 @@  int firmware_request_nowarn(const struct firmware **firmware, const char *name,
 	/* Need to pin this module until return */
 	__module_get(THIS_MODULE);
 	ret = _request_firmware(firmware, name, device, NULL, 0,
-				FW_OPT_UEVENT | FW_OPT_NO_WARN);
+				FW_OPT_UEVENT | FW_OPT_NO_WARN, 0);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -882,7 +894,7 @@  int request_firmware_direct(const struct firmware **firmware_p,
 	__module_get(THIS_MODULE);
 	ret = _request_firmware(firmware_p, name, device, NULL, 0,
 				FW_OPT_UEVENT | FW_OPT_NO_WARN |
-				FW_OPT_NOFALLBACK_SYSFS);
+				FW_OPT_NOFALLBACK_SYSFS, 0);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -906,7 +918,7 @@  int firmware_request_platform(const struct firmware **firmware,
 	/* Need to pin this module until return */
 	__module_get(THIS_MODULE);
 	ret = _request_firmware(firmware, name, device, NULL, 0,
-				FW_OPT_UEVENT | FW_OPT_FALLBACK_PLATFORM);
+				FW_OPT_UEVENT | FW_OPT_FALLBACK_PLATFORM, 0);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -943,6 +955,8 @@  EXPORT_SYMBOL_GPL(firmware_request_cache);
  * @device: device for which firmware is being loaded and DMA region allocated
  * @buf: address of buffer to load firmware into
  * @size: size of buffer
+ * @offset: offset into file to read
+ * @pread_flags: KERNEL_PREAD_FLAG_PART to allow partial file read
  *
  * This function works pretty much like request_firmware(), but it doesn't
  * allocate a buffer to hold the firmware data. Instead, the firmware
@@ -953,16 +967,22 @@  EXPORT_SYMBOL_GPL(firmware_request_cache);
  */
 int
 request_firmware_into_buf(const struct firmware **firmware_p, const char *name,
-			  struct device *device, void *buf, size_t size)
+			  struct device *device, void *buf, size_t size,
+			  size_t offset, unsigned int pread_flags)
 {
 	int ret;
+	enum fw_opt opt_flags;
 
 	if (fw_cache_is_setup(device, name))
 		return -EOPNOTSUPP;
 
 	__module_get(THIS_MODULE);
+	opt_flags = FW_OPT_UEVENT | FW_OPT_NOCACHE;
+	if (pread_flags & KERNEL_PREAD_FLAG_PART)
+		opt_flags |= FW_OPT_PARTIAL;
+
 	ret = _request_firmware(firmware_p, name, device, buf, size,
-				FW_OPT_UEVENT | FW_OPT_NOCACHE);
+				opt_flags, offset);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -1001,7 +1021,7 @@  static void request_firmware_work_func(struct work_struct *work)
 	fw_work = container_of(work, struct firmware_work, work);
 
 	_request_firmware(&fw, fw_work->name, fw_work->device, NULL, 0,
-			  fw_work->opt_flags);
+			  fw_work->opt_flags, 0);
 	fw_work->cont(fw, fw_work->context);
 	put_device(fw_work->device); /* taken in request_firmware_nowait() */
 
diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
index 24cd193dec55..00f3359f4f61 100644
--- a/drivers/soc/qcom/mdt_loader.c
+++ b/drivers/soc/qcom/mdt_loader.c
@@ -246,8 +246,11 @@  static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
 		} else if (phdr->p_filesz) {
 			/* Firmware not large enough, load split-out segments */
 			sprintf(fw_name + fw_name_len - 3, "b%02d", i);
-			ret = request_firmware_into_buf(&seg_fw, fw_name, dev,
-							ptr, phdr->p_filesz);
+			ret = request_firmware_into_buf
+						(&seg_fw, fw_name, dev,
+						 ptr, phdr->p_filesz,
+						 0,
+						 KERNEL_PREAD_FLAG_WHOLE);
 			if (ret) {
 				dev_err(dev, "failed to load %s\n", fw_name);
 				break;
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 4bbd0afd91b7..b98d17fb3bd1 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -4,6 +4,7 @@ 
 
 #include <linux/types.h>
 #include <linux/compiler.h>
+#include <linux/fs.h>
 #include <linux/gfp.h>
 
 #define FW_ACTION_NOHOTPLUG 0
@@ -53,7 +54,9 @@  int request_firmware_nowait(
 int request_firmware_direct(const struct firmware **fw, const char *name,
 			    struct device *device);
 int request_firmware_into_buf(const struct firmware **firmware_p,
-	const char *name, struct device *device, void *buf, size_t size);
+			      const char *name, struct device *device,
+			      void *buf, size_t size,
+			      size_t offset, unsigned int pread_flags);
 
 void release_firmware(const struct firmware *fw);
 #else
@@ -98,7 +101,8 @@  static inline int request_firmware_direct(const struct firmware **fw,
 }
 
 static inline int request_firmware_into_buf(const struct firmware **firmware_p,
-	const char *name, struct device *device, void *buf, size_t size)
+	const char *name, struct device *device, void *buf, size_t size,
+	size_t offset, unsigned int pread_flags)
 {
 	return -EINVAL;
 }
diff --git a/lib/test_firmware.c b/lib/test_firmware.c
index 9fee2b93a8d1..55176b0a8318 100644
--- a/lib/test_firmware.c
+++ b/lib/test_firmware.c
@@ -654,7 +654,9 @@  static int test_fw_run_batch_request(void *data)
 						    req->name,
 						    req->dev,
 						    test_buf,
-						    TEST_FIRMWARE_BUF_SIZE);
+						    TEST_FIRMWARE_BUF_SIZE,
+						    0,
+						    KERNEL_PREAD_FLAG_WHOLE);
 		if (!req->fw)
 			kfree(test_buf);
 	} else {