| Message ID | 20171120182409.27348-11-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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...
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 --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; } }
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(-)