[PATCH v2 0/7] remoteproc: Refactor function rproc_alloc()

[PATCH v2 0/7] remoteproc: Refactor function rproc_alloc()

[Mathieu Poirier]

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

[PATCH v2 1/7] remoteproc: Fix IDR initialisation in rproc_alloc()

[Mathieu Poirier]

From: Alex Elder <elder@linaro.org>

If ida_simple_get() returns an error when called in rproc_alloc(),
put_device() is called to clean things up.  By this time the rproc
device type has been assigned, with rproc_type_release() as the
release function.

The first thing rproc_type_release() does is call:
    idr_destroy(&rproc->notifyids);

But at the time the ida_simple_get() call is made, the notifyids
field in the remoteproc structure has not been initialized.

I'm not actually sure this case causes an observable problem, but
it's incorrect.  Fix this by initializing the notifyids field before
calling ida_simple_get() in rproc_alloc().

Fixes: b5ab5e24e960 ("remoteproc: maintain a generic child device for each rproc")
Signed-off-by: Alex Elder <elder@linaro.org>
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index e12a54e67588..80056513ae71 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2053,6 +2053,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 	rproc->dev.type = &rproc_type;
 	rproc->dev.class = &rproc_class;
 	rproc->dev.driver_data = rproc;
+	idr_init(&rproc->notifyids);
 
 	/* Assign a unique device index and name */
 	rproc->index = ida_simple_get(&rproc_dev_index, 0, 0, GFP_KERNEL);
@@ -2078,8 +2079,6 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 
 	mutex_init(&rproc->lock);
 
-	idr_init(&rproc->notifyids);
-
 	INIT_LIST_HEAD(&rproc->carveouts);
 	INIT_LIST_HEAD(&rproc->mappings);
 	INIT_LIST_HEAD(&rproc->traces);
-- 
2.20.1

[PATCH v2 2/7] remoteproc: Split firmware name allocation from rproc_alloc()

[Mathieu Poirier]

Make the firmware name allocation a function on its own in an
effort to cleanup function rproc_alloc().

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 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

[PATCH v2 3/7] remoteproc: Simplify default name allocation

[Mathieu Poirier]

In an effort to cleanup firmware name allocation, replace the
cumbersome mechanic used to allocate a default firmware name with
function kasprintf().

Suggested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 4dee63f319ba..9899467fa1cf 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1982,24 +1982,19 @@ static const struct device_type rproc_type = {
 static int rproc_alloc_firmware(struct rproc *rproc,
 				const char *name, const char *firmware)
 {
-	char *p, *template = "rproc-%s-fw";
-	int name_len;
+	char *p;
 
-	if (!firmware) {
+	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 = kasprintf(GFP_KERNEL, "rproc-%s-fw", name);
+	else
 		p = kstrdup(firmware, GFP_KERNEL);
-		if (!p)
-			return -ENOMEM;
-	}
+
+	if (!p)
+		return -ENOMEM;
 
 	rproc->firmware = p;
 
-- 
2.20.1

[PATCH v2 4/7] remoteproc: Use kstrdup_const() rather than kstrup()

[Mathieu Poirier]

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 <bjorn.andersson@linaro.org>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 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

[PATCH v2 5/7] remoteproc: Restructure firmware name allocation

[Mathieu Poirier]

Improve the readability of function rproc_alloc_firmware() by using
a non-negated condition.

Suggested-by: Alex Elder <elder@linaro.org>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 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

[PATCH v2 6/7] remoteproc: Split rproc_ops allocation from rproc_alloc()

[Mathieu Poirier]

Make the rproc_ops allocation a function on its own in an effort
to clean up function rproc_alloc().

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Alex Elder <elder@linaro.org>
---
 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

[PATCH v2 7/7] remoteproc: Get rid of tedious error path

[Mathieu Poirier]

Get rid of tedious error management by moving firmware and operation
allocation after calling device_initialize().  That way we take advantage
of the automatic call to rproc_type_release() to cleanup after ourselves
when put_device() is called.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Alex Elder <elder@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index a5a0ceb86b3f..405c94f151a7 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2056,12 +2056,6 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 	if (!rproc)
 		return NULL;
 
-	if (rproc_alloc_firmware(rproc, name, firmware))
-		goto free_rproc;
-
-	if (rproc_alloc_ops(rproc, ops))
-		goto free_firmware;
-
 	rproc->name = name;
 	rproc->priv = &rproc[1];
 	rproc->auto_boot = true;
@@ -2074,12 +2068,17 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 	rproc->dev.driver_data = rproc;
 	idr_init(&rproc->notifyids);
 
+	if (rproc_alloc_firmware(rproc, name, firmware))
+		goto put_device;
+
+	if (rproc_alloc_ops(rproc, ops))
+		goto put_device;
+
 	/* Assign a unique device index and name */
 	rproc->index = ida_simple_get(&rproc_dev_index, 0, 0, GFP_KERNEL);
 	if (rproc->index < 0) {
 		dev_err(dev, "ida_simple_get failed: %d\n", rproc->index);
-		put_device(&rproc->dev);
-		return NULL;
+		goto put_device;
 	}
 
 	dev_set_name(&rproc->dev, "remoteproc%d", rproc->index);
@@ -2100,11 +2099,8 @@ 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);
+put_device:
+	put_device(&rproc->dev);
 	return NULL;
 }
 EXPORT_SYMBOL(rproc_alloc);
-- 
2.20.1

Re: [PATCH v2 5/7] remoteproc: Restructure firmware name allocation

[Alex Elder]

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 <elder@linaro.org>
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>

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 <elder@linaro.org>

> ---
>  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;
> 

Re: [PATCH v2 4/7] remoteproc: Use kstrdup_const() rather than kstrup()

[Alex Elder]

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 <bjorn.andersson@linaro.org>
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>

Looks good.

Reviewed-by: Alex Elder <elder@linaro.org>

> ---
>  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;
> 

Re: [PATCH v2 3/7] remoteproc: Simplify default name allocation

[Alex Elder]

On 4/15/20 3:48 PM, Mathieu Poirier wrote:
> In an effort to cleanup firmware name allocation, replace the
> cumbersome mechanic used to allocate a default firmware name with
> function kasprintf().
> 
> Suggested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>

Looks good to me.

Reviewed-by: Alex Elder <elder@linaro.org>

> ---
>  drivers/remoteproc/remoteproc_core.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 4dee63f319ba..9899467fa1cf 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1982,24 +1982,19 @@ static const struct device_type rproc_type = {
>  static int rproc_alloc_firmware(struct rproc *rproc,
>  				const char *name, const char *firmware)
>  {
> -	char *p, *template = "rproc-%s-fw";
> -	int name_len;
> +	char *p;
>  
> -	if (!firmware) {
> +	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 = kasprintf(GFP_KERNEL, "rproc-%s-fw", name);
> +	else
>  		p = kstrdup(firmware, GFP_KERNEL);
> -		if (!p)
> -			return -ENOMEM;
> -	}
> +
> +	if (!p)
> +		return -ENOMEM;
>  
>  	rproc->firmware = p;
>  
> 

Re: [PATCH v2 2/7] remoteproc: Split firmware name allocation from rproc_alloc()

[Alex Elder]

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 <mathieu.poirier@linaro.org>

Looks good.

Reviewed-by: Alex Elder <elder@linaro.org>

> ---
>  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);
>  
> 

Re: [PATCH v2 5/7] remoteproc: Restructure firmware name allocation

[Markus Elfring]

…
> +++ 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

Re: [PATCH v2 0/7] remoteproc: Refactor function rproc_alloc()

[Suman Anna]

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(-)
> 

Re: [PATCH v2 1/7] remoteproc: Fix IDR initialisation in rproc_alloc()

[Suman Anna]

On 4/15/20 3:48 PM, Mathieu Poirier wrote:
> From: Alex Elder <elder@linaro.org>
> 
> If ida_simple_get() returns an error when called in rproc_alloc(),
> put_device() is called to clean things up.  By this time the rproc
> device type has been assigned, with rproc_type_release() as the
> release function.
> 
> The first thing rproc_type_release() does is call:
>      idr_destroy(&rproc->notifyids);
> 
> But at the time the ida_simple_get() call is made, the notifyids
> field in the remoteproc structure has not been initialized.
> 
> I'm not actually sure this case causes an observable problem, but
> it's incorrect.  Fix this by initializing the notifyids field before
> calling ida_simple_get() in rproc_alloc().
> 
> Fixes: b5ab5e24e960 ("remoteproc: maintain a generic child device for each rproc")
> Signed-off-by: Alex Elder <elder@linaro.org>
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

Reviewed-by: Suman Anna <s-anna@ti.com>

Thanks Alex. I had exactly this particular comment on Mathieu's sync 
patch series [1].

regards
Suman

[1] https://patchwork.kernel.org/comment/23254901/

> ---
>   drivers/remoteproc/remoteproc_core.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index e12a54e67588..80056513ae71 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2053,6 +2053,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>   	rproc->dev.type = &rproc_type;
>   	rproc->dev.class = &rproc_class;
>   	rproc->dev.driver_data = rproc;
> +	idr_init(&rproc->notifyids);
>   
>   	/* Assign a unique device index and name */
>   	rproc->index = ida_simple_get(&rproc_dev_index, 0, 0, GFP_KERNEL);
> @@ -2078,8 +2079,6 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>   
>   	mutex_init(&rproc->lock);
>   
> -	idr_init(&rproc->notifyids);
> -
>   	INIT_LIST_HEAD(&rproc->carveouts);
>   	INIT_LIST_HEAD(&rproc->mappings);
>   	INIT_LIST_HEAD(&rproc->traces);
> 

Re: [PATCH v2 5/7] remoteproc: Restructure firmware name allocation

[Suman Anna]

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

Re: [PATCH v2 4/7] remoteproc: Use kstrdup_const() rather than kstrup()

[Suman Anna]

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 <bjorn.andersson@linaro.org>
>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> 
> Looks good.
> 
> Reviewed-by: Alex Elder <elder@linaro.org>
> 
>> ---
>>   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;
>>
> 

Re: [PATCH v2 6/7] remoteproc: Split rproc_ops allocation from rproc_alloc()

[Suman Anna]

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 <mathieu.poirier@linaro.org>
> Reviewed-by: Alex Elder <elder@linaro.org>
> ---
>   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);
> 

Re: [PATCH v2 7/7] remoteproc: Get rid of tedious error path

[Suman Anna]

On 4/15/20 3:48 PM, Mathieu Poirier wrote:
> Get rid of tedious error management by moving firmware and operation
> allocation after calling device_initialize().  That way we take advantage
> of the automatic call to rproc_type_release() to cleanup after ourselves
> when put_device() is called.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Reviewed-by: Alex Elder <elder@linaro.org>

Acked-by: Suman Anna <s-anna@ti.com>

regards
Suman

> ---
>   drivers/remoteproc/remoteproc_core.c | 22 +++++++++-------------
>   1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index a5a0ceb86b3f..405c94f151a7 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2056,12 +2056,6 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>   	if (!rproc)
>   		return NULL;
>   
> -	if (rproc_alloc_firmware(rproc, name, firmware))
> -		goto free_rproc;
> -
> -	if (rproc_alloc_ops(rproc, ops))
> -		goto free_firmware;
> -
>   	rproc->name = name;
>   	rproc->priv = &rproc[1];
>   	rproc->auto_boot = true;
> @@ -2074,12 +2068,17 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>   	rproc->dev.driver_data = rproc;
>   	idr_init(&rproc->notifyids);
>   
> +	if (rproc_alloc_firmware(rproc, name, firmware))
> +		goto put_device;
> +
> +	if (rproc_alloc_ops(rproc, ops))
> +		goto put_device;
> +
>   	/* Assign a unique device index and name */
>   	rproc->index = ida_simple_get(&rproc_dev_index, 0, 0, GFP_KERNEL);
>   	if (rproc->index < 0) {
>   		dev_err(dev, "ida_simple_get failed: %d\n", rproc->index);
> -		put_device(&rproc->dev);
> -		return NULL;
> +		goto put_device;
>   	}
>   
>   	dev_set_name(&rproc->dev, "remoteproc%d", rproc->index);
> @@ -2100,11 +2099,8 @@ 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);
> +put_device:
> +	put_device(&rproc->dev);
>   	return NULL;
>   }
>   EXPORT_SYMBOL(rproc_alloc);
> 

Re: [PATCH v2 6/7] remoteproc: Split rproc_ops allocation from rproc_alloc()

[Suman Anna]

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 <mathieu.poirier@linaro.org>
>> Reviewed-by: Alex Elder <elder@linaro.org>
>> ---
>>   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);
>>
> 

Re: [v2 5/7] remoteproc: Restructure firmware name allocation

[Markus Elfring]

>>     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

Re: [v2 4/7] remoteproc: Use kstrdup_const() rather than kstrdup()

[Markus Elfring]

…
> +++ 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

Re: [v2 5/7] remoteproc: Restructure firmware name allocation

[Suman Anna]

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

Re: [v2 5/7] remoteproc: Restructure firmware name allocation

[Mathieu Poirier]

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

Re: [PATCH v2 5/7] remoteproc: Restructure firmware name allocation

[Mathieu Poirier]

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

Re: [PATCH v2 6/7] remoteproc: Split rproc_ops allocation from rproc_alloc()

[Mathieu Poirier]

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 <mathieu.poirier@linaro.org>
> > Reviewed-by: Alex Elder <elder@linaro.org>
> > ---
> >   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);
> > 
> 

Re: [PATCH v2 1/7] remoteproc: Fix IDR initialisation in rproc_alloc()

[Bjorn Andersson]

On Wed 15 Apr 13:48 PDT 2020, Mathieu Poirier wrote:

> From: Alex Elder <elder@linaro.org>
> 
> If ida_simple_get() returns an error when called in rproc_alloc(),
> put_device() is called to clean things up.  By this time the rproc
> device type has been assigned, with rproc_type_release() as the
> release function.
> 
> The first thing rproc_type_release() does is call:
>     idr_destroy(&rproc->notifyids);
> 
> But at the time the ida_simple_get() call is made, the notifyids
> field in the remoteproc structure has not been initialized.
> 
> I'm not actually sure this case causes an observable problem, but
> it's incorrect.  Fix this by initializing the notifyids field before
> calling ida_simple_get() in rproc_alloc().
> 
> Fixes: b5ab5e24e960 ("remoteproc: maintain a generic child device for each rproc")
> Signed-off-by: Alex Elder <elder@linaro.org>
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> ---
>  drivers/remoteproc/remoteproc_core.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index e12a54e67588..80056513ae71 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2053,6 +2053,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>  	rproc->dev.type = &rproc_type;
>  	rproc->dev.class = &rproc_class;
>  	rproc->dev.driver_data = rproc;
> +	idr_init(&rproc->notifyids);
>  
>  	/* Assign a unique device index and name */
>  	rproc->index = ida_simple_get(&rproc_dev_index, 0, 0, GFP_KERNEL);
> @@ -2078,8 +2079,6 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>  
>  	mutex_init(&rproc->lock);
>  
> -	idr_init(&rproc->notifyids);
> -
>  	INIT_LIST_HEAD(&rproc->carveouts);
>  	INIT_LIST_HEAD(&rproc->mappings);
>  	INIT_LIST_HEAD(&rproc->traces);
> -- 
> 2.20.1
> 

Re: [PATCH v2 2/7] remoteproc: Split firmware name allocation from rproc_alloc()

[Bjorn Andersson]

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 <mathieu.poirier@linaro.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

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
> 

Re: [PATCH v2 3/7] remoteproc: Simplify default name allocation

[Bjorn Andersson]

On Wed 15 Apr 13:48 PDT 2020, Mathieu Poirier wrote:

> In an effort to cleanup firmware name allocation, replace the
> cumbersome mechanic used to allocate a default firmware name with
> function kasprintf().
> 
> Suggested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> ---
>  drivers/remoteproc/remoteproc_core.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 4dee63f319ba..9899467fa1cf 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1982,24 +1982,19 @@ static const struct device_type rproc_type = {
>  static int rproc_alloc_firmware(struct rproc *rproc,
>  				const char *name, const char *firmware)
>  {
> -	char *p, *template = "rproc-%s-fw";
> -	int name_len;
> +	char *p;
>  
> -	if (!firmware) {
> +	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 = kasprintf(GFP_KERNEL, "rproc-%s-fw", name);
> +	else
>  		p = kstrdup(firmware, GFP_KERNEL);
> -		if (!p)
> -			return -ENOMEM;
> -	}
> +
> +	if (!p)
> +		return -ENOMEM;
>  
>  	rproc->firmware = p;
>  
> -- 
> 2.20.1
> 

Re: [PATCH v2 5/7] remoteproc: Restructure firmware name allocation

[Bjorn Andersson]

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 <elder@linaro.org>
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> 
> 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 <elder@linaro.org>
> 
> > ---
> >  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;
> > 
> 

Re: [PATCH v2 4/7] remoteproc: Use kstrdup_const() rather than kstrup()

[Bjorn Andersson]

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 <bjorn.andersson@linaro.org>
> > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > 
> > Looks good.
> > 
> > Reviewed-by: Alex Elder <elder@linaro.org>
> > 
> > > ---
> > >   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;
> > > 
> > 
> 

Re: [PATCH v2 2/7] remoteproc: Split firmware name allocation from rproc_alloc()

[Arnaud POULIQUEN]

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 <mathieu.poirier@linaro.org>
> ---
>  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);
>  
> 

Re: [PATCH v2 2/7] remoteproc: Split firmware name allocation from rproc_alloc()

[Mathieu Poirier]

Hey,

On Mon, 20 Apr 2020 at 03:24, Arnaud POULIQUEN <arnaud.pouliquen@st.com> 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 <mathieu.poirier@linaro.org>
> > ---
> >  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);
> >
> >