From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [PATCH v2 1/1] remoteproc: core: probe subdevices before booting coprocessor References: <1481143768-16377-1-git-send-email-loic.pallardy@st.com> <20161207232332.GJ30492@tuxbot> From: loic pallardy Message-ID: Date: Tue, 13 Dec 2016 18:22:58 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit To: Bjorn Andersson Cc: ohad@wizery.com, lee.jones@linaro.org, patrice.chotard@st.com, linux-remoteproc@vger.kernel.org, kernel@stlinux.com List-ID: 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 >>> --- >>> 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 >>>