Re: [PATCH v2 15/17] remoteproc: Correctly deal with MCU synchronisation when changing FW image

[Suman Anna <s-anna@ti.com>]

On 4/22/20 4:29 PM, Mathieu Poirier wrote:
> Hi Suman,
> 
> On Tue, Mar 31, 2020 at 05:14:18PM -0500, Suman Anna wrote:
>> Hi Mathieu,
>>
>> On 3/30/20 6:21 PM, Mathieu Poirier wrote:
>>> On Fri, Mar 27, 2020 at 01:50:18PM +0000, Loic PALLARDY wrote:
>>>>
>>>>> This patch prevents the firmware image from being displayed or changed
>>>>> when
>>>>> the remoteproc core is synchronising with an MCU. This is needed since
>>>>> there is no guarantee about the nature of the firmware image that is loaded
>>>>> by the external entity.
>>>>>
>>>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>>> ---
>>>>>   drivers/remoteproc/remoteproc_sysfs.c | 25
>>>>> ++++++++++++++++++++++++-
>>>>>   1 file changed, 24 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/remoteproc/remoteproc_sysfs.c
>>>>> b/drivers/remoteproc/remoteproc_sysfs.c
>>>>> index 7f8536b73295..4956577ad4b4 100644
>>>>> --- a/drivers/remoteproc/remoteproc_sysfs.c
>>>>> +++ b/drivers/remoteproc/remoteproc_sysfs.c
>>>>> @@ -13,9 +13,20 @@
>>>>>   static ssize_t firmware_show(struct device *dev, struct device_attribute
>>>>> *attr,
>>>>>   			  char *buf)
>>>>>   {
>>>>> +	ssize_t ret;
>>>>>   	struct rproc *rproc = to_rproc(dev);
>>>>>
>>>>> -	return sprintf(buf, "%s\n", rproc->firmware);
>>>>> +	/*
>>>>> +	 * In most instances there is no guarantee about the firmware
>>>>> +	 * that was loaded by the external entity.  As such simply don't
>>>>> +	 * print anything.
>>>>> +	 */
>>>>> +	if (rproc_sync_with_mcu(rproc))
>>>>> +		ret = sprintf(buf, "\n");
>>>> Is it enough to provide empty name, or should we add a message to indicate that's name is unkown/undefined ?
>>>>
>>>
>>> Don't know... It is easy to find plenty of cases in sysfs where null values are
>>> represented with a "\n", and just as many where "unknown", "undefined" or "-1"
>>> are used. I know GKH prefers the least amount of information as possible, hence
>>> going with a "\n".
>>>
>>> Again, no strong opinion...
>>>
>>>> Regards,
>>>> Loic
>>>>> +	else
>>>>> +		ret = sprintf(buf, "%s\n", rproc->firmware);
>>>>> +
>>>>> +	return ret;
>>>>>   }
>>>>>
>>>>>   /* Change firmware name via sysfs */
>>>>> @@ -33,6 +44,18 @@ static ssize_t firmware_store(struct device *dev,
>>>>>   		return -EINVAL;
>>>>>   	}
>>>>>
>>>>> +	/*
>>>>> +	 * There is no point in trying to change the firmware if the MCU
>>>>> +	 * is currently running or if loading of the image is done by
>>>>> +	 * another entity.
>>>>> +	 */
>>>>> +	if (rproc_sync_with_mcu(rproc)) {
>>>>> +		dev_err(dev,
>>>>> +			"can't change firmware while synchronising with
>>>>> MCU\n");
>>>>> +		err = -EBUSY;
>>>>> +		goto out;
>>>>> +	}
>>>>> +
>>
>> So, I have done a patch sometime back to deny sysfs operations [1] (the
>> primary usecase is for a rproc-client driver driven boot where auto-boot
>> is not set) which is still a need for me. Do you see that as orthogonal
>> to that, or can we leverage that here somehow. I cannot use the sync_
>> conditions for my cases since they are not already booted before.
> 
> No matter how much I try to fit the functionality provided by the
> "deny_sysfs_ops" flag in the patch you pointed out, I just can't come up with
> something I am happy with.
> 
> The only thing the topics of remote processor synchronisation and kernel
> initiated remote processor boot have in common is preventing sysfs access under
> specific circumstances. As such it is probably best to keep their implemenation
> seperated for the time being.

Yeah, OK, we can revisit this. We already have an in-kernel user 
wkup_m3_ipc driver that controls the boot and shutdown of the wkup_m3 
rproc driver, which is used for SoC Power Management, and so we really 
do not want any control from userspace for that. I will have the same 
need for the PRU remoteproc client usage perspective as well.

regards
Suman

> 
> Thanks,
> Mathieu
> 
>>
>> Also, any reason why you want to do this check before the rproc->state
>> unlike the logic around the 'state' file in the next patch?
>>
>> [1] https://patchwork.kernel.org/patch/10601325/
>>
>> regards
>> Suman
>>
>>>>>   	if (rproc->state != RPROC_OFFLINE) {
>>>>>   		dev_err(dev, "can't change firmware while running\n");
>>>>>   		err = -EBUSY;
>>>>> --
>>>>> 2.20.1
>>>>
>>