Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] remoteproc: rename "crashed" parameter
To:     Arnaud Pouliquen <arnaud.pouliquen@st.com>, 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 <elder@linaro.org>
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
Precedence: bulk

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);
>>  
>>