diff mbox series

remoteproc: Reset table_ptr in rproc_start() failure paths

Message ID 20180727011535.21729-1-s-anna@ti.com
State Accepted
Commit f68d51bd8a7141ba84f06e6207197817398e7f3a
Headers show
Series remoteproc: Reset table_ptr in rproc_start() failure paths | expand

Commit Message

Suman Anna July 27, 2018, 1:15 a.m. UTC
Unwind the modified table_ptr and restore it to the local copy
upon any subsequent failures in the rproc_start() function. This
keeps the function to remain balanced on failures without the need
to balance any modified variables elsewhere.

While at this, do some minor cleanup of the extra lines between
the failure labels as well.

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

---
 drivers/remoteproc/remoteproc_core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

-- 
2.18.0

Comments

Bjorn Andersson July 31, 2018, 4:15 a.m. UTC | #1
On Thu 26 Jul 18:15 PDT 2018, Suman Anna wrote:

> Unwind the modified table_ptr and restore it to the local copy

> upon any subsequent failures in the rproc_start() function. This

> keeps the function to remain balanced on failures without the need

> to balance any modified variables elsewhere.

> 


Good catch.

> While at this, do some minor cleanup of the extra lines between

> the failure labels as well.

> 

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

> ---

>  drivers/remoteproc/remoteproc_core.c | 7 ++++---

>  1 file changed, 4 insertions(+), 3 deletions(-)

> 

> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c

> index eadff6ce2f7f..afef2d491c5b 100644

> --- a/drivers/remoteproc/remoteproc_core.c

> +++ b/drivers/remoteproc/remoteproc_core.c

> @@ -953,7 +953,7 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)

>  	if (ret) {

>  		dev_err(dev, "failed to prepare subdevices for %s: %d\n",

>  			rproc->name, ret);

> -		return ret;

> +		goto reset_table_ptr;

>  	}

>  

>  	/* power up the remote processor */

> @@ -979,10 +979,11 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)

>  

>  stop_rproc:

>  	rproc->ops->stop(rproc);

> -

>  unprepare_subdevices:

>  	rproc_unprepare_subdevices(rproc);

> -

> +reset_table_ptr:

> +	if (loaded_table)


Regardless of us having a loaded_table it should have the same value as
cached_table when we return - which might be NULL if we don't have a
resource table.

So I applied this without the conditional, please object if I missed
something.

Regards,
Bjorn

> +		rproc->table_ptr = rproc->cached_table;

>  	return ret;

>  }

>  

> -- 

> 2.18.0

>
Suman Anna July 31, 2018, 2:49 p.m. UTC | #2
On 07/30/2018 11:15 PM, Bjorn Andersson wrote:
> On Thu 26 Jul 18:15 PDT 2018, Suman Anna wrote:

> 

>> Unwind the modified table_ptr and restore it to the local copy

>> upon any subsequent failures in the rproc_start() function. This

>> keeps the function to remain balanced on failures without the need

>> to balance any modified variables elsewhere.

>>

> 

> Good catch.

> 

>> While at this, do some minor cleanup of the extra lines between

>> the failure labels as well.

>>

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

>> ---

>>  drivers/remoteproc/remoteproc_core.c | 7 ++++---

>>  1 file changed, 4 insertions(+), 3 deletions(-)

>>

>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c

>> index eadff6ce2f7f..afef2d491c5b 100644

>> --- a/drivers/remoteproc/remoteproc_core.c

>> +++ b/drivers/remoteproc/remoteproc_core.c

>> @@ -953,7 +953,7 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)

>>  	if (ret) {

>>  		dev_err(dev, "failed to prepare subdevices for %s: %d\n",

>>  			rproc->name, ret);

>> -		return ret;

>> +		goto reset_table_ptr;

>>  	}

>>  

>>  	/* power up the remote processor */

>> @@ -979,10 +979,11 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)

>>  

>>  stop_rproc:

>>  	rproc->ops->stop(rproc);

>> -

>>  unprepare_subdevices:

>>  	rproc_unprepare_subdevices(rproc);

>> -

>> +reset_table_ptr:

>> +	if (loaded_table)

> 

> Regardless of us having a loaded_table it should have the same value as

> cached_table when we return - which might be NULL if we don't have a

> resource table.

> 

> So I applied this without the conditional, please object if I missed

> something.


Yeah, that's fine. I added the conditional only to keep it symmetric,
it gets modified only if loaded_table was non-NULL in the first place,
and is pointing to the cached_table otherwise.

regards
Suman

> 

> Regards,

> Bjorn

> 

>> +		rproc->table_ptr = rproc->cached_table;

>>  	return ret;

>>  }

>>  

>> -- 

>> 2.18.0

>>
diff mbox series

Patch

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index eadff6ce2f7f..afef2d491c5b 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -953,7 +953,7 @@  static int rproc_start(struct rproc *rproc, const struct firmware *fw)
 	if (ret) {
 		dev_err(dev, "failed to prepare subdevices for %s: %d\n",
 			rproc->name, ret);
-		return ret;
+		goto reset_table_ptr;
 	}
 
 	/* power up the remote processor */
@@ -979,10 +979,11 @@  static int rproc_start(struct rproc *rproc, const struct firmware *fw)
 
 stop_rproc:
 	rproc->ops->stop(rproc);
-
 unprepare_subdevices:
 	rproc_unprepare_subdevices(rproc);
-
+reset_table_ptr:
+	if (loaded_table)
+		rproc->table_ptr = rproc->cached_table;
 	return ret;
 }