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@xxxxxxxxxx>
>>> ---
>>> 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.
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
>>
