From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754783AbeDYOru (ORCPT ); Wed, 25 Apr 2018 10:47:50 -0400 Received: from mail-it0-f67.google.com ([209.85.214.67]:54015 "EHLO mail-it0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754203AbeDYOrs (ORCPT ); Wed, 25 Apr 2018 10:47:48 -0400 X-Google-Smtp-Source: AB8JxZqoWjXxLYfP6SRIjwIbOiV5D06NdAuUkbhDsoeVimgvZakpq7fjL+PteNaw6X67GyXUJq1LVg== Subject: Re: [PATCH] remoteproc: rename "crashed" parameter To: Arnaud Pouliquen , ohad@wizery.com, bjorn.andersson@linaro.org Cc: linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org References: <20180425133626.11781-1-elder@linaro.org> <93951de4-3758-0293-eeb1-bba9dd561c4b@st.com> From: Alex Elder Message-ID: <31b8864a-d94e-64cf-40dd-d03d40963513@linaro.org> Date: Wed, 25 Apr 2018 09:47:46 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <93951de4-3758-0293-eeb1-bba9dd561c4b@st.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 or Reviewed-by: Alex Elder -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 >> --- >> 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); >> >>