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

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

[Mathieu Poirier]

Hi Bjorn,

Function rproc_alloc() had to be reworked as part of my work on MCU
synchronisation[1].  Concurrent to that Suman [2] and Alex [3] also did
some work on the same function.  

In order to provide a clean foundation for everyone to work from and avoid
unpleasant merge conflicts this set gathers the work that both Alex and I
did.  That way Suman and I can move forward with our respective patchsets
without too much headache.   

Applies cleanly on v5.7-rc1.

Thanks,
Mathieu

[1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=261069
[2]. https://patchwork.kernel.org/patch/11456385/
[3]. https://patchwork.kernel.org/patch/11473241/ 

Alex Elder (1):
  remoteproc: Fix a bug in rproc_alloc()

Mathieu Poirier (3):
  remoteproc: Split firmware name allocation from rproc_alloc()
  remoteproc: Split rproc_ops allocation from rproc_alloc()
  remoteproc: Get rid of tedious error path

 drivers/remoteproc/remoteproc_core.c | 101 +++++++++++++++------------
 1 file changed, 58 insertions(+), 43 deletions(-)

-- 
2.20.1

[PATCH 1/4] remoteproc: Fix a bug 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().

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 2/4] remoteproc: Split firmware name allocation from rproc_alloc()

[Mathieu Poirier]

Make the firmware name allocation a function on its own in order to
introduce more flexibility to 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 3/4] remoteproc: Split rproc_ops allocation from rproc_alloc()

[Mathieu Poirier]

Make the rproc_ops allocation a function on its own in order to
introduce more flexibility to function rproc_alloc().

Signed-off-by: Mathieu Poirier <mathieu.poirier@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 4dee63f319ba..c272d78f07e8 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2006,6 +2006,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
@@ -2045,8 +2064,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;
@@ -2073,16 +2091,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 4/4] 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>
---
 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 c272d78f07e8..10009b95867a 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2061,12 +2061,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;
@@ -2079,12 +2073,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 out;
+
+	if (rproc_alloc_ops(rproc, ops))
+		goto out;
+
 	/* 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 out;
 	}
 
 	dev_set_name(&rproc->dev, "remoteproc%d", rproc->index);
@@ -2105,11 +2104,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);
+out:
+	put_device(&rproc->dev);
 	return NULL;
 }
 EXPORT_SYMBOL(rproc_alloc);
-- 
2.20.1

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

[Alex Elder]

On 4/13/20 2:33 PM, Mathieu Poirier wrote:
> Make the firmware name allocation a function on its own in order to
> introduce more flexibility to function rproc_alloc().
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>

I didn't look at the larger context (MCU series); I'm only looking
at this (and the others in this series) in isolation.  I like
that you're encapsulating this stuff into functions but doing so
doesn't really add any flexibility.

Two small suggestions for you to consider but they're truly
more about style so it's entirely up to you.  Outside of that
this looks straightforward to me, and the result of the series
is an improvement.

I'll let you comment on my suggestions before offering my
"reviewed-by" indication.

					-Alex

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

Not a big deal (and maybe it's not consistent with other nearby
style) but template and name_len could be defined inside the
"if (!firmware)" block.

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


I don't know if it would be an improvement, but you could
check for a null p value below for both cases.  I.e.:

		if (p)
			snprintf(p, ...);

(more below)

> +		if (!p)
> +			return -ENOMEM;
> +		snprintf(p, name_len, template, name);
> +	} else {
> +		p = kstrdup(firmware, GFP_KERNEL);
> +		if (!p)
> +			return -ENOMEM;
> +	}
> +

	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 3/4] remoteproc: Split rproc_ops allocation from rproc_alloc()

[Alex Elder]

On 4/13/20 2:34 PM, Mathieu Poirier wrote:
> Make the rproc_ops allocation a function on its own in order to
> introduce more flexibility to 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 | 32 +++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 4dee63f319ba..c272d78f07e8 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2006,6 +2006,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;

I'm glad you made this unconditional, the sanity check function
is always null when the load function is null.

> +		rproc->ops->get_boot_addr = rproc_elf_get_boot_addr;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * rproc_alloc() - allocate a remote processor handle
>   * @dev: the underlying device
> @@ -2045,8 +2064,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;
> @@ -2073,16 +2091,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 4/4] remoteproc: Get rid of tedious error path

[Alex Elder]

On 4/13/20 2:34 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.

Looks good to me.

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

> Signed-off-by: Mathieu Poirier <mathieu.poirier@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 c272d78f07e8..10009b95867a 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2061,12 +2061,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;
> @@ -2079,12 +2073,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 out;
> +
> +	if (rproc_alloc_ops(rproc, ops))
> +		goto out;
> +
>  	/* 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 out;
>  	}
>  
>  	dev_set_name(&rproc->dev, "remoteproc%d", rproc->index);
> @@ -2105,11 +2104,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);
> +out:
> +	put_device(&rproc->dev);
>  	return NULL;
>  }
>  EXPORT_SYMBOL(rproc_alloc);
> 

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

[Alex Elder]

On 4/13/20 2:34 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.

Looks good to me.

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

> Signed-off-by: Mathieu Poirier <mathieu.poirier@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 c272d78f07e8..10009b95867a 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2061,12 +2061,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;
> @@ -2079,12 +2073,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 out;
> +
> +	if (rproc_alloc_ops(rproc, ops))
> +		goto out;
> +
>  	/* 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 out;
>  	}
>  
>  	dev_set_name(&rproc->dev, "remoteproc%d", rproc->index);
> @@ -2105,11 +2104,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);
> +out:
> +	put_device(&rproc->dev);
>  	return NULL;
>  }
>  EXPORT_SYMBOL(rproc_alloc);
> 

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

[Bjorn Andersson]

On Mon 13 Apr 13:56 PDT 2020, Alex Elder wrote:

> On 4/13/20 2:33 PM, Mathieu Poirier wrote:
> > Make the firmware name allocation a function on its own in order to
> > introduce more flexibility to function rproc_alloc().
> > 
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> 
> I didn't look at the larger context (MCU series); I'm only looking
> at this (and the others in this series) in isolation.  I like
> that you're encapsulating this stuff into functions but doing so
> doesn't really add any flexibility.
> 
> Two small suggestions for you to consider but they're truly
> more about style so it's entirely up to you.  Outside of that
> this looks straightforward to me, and the result of the series
> is an improvement.
> 
> I'll let you comment on my suggestions before offering my
> "reviewed-by" indication.
> 
> 					-Alex
> 
> > ---
> >  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;
> 
> Not a big deal (and maybe it's not consistent with other nearby
> style) but template and name_len could be defined inside the
> "if (!firmware)" block.
> 

I prefer variables declared in the beginning of the function, so I'm
happy with this.

> > +	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);
> 
> 
> I don't know if it would be an improvement, but you could
> check for a null p value below for both cases.  I.e.:
> 
> 		if (p)
> 			snprintf(p, ...);
> 

Moving the common NULL check and return out seems nice, but given that
we then have to have this positive conditional I think the end result is
more complex.

That said, if we're not just doing a verbatim copy from rproc_alloc() I
think we should make this function:

	if (!firmware)
		p = kasprintf(GFP_KERNEL, "rproc-%s-fw", name);
	else
		p = kstrdup_const(firmware, GFP_KERNEL);

	rproc->firmware = p;

	return p ? 0 : -ENOMEM;

Regards,
Bjorn

> (more below)
> 
> > +		if (!p)
> > +			return -ENOMEM;
> > +		snprintf(p, name_len, template, name);
> > +	} else {
> > +		p = kstrdup(firmware, GFP_KERNEL);
> > +		if (!p)
> > +			return -ENOMEM;
> > +	}
> > +
> 
> 	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 2/4] remoteproc: Split firmware name allocation from rproc_alloc()

[Alex Elder]

On 4/13/20 7:55 PM, Bjorn Andersson wrote:
> On Mon 13 Apr 13:56 PDT 2020, Alex Elder wrote:
> 
>> On 4/13/20 2:33 PM, Mathieu Poirier wrote:
>>> Make the firmware name allocation a function on its own in order to
>>> introduce more flexibility to 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;
>>
>> Not a big deal (and maybe it's not consistent with other nearby
>> style) but template and name_len could be defined inside the
>> "if (!firmware)" block.
>>
> 
> I prefer variables declared in the beginning of the function, so I'm
> happy with this.

It should be obvious that this is fine with me.

>>> +	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);
>>
>>
>> I don't know if it would be an improvement, but you could
>> check for a null p value below for both cases.  I.e.:
>>
>> 		if (p)
>> 			snprintf(p, ...);
>>
> 
> Moving the common NULL check and return out seems nice, but given that
> we then have to have this positive conditional I think the end result is
> more complex.
> 
> That said, if we're not just doing a verbatim copy from rproc_alloc() I
> think we should make this function:
> 
> 	if (!firmware)
> 		p = kasprintf(GFP_KERNEL, "rproc-%s-fw", name);
> 	else
> 		p = kstrdup_const(firmware, GFP_KERNEL);

You know, I wanted to suggest this but I didn't know the
name of the function (kasprintf()) and didn't take the time
to find it.  I wholly agree with your suggestion.

The only additional minor tweak I'd add is that I prefer
using a non-negated condition where possible, though it
doesn't always "look right."  So:

	if (firmware)
		rproc->firmware = kstrdup_const(firmware, GFP_KERNEL);
	else
		rproc->firmware = kasprintf(GFP_KERNEL, "rproc-%s-fw", name);

					-Alex

> 	rproc->firmware = p;
> 
> 	return p ? 0 : -ENOMEM;
> 
> Regards,
> Bjorn
> 
>> (more below)
>>
>>> +		if (!p)
>>> +			return -ENOMEM;
>>> +		snprintf(p, name_len, template, name);
>>> +	} else {
>>> +		p = kstrdup(firmware, GFP_KERNEL);
>>> +		if (!p)
>>> +			return -ENOMEM;
>>> +	}
>>> +
>>
>> 	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 2/4] remoteproc: Split firmware name allocation from rproc_alloc()

[Mathieu Poirier]

Hi guys,

On Mon, 13 Apr 2020 at 18:54, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Mon 13 Apr 13:56 PDT 2020, Alex Elder wrote:
>
> > On 4/13/20 2:33 PM, Mathieu Poirier wrote:
> > > Make the firmware name allocation a function on its own in order to
> > > introduce more flexibility to function rproc_alloc().
> > >
> > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> >
> > I didn't look at the larger context (MCU series); I'm only looking
> > at this (and the others in this series) in isolation.  I like
> > that you're encapsulating this stuff into functions but doing so
> > doesn't really add any flexibility.
> >
> > Two small suggestions for you to consider but they're truly
> > more about style so it's entirely up to you.  Outside of that
> > this looks straightforward to me, and the result of the series
> > is an improvement.
> >
> > I'll let you comment on my suggestions before offering my
> > "reviewed-by" indication.
> >
> >                                       -Alex
> >
> > > ---
> > >  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;
> >
> > Not a big deal (and maybe it's not consistent with other nearby
> > style) but template and name_len could be defined inside the
> > "if (!firmware)" block.
> >
>
> I prefer variables declared in the beginning of the function, so I'm
> happy with this.
>
> > > +   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);
> >
> >
> > I don't know if it would be an improvement, but you could
> > check for a null p value below for both cases.  I.e.:
> >
> >               if (p)
> >                       snprintf(p, ...);
> >
>
> Moving the common NULL check and return out seems nice, but given that
> we then have to have this positive conditional I think the end result is
> more complex.
>
> That said, if we're not just doing a verbatim copy from rproc_alloc() I
> think we should make this function:
>
>         if (!firmware)
>                 p = kasprintf(GFP_KERNEL, "rproc-%s-fw", name);
>         else
>                 p = kstrdup_const(firmware, GFP_KERNEL);
>
>         rproc->firmware = p;
>
>         return p ? 0 : -ENOMEM;

At this time I was going for a pure re-arrangement of the code and
avoiding further improvement.  This is simple enough that it can be
rolled-in the next revision.

Thanks,
Mathieu

>
> Regards,
> Bjorn
>
> > (more below)
> >
> > > +           if (!p)
> > > +                   return -ENOMEM;
> > > +           snprintf(p, name_len, template, name);
> > > +   } else {
> > > +           p = kstrdup(firmware, GFP_KERNEL);
> > > +           if (!p)
> > > +                   return -ENOMEM;
> > > +   }
> > > +
> >
> >       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 2/4] remoteproc: Split firmware name allocation from rproc_alloc()

[Mathieu Poirier]

On Mon, 13 Apr 2020 at 14:56, Alex Elder <elder@linaro.org> wrote:
>
> On 4/13/20 2:33 PM, Mathieu Poirier wrote:
> > Make the firmware name allocation a function on its own in order to
> > introduce more flexibility to function rproc_alloc().
> >
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>
> I didn't look at the larger context (MCU series); I'm only looking
> at this (and the others in this series) in isolation.  I like
> that you're encapsulating this stuff into functions but doing so
> doesn't really add any flexibility.

You are correct.  I wrote the changelog with the MCU synchronisation
series in mind but this specific part of the work has nothing to do
with flexibility - it is a plane cleanup exercise.  I will address
that in the next revision.

Thanks,
Mathieu


>
> Two small suggestions for you to consider but they're truly
> more about style so it's entirely up to you.  Outside of that
> this looks straightforward to me, and the result of the series
> is an improvement.
>
> I'll let you comment on my suggestions before offering my
> "reviewed-by" indication.
>
>                                         -Alex
>
> > ---
> >  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;
>
> Not a big deal (and maybe it's not consistent with other nearby
> style) but template and name_len could be defined inside the
> "if (!firmware)" block.
>
> > +     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);
>
>
> I don't know if it would be an improvement, but you could
> check for a null p value below for both cases.  I.e.:
>
>                 if (p)
>                         snprintf(p, ...);
>
> (more below)
>
> > +             if (!p)
> > +                     return -ENOMEM;
> > +             snprintf(p, name_len, template, name);
> > +     } else {
> > +             p = kstrdup(firmware, GFP_KERNEL);
> > +             if (!p)
> > +                     return -ENOMEM;
> > +     }
> > +
>
>         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 2/4] remoteproc: Split firmware name allocation from rproc_alloc()

[Mathieu Poirier]

Hey Bjorn,

On Mon, Apr 13, 2020 at 05:55:06PM -0700, Bjorn Andersson wrote:
> On Mon 13 Apr 13:56 PDT 2020, Alex Elder wrote:
> 
> > On 4/13/20 2:33 PM, Mathieu Poirier wrote:
> > > Make the firmware name allocation a function on its own in order to
> > > introduce more flexibility to function rproc_alloc().
> > > 
> > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > 
> > I didn't look at the larger context (MCU series); I'm only looking
> > at this (and the others in this series) in isolation.  I like
> > that you're encapsulating this stuff into functions but doing so
> > doesn't really add any flexibility.
> > 
> > Two small suggestions for you to consider but they're truly
> > more about style so it's entirely up to you.  Outside of that
> > this looks straightforward to me, and the result of the series
> > is an improvement.
> > 
> > I'll let you comment on my suggestions before offering my
> > "reviewed-by" indication.
> > 
> > 					-Alex
> > 
> > > ---
> > >  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;
> > 
> > Not a big deal (and maybe it's not consistent with other nearby
> > style) but template and name_len could be defined inside the
> > "if (!firmware)" block.
> > 
> 
> I prefer variables declared in the beginning of the function, so I'm
> happy with this.
> 
> > > +	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);
> > 
> > 
> > I don't know if it would be an improvement, but you could
> > check for a null p value below for both cases.  I.e.:
> > 
> > 		if (p)
> > 			snprintf(p, ...);
> > 
> 
> Moving the common NULL check and return out seems nice, but given that
> we then have to have this positive conditional I think the end result is
> more complex.
> 
> That said, if we're not just doing a verbatim copy from rproc_alloc() I
> think we should make this function:
> 
> 	if (!firmware)
> 		p = kasprintf(GFP_KERNEL, "rproc-%s-fw", name);
> 	else
> 		p = kstrdup_const(firmware, GFP_KERNEL);

If you really want to use kstrdup_const() the return value has to be casted to a
"char *".  Variable 'p' can't be declared const "char *" because rproc->firmware is not
a "const".  Simply put somewhere the "const" will need to be dropped or casted out.

Mathieu

> 
> 	rproc->firmware = p;
> 
> 	return p ? 0 : -ENOMEM;
> 
> Regards,
> Bjorn
> 
> > (more below)
> > 
> > > +		if (!p)
> > > +			return -ENOMEM;
> > > +		snprintf(p, name_len, template, name);
> > > +	} else {
> > > +		p = kstrdup(firmware, GFP_KERNEL);
> > > +		if (!p)
> > > +			return -ENOMEM;
> > > +	}
> > > +
> > 
> > 	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 2/4] remoteproc: Split firmware name allocation from rproc_alloc()

[Bjorn Andersson]

On Tue 14 Apr 08:43 PDT 2020, Mathieu Poirier wrote:

> Hi guys,
> 
> On Mon, 13 Apr 2020 at 18:54, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > On Mon 13 Apr 13:56 PDT 2020, Alex Elder wrote:
> >
> > > On 4/13/20 2:33 PM, Mathieu Poirier wrote:
> > > > Make the firmware name allocation a function on its own in order to
> > > > introduce more flexibility to function rproc_alloc().
> > > >
> > > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > >
> > > I didn't look at the larger context (MCU series); I'm only looking
> > > at this (and the others in this series) in isolation.  I like
> > > that you're encapsulating this stuff into functions but doing so
> > > doesn't really add any flexibility.
> > >
> > > Two small suggestions for you to consider but they're truly
> > > more about style so it's entirely up to you.  Outside of that
> > > this looks straightforward to me, and the result of the series
> > > is an improvement.
> > >
> > > I'll let you comment on my suggestions before offering my
> > > "reviewed-by" indication.
> > >
> > >                                       -Alex
> > >
> > > > ---
> > > >  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;
> > >
> > > Not a big deal (and maybe it's not consistent with other nearby
> > > style) but template and name_len could be defined inside the
> > > "if (!firmware)" block.
> > >
> >
> > I prefer variables declared in the beginning of the function, so I'm
> > happy with this.
> >
> > > > +   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);
> > >
> > >
> > > I don't know if it would be an improvement, but you could
> > > check for a null p value below for both cases.  I.e.:
> > >
> > >               if (p)
> > >                       snprintf(p, ...);
> > >
> >
> > Moving the common NULL check and return out seems nice, but given that
> > we then have to have this positive conditional I think the end result is
> > more complex.
> >
> > That said, if we're not just doing a verbatim copy from rproc_alloc() I
> > think we should make this function:
> >
> >         if (!firmware)
> >                 p = kasprintf(GFP_KERNEL, "rproc-%s-fw", name);
> >         else
> >                 p = kstrdup_const(firmware, GFP_KERNEL);
> >
> >         rproc->firmware = p;
> >
> >         return p ? 0 : -ENOMEM;
> 
> At this time I was going for a pure re-arrangement of the code and
> avoiding further improvement.  This is simple enough that it can be
> rolled-in the next revision.
> 

The resulting patch would be "factor out AND rewrite", which generally
is good cause for splitting things in two patches...

Regards,
Bjorn

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

[Bjorn Andersson]

On Tue 14 Apr 12:44 PDT 2020, Mathieu Poirier wrote:

> Hey Bjorn,
> 
> On Mon, Apr 13, 2020 at 05:55:06PM -0700, Bjorn Andersson wrote:
> > On Mon 13 Apr 13:56 PDT 2020, Alex Elder wrote:
> > 
> > > On 4/13/20 2:33 PM, Mathieu Poirier wrote:
> > > > Make the firmware name allocation a function on its own in order to
> > > > introduce more flexibility to function rproc_alloc().
> > > > 
> > > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > > 
> > > I didn't look at the larger context (MCU series); I'm only looking
> > > at this (and the others in this series) in isolation.  I like
> > > that you're encapsulating this stuff into functions but doing so
> > > doesn't really add any flexibility.
> > > 
> > > Two small suggestions for you to consider but they're truly
> > > more about style so it's entirely up to you.  Outside of that
> > > this looks straightforward to me, and the result of the series
> > > is an improvement.
> > > 
> > > I'll let you comment on my suggestions before offering my
> > > "reviewed-by" indication.
> > > 
> > > 					-Alex
> > > 
> > > > ---
> > > >  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;
> > > 
> > > Not a big deal (and maybe it's not consistent with other nearby
> > > style) but template and name_len could be defined inside the
> > > "if (!firmware)" block.
> > > 
> > 
> > I prefer variables declared in the beginning of the function, so I'm
> > happy with this.
> > 
> > > > +	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);
> > > 
> > > 
> > > I don't know if it would be an improvement, but you could
> > > check for a null p value below for both cases.  I.e.:
> > > 
> > > 		if (p)
> > > 			snprintf(p, ...);
> > > 
> > 
> > Moving the common NULL check and return out seems nice, but given that
> > we then have to have this positive conditional I think the end result is
> > more complex.
> > 
> > That said, if we're not just doing a verbatim copy from rproc_alloc() I
> > think we should make this function:
> > 
> > 	if (!firmware)
> > 		p = kasprintf(GFP_KERNEL, "rproc-%s-fw", name);
> > 	else
> > 		p = kstrdup_const(firmware, GFP_KERNEL);
> 
> If you really want to use kstrdup_const() the return value has to be casted to a
> "char *".  Variable 'p' can't be declared const "char *" because rproc->firmware is not
> a "const".  Simply put somewhere the "const" will need to be dropped or casted out.
> 

The firmware parameter to rproc_alloc() is const char * and there's a
couple of places where a really const string is passed, so by using
kstrdup_const() we don't end up duplicating const data on the heap.

And afaict we can make both p and rproc->firmware const char * to allow
this, or am I missing something?

Regards,
Bjorn

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

[Mathieu Poirier]

On Tue, 14 Apr 2020 at 17:16, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Tue 14 Apr 12:44 PDT 2020, Mathieu Poirier wrote:
>
> > Hey Bjorn,
> >
> > On Mon, Apr 13, 2020 at 05:55:06PM -0700, Bjorn Andersson wrote:
> > > On Mon 13 Apr 13:56 PDT 2020, Alex Elder wrote:
> > >
> > > > On 4/13/20 2:33 PM, Mathieu Poirier wrote:
> > > > > Make the firmware name allocation a function on its own in order to
> > > > > introduce more flexibility to function rproc_alloc().
> > > > >
> > > > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > > >
> > > > I didn't look at the larger context (MCU series); I'm only looking
> > > > at this (and the others in this series) in isolation.  I like
> > > > that you're encapsulating this stuff into functions but doing so
> > > > doesn't really add any flexibility.
> > > >
> > > > Two small suggestions for you to consider but they're truly
> > > > more about style so it's entirely up to you.  Outside of that
> > > > this looks straightforward to me, and the result of the series
> > > > is an improvement.
> > > >
> > > > I'll let you comment on my suggestions before offering my
> > > > "reviewed-by" indication.
> > > >
> > > >                                   -Alex
> > > >
> > > > > ---
> > > > >  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;
> > > >
> > > > Not a big deal (and maybe it's not consistent with other nearby
> > > > style) but template and name_len could be defined inside the
> > > > "if (!firmware)" block.
> > > >
> > >
> > > I prefer variables declared in the beginning of the function, so I'm
> > > happy with this.
> > >
> > > > > +       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);
> > > >
> > > >
> > > > I don't know if it would be an improvement, but you could
> > > > check for a null p value below for both cases.  I.e.:
> > > >
> > > >           if (p)
> > > >                   snprintf(p, ...);
> > > >
> > >
> > > Moving the common NULL check and return out seems nice, but given that
> > > we then have to have this positive conditional I think the end result is
> > > more complex.
> > >
> > > That said, if we're not just doing a verbatim copy from rproc_alloc() I
> > > think we should make this function:
> > >
> > >     if (!firmware)
> > >             p = kasprintf(GFP_KERNEL, "rproc-%s-fw", name);
> > >     else
> > >             p = kstrdup_const(firmware, GFP_KERNEL);
> >
> > If you really want to use kstrdup_const() the return value has to be casted to a
> > "char *".  Variable 'p' can't be declared const "char *" because rproc->firmware is not
> > a "const".  Simply put somewhere the "const" will need to be dropped or casted out.
> >
>
> The firmware parameter to rproc_alloc() is const char * and there's a
> couple of places where a really const string is passed, so by using
> kstrdup_const() we don't end up duplicating const data on the heap.
>
> And afaict we can make both p and rproc->firmware const char * to allow
> this, or am I missing something?

I wasn't sure you were willing to go as far as making rproc->firmware
a const char *.  In that case it is quite easy...

>
> Regards,
> Bjorn