Good afternoon,
This is the second installment in this series, the first one can be
found here[1]. The goal of the work is to consolidate modifications to
function rproc_alloc() that were made over the last weeks[2][3][4] to
provide a common foundation to work from and avoid merge conflicts.
Applies cleanly on v5.7-rc1
Thanks,
Mathieu
New for V2:
- Reworked title for patch 01.
- Added "Fixes" tag to patch 01.
- Using kasprintf() instead of complex memory allocation.
- Using kstrdup_const() instead of kstrdup().
- Reworked rproc_alloc_firmware() to use non-negative form.
[1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=270239
[2]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=261069
[3]. https://patchwork.kernel.org/patch/11456385/
[4]. https://patchwork.kernel.org/patch/11473241/
Alex Elder (1):
remoteproc: Fix IDR initialisation in rproc_alloc()
Mathieu Poirier (6):
remoteproc: Split firmware name allocation from rproc_alloc()
remoteproc: Simplify default name allocation
remoteproc: Use kstrdup_const() rather than kstrup()
remoteproc: Restructure firmware name allocation
remoteproc: Split rproc_ops allocation from rproc_alloc()
remoteproc: Get rid of tedious error path
drivers/remoteproc/remoteproc_core.c | 96 +++++++++++++++-------------
include/linux/remoteproc.h | 2 +-
2 files changed, 54 insertions(+), 44 deletions(-)
--
2.20.1
For cases where @firmware is declared "const char *", use function
kstrdup_const() to avoid needlessly creating another copy on the
heap.
Suggested-by: Bjorn Andersson <[email protected]>
Signed-off-by: Mathieu Poirier <[email protected]>
---
drivers/remoteproc/remoteproc_core.c | 4 ++--
include/linux/remoteproc.h | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 9899467fa1cf..ebaff496ef81 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1982,7 +1982,7 @@ static const struct device_type rproc_type = {
static int rproc_alloc_firmware(struct rproc *rproc,
const char *name, const char *firmware)
{
- char *p;
+ const char *p;
if (!firmware)
/*
@@ -1991,7 +1991,7 @@ static int rproc_alloc_firmware(struct rproc *rproc,
*/
p = kasprintf(GFP_KERNEL, "rproc-%s-fw", name);
else
- p = kstrdup(firmware, GFP_KERNEL);
+ p = kstrdup_const(firmware, GFP_KERNEL);
if (!p)
return -ENOMEM;
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 9c07d7958c53..38607107b7cb 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -489,7 +489,7 @@ struct rproc {
struct list_head node;
struct iommu_domain *domain;
const char *name;
- char *firmware;
+ const char *firmware;
void *priv;
struct rproc_ops *ops;
struct device dev;
--
2.20.1
Make the rproc_ops allocation a function on its own in an effort
to clean up function rproc_alloc().
Signed-off-by: Mathieu Poirier <[email protected]>
Reviewed-by: Alex Elder <[email protected]>
---
drivers/remoteproc/remoteproc_core.c | 32 +++++++++++++++++-----------
1 file changed, 20 insertions(+), 12 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 0bfa6998705d..a5a0ceb86b3f 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2001,6 +2001,25 @@ static int rproc_alloc_firmware(struct rproc *rproc,
return 0;
}
+static int rproc_alloc_ops(struct rproc *rproc, const struct rproc_ops *ops)
+{
+ rproc->ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL);
+ if (!rproc->ops)
+ return -ENOMEM;
+
+ /* Default to ELF loader if no load function is specified */
+ if (!rproc->ops->load) {
+ rproc->ops->load = rproc_elf_load_segments;
+ rproc->ops->parse_fw = rproc_elf_load_rsc_table;
+ rproc->ops->find_loaded_rsc_table =
+ rproc_elf_find_loaded_rsc_table;
+ rproc->ops->sanity_check = rproc_elf_sanity_check;
+ rproc->ops->get_boot_addr = rproc_elf_get_boot_addr;
+ }
+
+ return 0;
+}
+
/**
* rproc_alloc() - allocate a remote processor handle
* @dev: the underlying device
@@ -2040,8 +2059,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
if (rproc_alloc_firmware(rproc, name, firmware))
goto free_rproc;
- rproc->ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL);
- if (!rproc->ops)
+ if (rproc_alloc_ops(rproc, ops))
goto free_firmware;
rproc->name = name;
@@ -2068,16 +2086,6 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
atomic_set(&rproc->power, 0);
- /* Default to ELF loader if no load function is specified */
- if (!rproc->ops->load) {
- rproc->ops->load = rproc_elf_load_segments;
- rproc->ops->parse_fw = rproc_elf_load_rsc_table;
- rproc->ops->find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table;
- if (!rproc->ops->sanity_check)
- rproc->ops->sanity_check = rproc_elf32_sanity_check;
- rproc->ops->get_boot_addr = rproc_elf_get_boot_addr;
- }
-
mutex_init(&rproc->lock);
INIT_LIST_HEAD(&rproc->carveouts);
--
2.20.1
Improve the readability of function rproc_alloc_firmware() by using
a non-negated condition.
Suggested-by: Alex Elder <[email protected]>
Signed-off-by: Mathieu Poirier <[email protected]>
---
drivers/remoteproc/remoteproc_core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index ebaff496ef81..0bfa6998705d 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1984,14 +1984,14 @@ static int rproc_alloc_firmware(struct rproc *rproc,
{
const char *p;
- if (!firmware)
+ if (firmware)
+ p = kstrdup_const(firmware, GFP_KERNEL);
+ else
/*
* If the caller didn't pass in a firmware name then
* construct a default name.
*/
p = kasprintf(GFP_KERNEL, "rproc-%s-fw", name);
- else
- p = kstrdup_const(firmware, GFP_KERNEL);
if (!p)
return -ENOMEM;
--
2.20.1
Make the firmware name allocation a function on its own in an
effort to cleanup function rproc_alloc().
Signed-off-by: Mathieu Poirier <[email protected]>
---
drivers/remoteproc/remoteproc_core.c | 66 ++++++++++++++++------------
1 file changed, 39 insertions(+), 27 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 80056513ae71..4dee63f319ba 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1979,6 +1979,33 @@ static const struct device_type rproc_type = {
.release = rproc_type_release,
};
+static int rproc_alloc_firmware(struct rproc *rproc,
+ const char *name, const char *firmware)
+{
+ char *p, *template = "rproc-%s-fw";
+ int name_len;
+
+ if (!firmware) {
+ /*
+ * If the caller didn't pass in a firmware name then
+ * construct a default name.
+ */
+ name_len = strlen(name) + strlen(template) - 2 + 1;
+ p = kmalloc(name_len, GFP_KERNEL);
+ if (!p)
+ return -ENOMEM;
+ snprintf(p, name_len, template, name);
+ } else {
+ p = kstrdup(firmware, GFP_KERNEL);
+ if (!p)
+ return -ENOMEM;
+ }
+
+ rproc->firmware = p;
+
+ return 0;
+}
+
/**
* rproc_alloc() - allocate a remote processor handle
* @dev: the underlying device
@@ -2007,42 +2034,21 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
const char *firmware, int len)
{
struct rproc *rproc;
- char *p, *template = "rproc-%s-fw";
- int name_len;
if (!dev || !name || !ops)
return NULL;
- if (!firmware) {
- /*
- * If the caller didn't pass in a firmware name then
- * construct a default name.
- */
- name_len = strlen(name) + strlen(template) - 2 + 1;
- p = kmalloc(name_len, GFP_KERNEL);
- if (!p)
- return NULL;
- snprintf(p, name_len, template, name);
- } else {
- p = kstrdup(firmware, GFP_KERNEL);
- if (!p)
- return NULL;
- }
-
rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL);
- if (!rproc) {
- kfree(p);
+ if (!rproc)
return NULL;
- }
+
+ if (rproc_alloc_firmware(rproc, name, firmware))
+ goto free_rproc;
rproc->ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL);
- if (!rproc->ops) {
- kfree(p);
- kfree(rproc);
- return NULL;
- }
+ if (!rproc->ops)
+ goto free_firmware;
- rproc->firmware = p;
rproc->name = name;
rproc->priv = &rproc[1];
rproc->auto_boot = true;
@@ -2091,6 +2097,12 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
rproc->state = RPROC_OFFLINE;
return rproc;
+
+free_firmware:
+ kfree(rproc->firmware);
+free_rproc:
+ kfree(rproc);
+ return NULL;
}
EXPORT_SYMBOL(rproc_alloc);
--
2.20.1
On 4/15/20 3:48 PM, Mathieu Poirier wrote:
> Improve the readability of function rproc_alloc_firmware() by using
> a non-negated condition.
>
> Suggested-by: Alex Elder <[email protected]>
> Signed-off-by: Mathieu Poirier <[email protected]>
If it were me, I'd move the comment above the if statement and
perhaps reword it a little bit to describe what's happening.
But no matter, this looks good.
Reviewed-by: Alex Elder <[email protected]>
> ---
> drivers/remoteproc/remoteproc_core.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index ebaff496ef81..0bfa6998705d 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1984,14 +1984,14 @@ static int rproc_alloc_firmware(struct rproc *rproc,
> {
> const char *p;
>
> - if (!firmware)
> + if (firmware)
> + p = kstrdup_const(firmware, GFP_KERNEL);
> + else
> /*
> * If the caller didn't pass in a firmware name then
> * construct a default name.
> */
> p = kasprintf(GFP_KERNEL, "rproc-%s-fw", name);
> - else
> - p = kstrdup_const(firmware, GFP_KERNEL);
>
> if (!p)
> return -ENOMEM;
>
On 4/15/20 3:48 PM, Mathieu Poirier wrote:
> For cases where @firmware is declared "const char *", use function
> kstrdup_const() to avoid needlessly creating another copy on the
> heap.
>
> Suggested-by: Bjorn Andersson <[email protected]>
> Signed-off-by: Mathieu Poirier <[email protected]>
Looks good.
Reviewed-by: Alex Elder <[email protected]>
> ---
> drivers/remoteproc/remoteproc_core.c | 4 ++--
> include/linux/remoteproc.h | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 9899467fa1cf..ebaff496ef81 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1982,7 +1982,7 @@ static const struct device_type rproc_type = {
> static int rproc_alloc_firmware(struct rproc *rproc,
> const char *name, const char *firmware)
> {
> - char *p;
> + const char *p;
>
> if (!firmware)
> /*
> @@ -1991,7 +1991,7 @@ static int rproc_alloc_firmware(struct rproc *rproc,
> */
> p = kasprintf(GFP_KERNEL, "rproc-%s-fw", name);
> else
> - p = kstrdup(firmware, GFP_KERNEL);
> + p = kstrdup_const(firmware, GFP_KERNEL);
>
> if (!p)
> return -ENOMEM;
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 9c07d7958c53..38607107b7cb 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -489,7 +489,7 @@ struct rproc {
> struct list_head node;
> struct iommu_domain *domain;
> const char *name;
> - char *firmware;
> + const char *firmware;
> void *priv;
> struct rproc_ops *ops;
> struct device dev;
>
On 4/15/20 3:48 PM, Mathieu Poirier wrote:
> Make the firmware name allocation a function on its own in an
> effort to cleanup function rproc_alloc().
>
> Signed-off-by: Mathieu Poirier <[email protected]>
Looks good.
Reviewed-by: Alex Elder <[email protected]>
> ---
> drivers/remoteproc/remoteproc_core.c | 66 ++++++++++++++++------------
> 1 file changed, 39 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 80056513ae71..4dee63f319ba 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1979,6 +1979,33 @@ static const struct device_type rproc_type = {
> .release = rproc_type_release,
> };
>
> +static int rproc_alloc_firmware(struct rproc *rproc,
> + const char *name, const char *firmware)
> +{
> + char *p, *template = "rproc-%s-fw";
> + int name_len;
> +
> + if (!firmware) {
> + /*
> + * If the caller didn't pass in a firmware name then
> + * construct a default name.
> + */
> + name_len = strlen(name) + strlen(template) - 2 + 1;
> + p = kmalloc(name_len, GFP_KERNEL);
> + if (!p)
> + return -ENOMEM;
> + snprintf(p, name_len, template, name);
> + } else {
> + p = kstrdup(firmware, GFP_KERNEL);
> + if (!p)
> + return -ENOMEM;
> + }
> +
> + rproc->firmware = p;
> +
> + return 0;
> +}
> +
> /**
> * rproc_alloc() - allocate a remote processor handle
> * @dev: the underlying device
> @@ -2007,42 +2034,21 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
> const char *firmware, int len)
> {
> struct rproc *rproc;
> - char *p, *template = "rproc-%s-fw";
> - int name_len;
>
> if (!dev || !name || !ops)
> return NULL;
>
> - if (!firmware) {
> - /*
> - * If the caller didn't pass in a firmware name then
> - * construct a default name.
> - */
> - name_len = strlen(name) + strlen(template) - 2 + 1;
> - p = kmalloc(name_len, GFP_KERNEL);
> - if (!p)
> - return NULL;
> - snprintf(p, name_len, template, name);
> - } else {
> - p = kstrdup(firmware, GFP_KERNEL);
> - if (!p)
> - return NULL;
> - }
> -
> rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL);
> - if (!rproc) {
> - kfree(p);
> + if (!rproc)
> return NULL;
> - }
> +
> + if (rproc_alloc_firmware(rproc, name, firmware))
> + goto free_rproc;
>
> rproc->ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL);
> - if (!rproc->ops) {
> - kfree(p);
> - kfree(rproc);
> - return NULL;
> - }
> + if (!rproc->ops)
> + goto free_firmware;
>
> - rproc->firmware = p;
> rproc->name = name;
> rproc->priv = &rproc[1];
> rproc->auto_boot = true;
> @@ -2091,6 +2097,12 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
> rproc->state = RPROC_OFFLINE;
>
> return rproc;
> +
> +free_firmware:
> + kfree(rproc->firmware);
> +free_rproc:
> + kfree(rproc);
> + return NULL;
> }
> EXPORT_SYMBOL(rproc_alloc);
>
>
…
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1984,14 +1984,14 @@ static int rproc_alloc_firmware(struct rproc *rproc,
> {
> const char *p;
>
> - if (!firmware)
> + if (firmware)
> + p = kstrdup_const(firmware, GFP_KERNEL);
> + else
> /*
> * If the caller didn't pass in a firmware name then
> * construct a default name.
> */
> p = kasprintf(GFP_KERNEL, "rproc-%s-fw", name);
> - else
> - p = kstrdup_const(firmware, GFP_KERNEL);
Can the use of the conditional operator make sense at such source code places?
p = firmware ? kstrdup_const(…) : kasprintf(…);
Regards,
Markus
Hi Mathieu,
On 4/15/20 3:48 PM, Mathieu Poirier wrote:
> Good afternoon,
>
> This is the second installment in this series, the first one can be
> found here[1]. The goal of the work is to consolidate modifications to
> function rproc_alloc() that were made over the last weeks[2][3][4] to
> provide a common foundation to work from and avoid merge conflicts.
>
> Applies cleanly on v5.7-rc1
Thanks for the patches. Overall looks good. I have couple of minor
comments, will post them in the respective patches.
>
> Thanks,
> Mathieu
>
> New for V2:
> - Reworked title for patch 01.
> - Added "Fixes" tag to patch 01.
> - Using kasprintf() instead of complex memory allocation.
> - Using kstrdup_const() instead of kstrdup().
> - Reworked rproc_alloc_firmware() to use non-negative form.
>
> [1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=270239
> [2]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=261069
> [3]. https://patchwork.kernel.org/patch/11456385/
I have since reworked this and posted the next version on top of this
series.
https://patchwork.kernel.org/patch/11493941/
regards
Suman
> [4]. https://patchwork.kernel.org/patch/11473241/
>
> Alex Elder (1):
> remoteproc: Fix IDR initialisation in rproc_alloc()
>
> Mathieu Poirier (6):
> remoteproc: Split firmware name allocation from rproc_alloc()
> remoteproc: Simplify default name allocation
> remoteproc: Use kstrdup_const() rather than kstrup()
> remoteproc: Restructure firmware name allocation
> remoteproc: Split rproc_ops allocation from rproc_alloc()
> remoteproc: Get rid of tedious error path
>
> drivers/remoteproc/remoteproc_core.c | 96 +++++++++++++++-------------
> include/linux/remoteproc.h | 2 +-
> 2 files changed, 54 insertions(+), 44 deletions(-)
>
Hi Markus,
On 4/16/20 1:26 AM, Markus Elfring wrote:
> …
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1984,14 +1984,14 @@ static int rproc_alloc_firmware(struct rproc *rproc,
>> {
>> const char *p;
>>
>> - if (!firmware)
>> + if (firmware)
>> + p = kstrdup_const(firmware, GFP_KERNEL);
>> + else
>> /*
>> * If the caller didn't pass in a firmware name then
>> * construct a default name.
>> */
>> p = kasprintf(GFP_KERNEL, "rproc-%s-fw", name);
>> - else
>> - p = kstrdup_const(firmware, GFP_KERNEL);
>
> Can the use of the conditional operator make sense at such source code places?
>
> p = firmware ? kstrdup_const(…) : kasprintf(…);
For simple assignments, I too prefer the ternary operator, but in this
case, I think it is better to leave the current code as is.
regards
Suman
On 4/15/20 4:25 PM, Alex Elder wrote:
> On 4/15/20 3:48 PM, Mathieu Poirier wrote:
>> For cases where @firmware is declared "const char *", use function
>> kstrdup_const() to avoid needlessly creating another copy on the
>> heap.
>>
>> Suggested-by: Bjorn Andersson <[email protected]>
>> Signed-off-by: Mathieu Poirier <[email protected]>
>
> Looks good.
>
> Reviewed-by: Alex Elder <[email protected]>
>
>> ---
>> drivers/remoteproc/remoteproc_core.c | 4 ++--
>> include/linux/remoteproc.h | 2 +-
>> 2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index 9899467fa1cf..ebaff496ef81 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1982,7 +1982,7 @@ static const struct device_type rproc_type = {
>> static int rproc_alloc_firmware(struct rproc *rproc,
>> const char *name, const char *firmware)
>> {
>> - char *p;
>> + const char *p;
>>
>> if (!firmware)
>> /*
>> @@ -1991,7 +1991,7 @@ static int rproc_alloc_firmware(struct rproc *rproc,
>> */
>> p = kasprintf(GFP_KERNEL, "rproc-%s-fw", name);
So, to be consistent for both paths, should we be using
kvasprintf_const() here and kfree_const() in release. The kfree_const()
is needed to account for the kstrdup_const below for sure.
regards
Suman
>> else
>> - p = kstrdup(firmware, GFP_KERNEL);
>> + p = kstrdup_const(firmware, GFP_KERNEL);
>>
>> if (!p)
>> return -ENOMEM;
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index 9c07d7958c53..38607107b7cb 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -489,7 +489,7 @@ struct rproc {
>> struct list_head node;
>> struct iommu_domain *domain;
>> const char *name;
>> - char *firmware;
>> + const char *firmware;
>> void *priv;
>> struct rproc_ops *ops;
>> struct device dev;
>>
>
On 4/15/20 3:48 PM, Mathieu Poirier wrote:
> Make the rproc_ops allocation a function on its own in an effort
> to clean up function rproc_alloc().
>
> Signed-off-by: Mathieu Poirier <[email protected]>
> Reviewed-by: Alex Elder <[email protected]>
> ---
> drivers/remoteproc/remoteproc_core.c | 32 +++++++++++++++++-----------
> 1 file changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 0bfa6998705d..a5a0ceb86b3f 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2001,6 +2001,25 @@ static int rproc_alloc_firmware(struct rproc *rproc,
> return 0;
> }
>
> +static int rproc_alloc_ops(struct rproc *rproc, const struct rproc_ops *ops)
> +{
> + rproc->ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL);
> + if (!rproc->ops)
> + return -ENOMEM;
> +
> + /* Default to ELF loader if no load function is specified */
> + if (!rproc->ops->load) {
> + rproc->ops->load = rproc_elf_load_segments;
> + rproc->ops->parse_fw = rproc_elf_load_rsc_table;
> + rproc->ops->find_loaded_rsc_table =
> + rproc_elf_find_loaded_rsc_table;
> + rproc->ops->sanity_check = rproc_elf_sanity_check;
So, the conditional check on sanity check is dropped and the callback
switched here without the changelog reflecting anything why. You should
just rebase this patch on top of Clement's patch [1] that removes the
conditional flag, and also usage from the remoteproc platform drivers.
regards
Suman
[1] https://patchwork.kernel.org/patch/11462013/
> + rproc->ops->get_boot_addr = rproc_elf_get_boot_addr;
> + }
> +
> + return 0;
> +}
> +
> /**
> * rproc_alloc() - allocate a remote processor handle
> * @dev: the underlying device
> @@ -2040,8 +2059,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
> if (rproc_alloc_firmware(rproc, name, firmware))
> goto free_rproc;
>
> - rproc->ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL);
> - if (!rproc->ops)
> + if (rproc_alloc_ops(rproc, ops))
> goto free_firmware;
>
> rproc->name = name;
> @@ -2068,16 +2086,6 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>
> atomic_set(&rproc->power, 0);
>
> - /* Default to ELF loader if no load function is specified */
> - if (!rproc->ops->load) {
> - rproc->ops->load = rproc_elf_load_segments;
> - rproc->ops->parse_fw = rproc_elf_load_rsc_table;
> - rproc->ops->find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table;
> - if (!rproc->ops->sanity_check)
> - rproc->ops->sanity_check = rproc_elf32_sanity_check;
> - rproc->ops->get_boot_addr = rproc_elf_get_boot_addr;
> - }
> -
> mutex_init(&rproc->lock);
>
> INIT_LIST_HEAD(&rproc->carveouts);
>
On 4/17/20 8:49 AM, Suman Anna wrote:
> On 4/15/20 3:48 PM, Mathieu Poirier wrote:
>> Make the rproc_ops allocation a function on its own in an effort
>> to clean up function rproc_alloc().
>>
>> Signed-off-by: Mathieu Poirier <[email protected]>
>> Reviewed-by: Alex Elder <[email protected]>
>> ---
>> drivers/remoteproc/remoteproc_core.c | 32 +++++++++++++++++-----------
>> 1 file changed, 20 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c
>> b/drivers/remoteproc/remoteproc_core.c
>> index 0bfa6998705d..a5a0ceb86b3f 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -2001,6 +2001,25 @@ static int rproc_alloc_firmware(struct rproc
>> *rproc,
>> return 0;
>> }
>> +static int rproc_alloc_ops(struct rproc *rproc, const struct
>> rproc_ops *ops)
>> +{
>> + rproc->ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL);
>> + if (!rproc->ops)
>> + return -ENOMEM;
>> +
>> + /* Default to ELF loader if no load function is specified */
>> + if (!rproc->ops->load) {
>> + rproc->ops->load = rproc_elf_load_segments;
>> + rproc->ops->parse_fw = rproc_elf_load_rsc_table;
>> + rproc->ops->find_loaded_rsc_table =
>> + rproc_elf_find_loaded_rsc_table;
>> + rproc->ops->sanity_check = rproc_elf_sanity_check;
>
> So, the conditional check on sanity check is dropped and the callback
> switched here without the changelog reflecting anything why. You should
> just rebase this patch on top of Clement's patch [1] that removes the
> conditional flag, and also usage from the remoteproc platform drivers.
>
> regards
> Suman
>
> [1] https://patchwork.kernel.org/patch/11462013/
Sorry, gave the wrong link, that was v1. This is the latest,
https://patchwork.kernel.org/patch/11466955/
>
>
>> + rproc->ops->get_boot_addr = rproc_elf_get_boot_addr;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> /**
>> * rproc_alloc() - allocate a remote processor handle
>> * @dev: the underlying device
>> @@ -2040,8 +2059,7 @@ struct rproc *rproc_alloc(struct device *dev,
>> const char *name,
>> if (rproc_alloc_firmware(rproc, name, firmware))
>> goto free_rproc;
>> - rproc->ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL);
>> - if (!rproc->ops)
>> + if (rproc_alloc_ops(rproc, ops))
>> goto free_firmware;
>> rproc->name = name;
>> @@ -2068,16 +2086,6 @@ struct rproc *rproc_alloc(struct device *dev,
>> const char *name,
>> atomic_set(&rproc->power, 0);
>> - /* Default to ELF loader if no load function is specified */
>> - if (!rproc->ops->load) {
>> - rproc->ops->load = rproc_elf_load_segments;
>> - rproc->ops->parse_fw = rproc_elf_load_rsc_table;
>> - rproc->ops->find_loaded_rsc_table =
>> rproc_elf_find_loaded_rsc_table;
>> - if (!rproc->ops->sanity_check)
>> - rproc->ops->sanity_check = rproc_elf32_sanity_check;
>> - rproc->ops->get_boot_addr = rproc_elf_get_boot_addr;
>> - }
>> -
>> mutex_init(&rproc->lock);
>> INIT_LIST_HEAD(&rproc->carveouts);
>>
>
>> p = firmware ? kstrdup_const(…) : kasprintf(…);
>
> For simple assignments, I too prefer the ternary operator,
Thanks for your feedback.
> but in this case, I think it is better to leave the current code as is.
Would you like to consider the use of the function “kvasprintf_const”
according to your review comment for the update step “[PATCH v2 4/7] remoteproc:
Use kstrdup_const() rather than kstrup()”?
Regards,
Markus
…
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1982,7 +1982,7 @@ static const struct device_type rproc_type = {
…
> - p = kstrdup(firmware, GFP_KERNEL);
> + p = kstrdup_const(firmware, GFP_KERNEL);
How do you think about to avoid a typo for a function name in
the final commit subject?
Regards,
Markus
On 4/17/20 10:48 AM, Markus Elfring wrote:
>>> p = firmware ? kstrdup_const(…) : kasprintf(…);
>>
>> For simple assignments, I too prefer the ternary operator,
>
> Thanks for your feedback.
>
>
>> but in this case, I think it is better to leave the current code as is.
>
> Would you like to consider the use of the function “kvasprintf_const”
> according to your review comment for the update step “[PATCH v2 4/7] remoteproc:
> Use kstrdup_const() rather than kstrup()”?
This patch is just swapping the condition order, so will automatically
be adjusted for any changes in patch 4 during the rebase.
regards
Suman
Hi Markus,
On Fri, Apr 17, 2020 at 05:48:49PM +0200, Markus Elfring wrote:
> >> p = firmware ? kstrdup_const(…) : kasprintf(…);
> >
> > For simple assignments, I too prefer the ternary operator,
>
> Thanks for your feedback.
>
>
> > but in this case, I think it is better to leave the current code as is.
>
> Would you like to consider the use of the function “kvasprintf_const”
> according to your review comment for the update step “[PATCH v2 4/7] remoteproc:
> Use kstrdup_const() rather than kstrup()”?
>
Looking at the implementation of kvasprintf_const(), using it here wouldn't give
us anything. Indeed allocation of duplicate memory is avoided only if the
string is pre determined of if it is exactly %s, which isn't the case here.
Otherwise things default to kvasprintf().
Thanks,
Mathieu
> Regards,
> Markus
On Fri, Apr 17, 2020 at 08:39:39AM -0500, Suman Anna wrote:
> Hi Markus,
>
> On 4/16/20 1:26 AM, Markus Elfring wrote:
> > …
> > > +++ b/drivers/remoteproc/remoteproc_core.c
> > > @@ -1984,14 +1984,14 @@ static int rproc_alloc_firmware(struct rproc *rproc,
> > > {
> > > const char *p;
> > >
> > > - if (!firmware)
> > > + if (firmware)
> > > + p = kstrdup_const(firmware, GFP_KERNEL);
> > > + else
> > > /*
> > > * If the caller didn't pass in a firmware name then
> > > * construct a default name.
> > > */
> > > p = kasprintf(GFP_KERNEL, "rproc-%s-fw", name);
> > > - else
> > > - p = kstrdup_const(firmware, GFP_KERNEL);
> >
> > Can the use of the conditional operator make sense at such source code places?
> >
> > p = firmware ? kstrdup_const(…) : kasprintf(…);
>
> For simple assignments, I too prefer the ternary operator, but in this case,
> I think it is better to leave the current code as is.
I agree with Suman, that's why I didn't use the conditional operator.
>
> regards
> Suman
On Fri, Apr 17, 2020 at 08:49:25AM -0500, Suman Anna wrote:
> On 4/15/20 3:48 PM, Mathieu Poirier wrote:
> > Make the rproc_ops allocation a function on its own in an effort
> > to clean up function rproc_alloc().
> >
> > Signed-off-by: Mathieu Poirier <[email protected]>
> > Reviewed-by: Alex Elder <[email protected]>
> > ---
> > drivers/remoteproc/remoteproc_core.c | 32 +++++++++++++++++-----------
> > 1 file changed, 20 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index 0bfa6998705d..a5a0ceb86b3f 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -2001,6 +2001,25 @@ static int rproc_alloc_firmware(struct rproc *rproc,
> > return 0;
> > }
> > +static int rproc_alloc_ops(struct rproc *rproc, const struct rproc_ops *ops)
> > +{
> > + rproc->ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL);
> > + if (!rproc->ops)
> > + return -ENOMEM;
> > +
> > + /* Default to ELF loader if no load function is specified */
> > + if (!rproc->ops->load) {
> > + rproc->ops->load = rproc_elf_load_segments;
> > + rproc->ops->parse_fw = rproc_elf_load_rsc_table;
> > + rproc->ops->find_loaded_rsc_table =
> > + rproc_elf_find_loaded_rsc_table;
> > + rproc->ops->sanity_check = rproc_elf_sanity_check;
>
> So, the conditional check on sanity check is dropped and the callback
> switched here without the changelog reflecting anything why. You should just
> rebase this patch on top of Clement's patch [1] that removes the conditional
> flag, and also usage from the remoteproc platform drivers.
That's a rebase that went very wrong...
Thanks for pointing it out,
Mathieu
>
> regards
> Suman
>
> [1] https://patchwork.kernel.org/patch/11462013/
>
>
> > + rproc->ops->get_boot_addr = rproc_elf_get_boot_addr;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > /**
> > * rproc_alloc() - allocate a remote processor handle
> > * @dev: the underlying device
> > @@ -2040,8 +2059,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
> > if (rproc_alloc_firmware(rproc, name, firmware))
> > goto free_rproc;
> > - rproc->ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL);
> > - if (!rproc->ops)
> > + if (rproc_alloc_ops(rproc, ops))
> > goto free_firmware;
> > rproc->name = name;
> > @@ -2068,16 +2086,6 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
> > atomic_set(&rproc->power, 0);
> > - /* Default to ELF loader if no load function is specified */
> > - if (!rproc->ops->load) {
> > - rproc->ops->load = rproc_elf_load_segments;
> > - rproc->ops->parse_fw = rproc_elf_load_rsc_table;
> > - rproc->ops->find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table;
> > - if (!rproc->ops->sanity_check)
> > - rproc->ops->sanity_check = rproc_elf32_sanity_check;
> > - rproc->ops->get_boot_addr = rproc_elf_get_boot_addr;
> > - }
> > -
> > mutex_init(&rproc->lock);
> > INIT_LIST_HEAD(&rproc->carveouts);
> >
>
On Wed 15 Apr 13:48 PDT 2020, Mathieu Poirier wrote:
> Make the firmware name allocation a function on its own in an
> effort to cleanup function rproc_alloc().
>
> Signed-off-by: Mathieu Poirier <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
Regards,
Bjorn
> ---
> drivers/remoteproc/remoteproc_core.c | 66 ++++++++++++++++------------
> 1 file changed, 39 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 80056513ae71..4dee63f319ba 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1979,6 +1979,33 @@ static const struct device_type rproc_type = {
> .release = rproc_type_release,
> };
>
> +static int rproc_alloc_firmware(struct rproc *rproc,
> + const char *name, const char *firmware)
> +{
> + char *p, *template = "rproc-%s-fw";
> + int name_len;
> +
> + if (!firmware) {
> + /*
> + * If the caller didn't pass in a firmware name then
> + * construct a default name.
> + */
> + name_len = strlen(name) + strlen(template) - 2 + 1;
> + p = kmalloc(name_len, GFP_KERNEL);
> + if (!p)
> + return -ENOMEM;
> + snprintf(p, name_len, template, name);
> + } else {
> + p = kstrdup(firmware, GFP_KERNEL);
> + if (!p)
> + return -ENOMEM;
> + }
> +
> + rproc->firmware = p;
> +
> + return 0;
> +}
> +
> /**
> * rproc_alloc() - allocate a remote processor handle
> * @dev: the underlying device
> @@ -2007,42 +2034,21 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
> const char *firmware, int len)
> {
> struct rproc *rproc;
> - char *p, *template = "rproc-%s-fw";
> - int name_len;
>
> if (!dev || !name || !ops)
> return NULL;
>
> - if (!firmware) {
> - /*
> - * If the caller didn't pass in a firmware name then
> - * construct a default name.
> - */
> - name_len = strlen(name) + strlen(template) - 2 + 1;
> - p = kmalloc(name_len, GFP_KERNEL);
> - if (!p)
> - return NULL;
> - snprintf(p, name_len, template, name);
> - } else {
> - p = kstrdup(firmware, GFP_KERNEL);
> - if (!p)
> - return NULL;
> - }
> -
> rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL);
> - if (!rproc) {
> - kfree(p);
> + if (!rproc)
> return NULL;
> - }
> +
> + if (rproc_alloc_firmware(rproc, name, firmware))
> + goto free_rproc;
>
> rproc->ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL);
> - if (!rproc->ops) {
> - kfree(p);
> - kfree(rproc);
> - return NULL;
> - }
> + if (!rproc->ops)
> + goto free_firmware;
>
> - rproc->firmware = p;
> rproc->name = name;
> rproc->priv = &rproc[1];
> rproc->auto_boot = true;
> @@ -2091,6 +2097,12 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
> rproc->state = RPROC_OFFLINE;
>
> return rproc;
> +
> +free_firmware:
> + kfree(rproc->firmware);
> +free_rproc:
> + kfree(rproc);
> + return NULL;
> }
> EXPORT_SYMBOL(rproc_alloc);
>
> --
> 2.20.1
>
On Wed 15 Apr 14:23 PDT 2020, Alex Elder wrote:
> On 4/15/20 3:48 PM, Mathieu Poirier wrote:
> > Improve the readability of function rproc_alloc_firmware() by using
> > a non-negated condition.
> >
> > Suggested-by: Alex Elder <[email protected]>
> > Signed-off-by: Mathieu Poirier <[email protected]>
>
> If it were me, I'd move the comment above the if statement and
> perhaps reword it a little bit to describe what's happening.
> But no matter, this looks good.
>
This would also avoid the fact that we have a multiline block without
braces (which isn't needed, but looks odd to me). So that sounds like a
good idea.
Regards,
Bjorn
> Reviewed-by: Alex Elder <[email protected]>
>
> > ---
> > drivers/remoteproc/remoteproc_core.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index ebaff496ef81..0bfa6998705d 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -1984,14 +1984,14 @@ static int rproc_alloc_firmware(struct rproc *rproc,
> > {
> > const char *p;
> >
> > - if (!firmware)
> > + if (firmware)
> > + p = kstrdup_const(firmware, GFP_KERNEL);
> > + else
> > /*
> > * If the caller didn't pass in a firmware name then
> > * construct a default name.
> > */
> > p = kasprintf(GFP_KERNEL, "rproc-%s-fw", name);
> > - else
> > - p = kstrdup_const(firmware, GFP_KERNEL);
> >
> > if (!p)
> > return -ENOMEM;
> >
>
On Fri 17 Apr 06:44 PDT 2020, Suman Anna wrote:
> On 4/15/20 4:25 PM, Alex Elder wrote:
> > On 4/15/20 3:48 PM, Mathieu Poirier wrote:
> > > For cases where @firmware is declared "const char *", use function
> > > kstrdup_const() to avoid needlessly creating another copy on the
> > > heap.
> > >
> > > Suggested-by: Bjorn Andersson <[email protected]>
> > > Signed-off-by: Mathieu Poirier <[email protected]>
> >
> > Looks good.
> >
> > Reviewed-by: Alex Elder <[email protected]>
> >
> > > ---
> > > drivers/remoteproc/remoteproc_core.c | 4 ++--
> > > include/linux/remoteproc.h | 2 +-
> > > 2 files changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > > index 9899467fa1cf..ebaff496ef81 100644
> > > --- a/drivers/remoteproc/remoteproc_core.c
> > > +++ b/drivers/remoteproc/remoteproc_core.c
> > > @@ -1982,7 +1982,7 @@ static const struct device_type rproc_type = {
> > > static int rproc_alloc_firmware(struct rproc *rproc,
> > > const char *name, const char *firmware)
> > > {
> > > - char *p;
> > > + const char *p;
> > > if (!firmware)
> > > /*
> > > @@ -1991,7 +1991,7 @@ static int rproc_alloc_firmware(struct rproc *rproc,
> > > */
> > > p = kasprintf(GFP_KERNEL, "rproc-%s-fw", name);
>
> So, to be consistent for both paths, should we be using kvasprintf_const()
> here and kfree_const() in release.
Given that the second argument is a "proper" format string
kvasprintf_const() is really just kasprintf() - but with the requirement
that we set up a va_list. So I prefer that we stick with this.
> The kfree_const() is needed to account
> for the kstrdup_const below for sure.
>
You are correct Suman, this patch needs to also change the kfree() to a
kfree_const() or bad things will happen after a visit to
rproc_type_release().
Regards,
Bjorn
> regards
> Suman
>
> > > else
> > > - p = kstrdup(firmware, GFP_KERNEL);
> > > + p = kstrdup_const(firmware, GFP_KERNEL);
> > > if (!p)
> > > return -ENOMEM;
> > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > > index 9c07d7958c53..38607107b7cb 100644
> > > --- a/include/linux/remoteproc.h
> > > +++ b/include/linux/remoteproc.h
> > > @@ -489,7 +489,7 @@ struct rproc {
> > > struct list_head node;
> > > struct iommu_domain *domain;
> > > const char *name;
> > > - char *firmware;
> > > + const char *firmware;
> > > void *priv;
> > > struct rproc_ops *ops;
> > > struct device dev;
> > >
> >
>
Hi Mathieu,
On 4/15/20 10:48 PM, Mathieu Poirier wrote:
> Make the firmware name allocation a function on its own in an
> effort to cleanup function rproc_alloc().
>
> Signed-off-by: Mathieu Poirier <[email protected]>
> ---
> drivers/remoteproc/remoteproc_core.c | 66 ++++++++++++++++------------
> 1 file changed, 39 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 80056513ae71..4dee63f319ba 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1979,6 +1979,33 @@ static const struct device_type rproc_type = {
> .release = rproc_type_release,
> };
>
> +static int rproc_alloc_firmware(struct rproc *rproc,
> + const char *name, const char *firmware)
nitpicking: here you do not allocate memory for the firmware but for its name
The name of the function seems to me quite confusing...
Else LGTM for the series
Thanks,
Arnaud
> +{
> + char *p, *template = "rproc-%s-fw";
> + int name_len;
> +
> + if (!firmware) {
> + /*
> + * If the caller didn't pass in a firmware name then
> + * construct a default name.
> + */
> + name_len = strlen(name) + strlen(template) - 2 + 1;
> + p = kmalloc(name_len, GFP_KERNEL);
> + if (!p)
> + return -ENOMEM;
> + snprintf(p, name_len, template, name);
> + } else {
> + p = kstrdup(firmware, GFP_KERNEL);
> + if (!p)
> + return -ENOMEM;
> + }
> +
> + rproc->firmware = p;
> +
> + return 0;
> +}
> +
> /**
> * rproc_alloc() - allocate a remote processor handle
> * @dev: the underlying device
> @@ -2007,42 +2034,21 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
> const char *firmware, int len)
> {
> struct rproc *rproc;
> - char *p, *template = "rproc-%s-fw";
> - int name_len;
>
> if (!dev || !name || !ops)
> return NULL;
>
> - if (!firmware) {
> - /*
> - * If the caller didn't pass in a firmware name then
> - * construct a default name.
> - */
> - name_len = strlen(name) + strlen(template) - 2 + 1;
> - p = kmalloc(name_len, GFP_KERNEL);
> - if (!p)
> - return NULL;
> - snprintf(p, name_len, template, name);
> - } else {
> - p = kstrdup(firmware, GFP_KERNEL);
> - if (!p)
> - return NULL;
> - }
> -
> rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL);
> - if (!rproc) {
> - kfree(p);
> + if (!rproc)
> return NULL;
> - }
> +
> + if (rproc_alloc_firmware(rproc, name, firmware))
> + goto free_rproc;
>
> rproc->ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL);
> - if (!rproc->ops) {
> - kfree(p);
> - kfree(rproc);
> - return NULL;
> - }
> + if (!rproc->ops)
> + goto free_firmware;
>
> - rproc->firmware = p;
> rproc->name = name;
> rproc->priv = &rproc[1];
> rproc->auto_boot = true;
> @@ -2091,6 +2097,12 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
> rproc->state = RPROC_OFFLINE;
>
> return rproc;
> +
> +free_firmware:
> + kfree(rproc->firmware);
> +free_rproc:
> + kfree(rproc);
> + return NULL;
> }
> EXPORT_SYMBOL(rproc_alloc);
>
>
Hey,
On Mon, 20 Apr 2020 at 03:24, Arnaud POULIQUEN <[email protected]> wrote:
>
> Hi Mathieu,
>
> On 4/15/20 10:48 PM, Mathieu Poirier wrote:
> > Make the firmware name allocation a function on its own in an
> > effort to cleanup function rproc_alloc().
> >
> > Signed-off-by: Mathieu Poirier <[email protected]>
> > ---
> > drivers/remoteproc/remoteproc_core.c | 66 ++++++++++++++++------------
> > 1 file changed, 39 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index 80056513ae71..4dee63f319ba 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -1979,6 +1979,33 @@ static const struct device_type rproc_type = {
> > .release = rproc_type_release,
> > };
> >
> > +static int rproc_alloc_firmware(struct rproc *rproc,
> > + const char *name, const char *firmware)
>
> nitpicking: here you do not allocate memory for the firmware but for its name
> The name of the function seems to me quite confusing...
Ok, I'll see if I can find something better.
>
> Else LGTM for the series
V3 will be out shortly and it will be fairly different from this one.
>
> Thanks,
>
> Arnaud
>
> > +{
> > + char *p, *template = "rproc-%s-fw";
> > + int name_len;
> > +
> > + if (!firmware) {
> > + /*
> > + * If the caller didn't pass in a firmware name then
> > + * construct a default name.
> > + */
> > + name_len = strlen(name) + strlen(template) - 2 + 1;
> > + p = kmalloc(name_len, GFP_KERNEL);
> > + if (!p)
> > + return -ENOMEM;
> > + snprintf(p, name_len, template, name);
> > + } else {
> > + p = kstrdup(firmware, GFP_KERNEL);
> > + if (!p)
> > + return -ENOMEM;
> > + }
> > +
> > + rproc->firmware = p;
> > +
> > + return 0;
> > +}
> > +
> > /**
> > * rproc_alloc() - allocate a remote processor handle
> > * @dev: the underlying device
> > @@ -2007,42 +2034,21 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
> > const char *firmware, int len)
> > {
> > struct rproc *rproc;
> > - char *p, *template = "rproc-%s-fw";
> > - int name_len;
> >
> > if (!dev || !name || !ops)
> > return NULL;
> >
> > - if (!firmware) {
> > - /*
> > - * If the caller didn't pass in a firmware name then
> > - * construct a default name.
> > - */
> > - name_len = strlen(name) + strlen(template) - 2 + 1;
> > - p = kmalloc(name_len, GFP_KERNEL);
> > - if (!p)
> > - return NULL;
> > - snprintf(p, name_len, template, name);
> > - } else {
> > - p = kstrdup(firmware, GFP_KERNEL);
> > - if (!p)
> > - return NULL;
> > - }
> > -
> > rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL);
> > - if (!rproc) {
> > - kfree(p);
> > + if (!rproc)
> > return NULL;
> > - }
> > +
> > + if (rproc_alloc_firmware(rproc, name, firmware))
> > + goto free_rproc;
> >
> > rproc->ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL);
> > - if (!rproc->ops) {
> > - kfree(p);
> > - kfree(rproc);
> > - return NULL;
> > - }
> > + if (!rproc->ops)
> > + goto free_firmware;
> >
> > - rproc->firmware = p;
> > rproc->name = name;
> > rproc->priv = &rproc[1];
> > rproc->auto_boot = true;
> > @@ -2091,6 +2097,12 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
> > rproc->state = RPROC_OFFLINE;
> >
> > return rproc;
> > +
> > +free_firmware:
> > + kfree(rproc->firmware);
> > +free_rproc:
> > + kfree(rproc);
> > + return NULL;
> > }
> > EXPORT_SYMBOL(rproc_alloc);
> >
> >