remoteproc: rename "crashed" parameter

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 <[email protected]>

---
 drivers/remoteproc/remoteproc_core.c | 4 ++--
 include/linux/remoteproc.h           | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

drivers/remoteproc/remoteproc_core.c +2 -2

index 6d9c5832ce47..a9609d971f7f 100644
@@ -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;

include/linux/remoteproc.h +1 -1

index d09a9c7af109..dfdaede9139e 100644
@@ -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);
 

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

Quoted 79 lines · Alex Elder

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 <[email protected]>

---
 drivers/remoteproc/remoteproc_core.c | 4 ++--
 include/linux/remoteproc.h           | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

drivers/remoteproc/remoteproc_core.c +2 -2

index 6d9c5832ce47..a9609d971f7f 100644
@@ -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;

include/linux/remoteproc.h +1 -1

index d09a9c7af109..dfdaede9139e 100644
@@ -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);