diff mbox

[v2,10/23] firmware: add helper to copy built-in data to pre-alloc buffer

Message ID 20171120182409.27348-11-mcgrof@kernel.org
State New, archived
Headers show

Commit Message

Luis Chamberlain Nov. 20, 2017, 6:23 p.m. UTC
This makes it clearer that the parameters passed are only used for
the preallocated buffer option, ie, when a caller uses:

	request_firmware_into_buf()

Otherwise this code won't run. We flip the logic just so the actual
prellocated buf code is not indented.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/base/firmware_class.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Greg Kroah-Hartman Nov. 29, 2017, 10:17 a.m. UTC | #1
On Mon, Nov 20, 2017 at 10:23:56AM -0800, Luis R. Rodriguez wrote:
> This makes it clearer that the parameters passed are only used for
> the preallocated buffer option, ie, when a caller uses:
> 
> 	request_firmware_into_buf()
> 
> Otherwise this code won't run. We flip the logic just so the actual
> prellocated buf code is not indented.
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
>  drivers/base/firmware_class.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 4f64410fe7e6..aba3f2cbe2f4 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -146,6 +146,14 @@ static struct firmware_cache fw_cache;
>  extern struct builtin_fw __start_builtin_fw[];
>  extern struct builtin_fw __end_builtin_fw[];
>  
> +static void fw_copy_to_prealloc_buf(struct firmware *fw,
> +				    void *buf, size_t size)
> +{
> +	if (!buf || size < fw->size)
> +		return;

Shouldn't this return an error?

> +	memcpy(buf, fw->data, fw->size);

I'll take this, but it feels really odd...
Luis Chamberlain Nov. 30, 2017, 8:18 p.m. UTC | #2
On Wed, Nov 29, 2017 at 11:17:15AM +0100, Greg KH wrote:
> On Mon, Nov 20, 2017 at 10:23:56AM -0800, Luis R. Rodriguez wrote:
> > This makes it clearer that the parameters passed are only used for
> > the preallocated buffer option, ie, when a caller uses:
> > 
> > 	request_firmware_into_buf()
> > 
> > Otherwise this code won't run. We flip the logic just so the actual
> > prellocated buf code is not indented.
> > 
> > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> > ---
> >  drivers/base/firmware_class.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index 4f64410fe7e6..aba3f2cbe2f4 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -146,6 +146,14 @@ static struct firmware_cache fw_cache;
> >  extern struct builtin_fw __start_builtin_fw[];
> >  extern struct builtin_fw __end_builtin_fw[];
> >  
> > +static void fw_copy_to_prealloc_buf(struct firmware *fw,
> > +				    void *buf, size_t size)
> > +{
> > +	if (!buf || size < fw->size)
> > +		return;
> 
> Shouldn't this return an error?

No, its a no-op when these are not set, and its actually *why* I created
the function. The parameters are *only* useful for the prealloc buf fw
calls. Keeping track of all this while reading the code is actually not
easy and this should make it clearer.

> 
> > +	memcpy(buf, fw->data, fw->size);
> 
> I'll take this, but it feels really odd...

Thanks.

  Luis
diff mbox

Patch

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 4f64410fe7e6..aba3f2cbe2f4 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -146,6 +146,14 @@  static struct firmware_cache fw_cache;
 extern struct builtin_fw __start_builtin_fw[];
 extern struct builtin_fw __end_builtin_fw[];
 
+static void fw_copy_to_prealloc_buf(struct firmware *fw,
+				    void *buf, size_t size)
+{
+	if (!buf || size < fw->size)
+		return;
+	memcpy(buf, fw->data, fw->size);
+}
+
 static bool fw_get_builtin_firmware(struct firmware *fw, const char *name,
 				    void *buf, size_t size)
 {
@@ -155,9 +163,8 @@  static bool fw_get_builtin_firmware(struct firmware *fw, const char *name,
 		if (strcmp(name, b_fw->name) == 0) {
 			fw->size = b_fw->size;
 			fw->data = b_fw->data;
+			fw_copy_to_prealloc_buf(fw, buf, size);
 
-			if (buf && fw->size <= size)
-				memcpy(buf, fw->data, fw->size);
 			return true;
 		}
 	}