[PATCH] remoteproc: rename "crashed" parameter

[PATCH] remoteproc: rename "crashed" parameter

[Alex Elder]

The last commit to "remoteproc_core.c":
  880f5b388252 remoteproc: Pass type of shutdown to subdev remove
added a Boolean flag to the subdevice remove method, to distinguish
between graceful shutdown and a crash.  Unfortunately, the names of
the parameters were inconsistent, and in fact have opposite meanings.

In most cases, the parameter is named "crashed", but rproc_add_subdev()
names the parameter "graceful" in the prototype for the remove method.

The remove method is ultimately called (and supplied with the Boolean
flag value) by rproc_remove_subdevices().  That is only called by
rproc_stop(), and in the two spots where that is used, "graceful" is
the right name for the flag:
  rproc_shutdown() passes true, indicating a graceful shutdown
  rproc_trigger_recovery() passes false, indicating a crash

The fix is to make the parameter name consistent, and making the
name and sense of the parameter to always be "crashed" produces the
smallest change.  So that's what this patch does.

To verify this change, rproc_add_subdev() is called in five spots:
  - qcom_add_glink_subdev() passes glink_subdev_remove()
  - qcom_add_smd_subdev() passes smd_subdev_remove()
  - qcom_add_ssr_subdev() passes ssr_notify_stop()
  - qcom_add_sysmon_subdev() passes sysmon_stop()
  - rproc_handle_vdev() passes rproc_vdev_do_remove()

Of these, only sysmon_stop() uses its "crashed" parameter.  And it
clearly assumes that "crashed" is the intended meaning:

        /* Don't request graceful shutdown if we've crashed */
        if (crashed)
                return;

So this function (added after the "crashed" parameter was added)
exhibited buggy behavior, which is now fixed by this patch.

Signed-off-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 6d9c5832ce47..a9609d971f7f 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1163,7 +1163,7 @@ int rproc_trigger_recovery(struct rproc *rproc)
 	if (ret)
 		return ret;
 
-	ret = rproc_stop(rproc, false);
+	ret = rproc_stop(rproc, true);
 	if (ret)
 		goto unlock_mutex;
 
@@ -1316,7 +1316,7 @@ void rproc_shutdown(struct rproc *rproc)
 	if (!atomic_dec_and_test(&rproc->power))
 		goto out;
 
-	ret = rproc_stop(rproc, true);
+	ret = rproc_stop(rproc, false);
 	if (ret) {
 		atomic_inc(&rproc->power);
 		goto out;
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index d09a9c7af109..dfdaede9139e 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -569,7 +569,7 @@ static inline struct rproc *vdev_to_rproc(struct virtio_device *vdev)
 void rproc_add_subdev(struct rproc *rproc,
 		      struct rproc_subdev *subdev,
 		      int (*probe)(struct rproc_subdev *subdev),
-		      void (*remove)(struct rproc_subdev *subdev, bool graceful));
+		      void (*remove)(struct rproc_subdev *subdev, bool crashed));
 
 void rproc_remove_subdev(struct rproc *rproc, struct rproc_subdev *subdev);
 
-- 
2.14.1

Re: [PATCH] remoteproc: rename "crashed" parameter

[Arnaud Pouliquen]

Hello Alex,

I have already proposed it few weeks ago.

https://lkml.org/lkml/2018/4/10/192

could you ack it, if you test it on your side?

Thanks,
Arnaud

On 04/25/2018 03:36 PM, Alex Elder wrote:
> The last commit to "remoteproc_core.c":
>   880f5b388252 remoteproc: eAPass type of shutdown to subdev remove
> added a Boolean flag to the subdevice remove method, to distinguish
> between graceful shutdown and a crash.  Unfortunately, the names of
> the parameters were inconsistent, and in fact have opposite meanings.
> 
> In most cases, the parameter is named "crashed", but rproc_add_subdev()
> names the parameter "graceful" in the prototype for the remove method.
> 
> The remove method is ultimately called (and supplied with the Boolean
> flag value) by rproc_remove_subdevices().  That is only called by
> rproc_stop(), and in the two spots where that is used, "graceful" is
> the right name for the flag:
>   rproc_shutdown() passes true, indicating a graceful shutdown
>   rproc_trigger_recovery() passes false, indicating a crash
> 
> The fix is to make the parameter name consistent, and making the
> name and sense of the parameter to always be "crashed" produces the
> smallest change.  So that's what this patch does.
> 
> To verify this change, rproc_add_subdev() is called in five spots:
>   - qcom_add_glink_subdev() passes glink_subdev_remove()
>   - qcom_add_smd_subdev() passes smd_subdev_remove()
>   - qcom_add_ssr_subdev() passes ssr_notify_stop()
>   - qcom_add_sysmon_subdev() passes sysmon_stop()
>   - rproc_handle_vdev() passes rproc_vdev_do_remove()
> 
> Of these, only sysmon_stop() uses its "crashed" parameter.  And it
> clearly assumes that "crashed" is the intended meaning:
> 
>         /* Don't request graceful shutdown if we've crashed */
>         if (crashed)
>                 return;
> 
> So this function (added after the "crashed" parameter was added)
> exhibited buggy behavior, which is now fixed by this patch.
> 
> Signed-off-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 6d9c5832ce47..a9609d971f7f 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1163,7 +1163,7 @@ int rproc_trigger_recovery(struct rproc *rproc)
>  	if (ret)
>  		return ret;
>  
> -	ret = rproc_stop(rproc, false);
> +	ret = rproc_stop(rproc, true);
>  	if (ret)
>  		goto unlock_mutex;
>  
> @@ -1316,7 +1316,7 @@ void rproc_shutdown(struct rproc *rproc)
>  	if (!atomic_dec_and_test(&rproc->power))
>  		goto out;
>  
> -	ret = rproc_stop(rproc, true);
> +	ret = rproc_stop(rproc, false);
>  	if (ret) {
>  		atomic_inc(&rproc->power);
>  		goto out;
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index d09a9c7af109..dfdaede9139e 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -569,7 +569,7 @@ static inline struct rproc *vdev_to_rproc(struct virtio_device *vdev)
>  void rproc_add_subdev(struct rproc *rproc,
>  		      struct rproc_subdev *subdev,
>  		      int (*probe)(struct rproc_subdev *subdev),
> -		      void (*remove)(struct rproc_subdev *subdev, bool graceful));
> +		      void (*remove)(struct rproc_subdev *subdev, bool crashed));
>  
>  void rproc_remove_subdev(struct rproc *rproc, struct rproc_subdev *subdev);
>  
> 

Re: [PATCH] remoteproc: rename "crashed" parameter

[Alex Elder]

On 04/25/2018 08:49 AM, Arnaud Pouliquen wrote:
> Hello Alex,
> 
> I have already proposed it few weeks ago.
> 
> https://lkml.org/lkml/2018/4/10/192
> 
> could you ack it, if you test it on your side?

I am unable to test it.  I found the bug by inspection (perhaps as
you did).  However I'll give you:

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

or 

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

					-Alex

> Thanks,
> Arnaud
> 
> On 04/25/2018 03:36 PM, Alex Elder wrote:
>> The last commit to "remoteproc_core.c":
>>   880f5b388252 remoteproc: eAPass type of shutdown to subdev remove
>> added a Boolean flag to the subdevice remove method, to distinguish
>> between graceful shutdown and a crash.  Unfortunately, the names of
>> the parameters were inconsistent, and in fact have opposite meanings.
>>
>> In most cases, the parameter is named "crashed", but rproc_add_subdev()
>> names the parameter "graceful" in the prototype for the remove method.
>>
>> The remove method is ultimately called (and supplied with the Boolean
>> flag value) by rproc_remove_subdevices().  That is only called by
>> rproc_stop(), and in the two spots where that is used, "graceful" is
>> the right name for the flag:
>>   rproc_shutdown() passes true, indicating a graceful shutdown
>>   rproc_trigger_recovery() passes false, indicating a crash
>>
>> The fix is to make the parameter name consistent, and making the
>> name and sense of the parameter to always be "crashed" produces the
>> smallest change.  So that's what this patch does.
>>
>> To verify this change, rproc_add_subdev() is called in five spots:
>>   - qcom_add_glink_subdev() passes glink_subdev_remove()
>>   - qcom_add_smd_subdev() passes smd_subdev_remove()
>>   - qcom_add_ssr_subdev() passes ssr_notify_stop()
>>   - qcom_add_sysmon_subdev() passes sysmon_stop()
>>   - rproc_handle_vdev() passes rproc_vdev_do_remove()
>>
>> Of these, only sysmon_stop() uses its "crashed" parameter.  And it
>> clearly assumes that "crashed" is the intended meaning:
>>
>>         /* Don't request graceful shutdown if we've crashed */
>>         if (crashed)
>>                 return;
>>
>> So this function (added after the "crashed" parameter was added)
>> exhibited buggy behavior, which is now fixed by this patch.
>>
>> Signed-off-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 6d9c5832ce47..a9609d971f7f 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1163,7 +1163,7 @@ int rproc_trigger_recovery(struct rproc *rproc)
>>  	if (ret)
>>  		return ret;
>>  
>> -	ret = rproc_stop(rproc, false);
>> +	ret = rproc_stop(rproc, true);
>>  	if (ret)
>>  		goto unlock_mutex;
>>  
>> @@ -1316,7 +1316,7 @@ void rproc_shutdown(struct rproc *rproc)
>>  	if (!atomic_dec_and_test(&rproc->power))
>>  		goto out;
>>  
>> -	ret = rproc_stop(rproc, true);
>> +	ret = rproc_stop(rproc, false);
>>  	if (ret) {
>>  		atomic_inc(&rproc->power);
>>  		goto out;
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index d09a9c7af109..dfdaede9139e 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -569,7 +569,7 @@ static inline struct rproc *vdev_to_rproc(struct virtio_device *vdev)
>>  void rproc_add_subdev(struct rproc *rproc,
>>  		      struct rproc_subdev *subdev,
>>  		      int (*probe)(struct rproc_subdev *subdev),
>> -		      void (*remove)(struct rproc_subdev *subdev, bool graceful));
>> +		      void (*remove)(struct rproc_subdev *subdev, bool crashed));
>>  
>>  void rproc_remove_subdev(struct rproc *rproc, struct rproc_subdev *subdev);
>>  
>>