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@xxxxxxxxxx> > --- > 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); > > -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
