Re: [PATCH v2 1/1] remoteproc: core: probe subdevices before booting coprocessor

From: loic pallardy <loic.pallardy@st.com>
To: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: ohad@wizery.com, lee.jones@linaro.org, patrice.chotard@st.com,
	linux-remoteproc@vger.kernel.org, kernel@stlinux.com
Subject: Re: [PATCH v2 1/1] remoteproc: core: probe subdevices before booting coprocessor
Date: Tue, 13 Dec 2016 18:22:58 +0100	[thread overview]
Message-ID: <aa670a73-757e-9b7c-3fcf-cef5d4e1acc2@st.com> (raw)
In-Reply-To: <df027c54-fb48-da31-752b-5b551c698f8f@st.com>

On 12/11/2016 09:30 PM, loic pallardy wrote:
>
>
> On 12/08/2016 12:23 AM, Bjorn Andersson wrote:
>> On Wed 07 Dec 12:49 PST 2016, Loic Pallardy wrote:
>>
>>> With subdevice support introduction, coprocessor boot sequence has
>>> changed. Related coprocessor subdevices are now starting after firmware
>>> and resource table loading and coprocessor boot.
>>>
[..]
>>
>>
>> I still believe that resources that are so strictly tied to the state of
>> the firmware should be declared as part of the remoteproc, not
>> registered by other devices that might or might not finish their
>> registration in time - unless those resources can be associated
>> to the remote dynamically.
> Yes please drop out this patch. I'll work on firmware side to be more
> flexible for initialization sequence.
>
Hi Bjorn,

Reworking coprocessor boot sequence to guarantee struct fw_rsc_vdev of 
resource table is accessed only after first kick generated by 
virtio_rpmsg, I found the issue in coming from a patch present in 
remoteproc pull request for v4.10 [1]: remoteproc: Merge table_ptr and 
cached_table pointers [2]

This patch disables the capability of rproc subdev to modifiy 
dynamically the resource table after coprocessor boot. rproc_virtio_set 
function has no more effect after rproc_boot.

Patch above was not fixing the right issue but just masking it...
Reverting patch [2] fix it.

As subdevices are booted after rproc (and I agree with your arguments 
mentioned previously), dynamic resource table modifications should be 
possible (of course HW sync between host and rproc is required for rsc 
access).

Moreover reading patch [2] commit header, I think that security memory 
lock down should be an option and not the default. If memory region has 
to be locked down, I agree that all resource table content will be fixed 
and no dynamic modification possible. But that's not valid all today.
May be a DT properties will help to manage both configurations? And 
adding a warning in rproc_virtio_set function when rsc table in more 
accessible will be nice too.

Regards,
Loic

[1] https://lkml.org/lkml/2016/12/12/661 : remoteproc: Merge table_ptr 
and cached_table pointers
[2] http://www.spinics.net/lists/linux-remoteproc/msg00717.html

> Regards,
> Loic
>>
>> Regards,
>> Bjorn
>>
>>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>>> ---
>>> v2: remove unused stop_rproc label
>>>
>>>  drivers/remoteproc/remoteproc_core.c | 18 ++++++++----------
>>>  1 file changed, 8 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/remoteproc/remoteproc_core.c
>>> b/drivers/remoteproc/remoteproc_core.c
>>> index f0f6ec1..61f06a7 100644
>>> --- a/drivers/remoteproc/remoteproc_core.c
>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>> @@ -913,6 +913,14 @@ static int rproc_fw_boot(struct rproc *rproc,
>>> const struct firmware *fw)
>>>          goto clean_up_resources;
>>>      }
>>>
>>> +    /* probe any subdevices for the remote processor */
>>> +    ret = rproc_probe_subdevices(rproc);
>>> +    if (ret) {
>>> +        dev_err(dev, "failed to probe subdevices for %s: %d\n",
>>> +            rproc->name, ret);
>>> +        goto clean_up_resources;
>>> +    }
>>> +
>>>      /*
>>>       * The starting device has been given the rproc->table_ptr as the
>>>       * resource table. The address of the vring along with the other
>>> @@ -932,22 +940,12 @@ static int rproc_fw_boot(struct rproc *rproc,
>>> const struct firmware *fw)
>>>          goto clean_up_resources;
>>>      }
>>>
>>> -    /* probe any subdevices for the remote processor */
>>> -    ret = rproc_probe_subdevices(rproc);
>>> -    if (ret) {
>>> -        dev_err(dev, "failed to probe subdevices for %s: %d\n",
>>> -            rproc->name, ret);
>>> -        goto stop_rproc;
>>> -    }
>>> -
>>>      rproc->state = RPROC_RUNNING;
>>>
>>>      dev_info(dev, "remote processor %s is now up\n", rproc->name);
>>>
>>>      return 0;
>>>
>>> -stop_rproc:
>>> -    rproc->ops->stop(rproc);
>>>  clean_up_resources:
>>>      rproc_resource_cleanup(rproc);
>>>  clean_up:
>>> --
>>> 1.9.1
>>>