On 4/30/20 10:23 PM, Mathieu Poirier wrote: > On Wed, Apr 29, 2020 at 10:19:49AM +0200, Arnaud POULIQUEN wrote: >> >> >> On 4/24/20 10:01 PM, Mathieu Poirier wrote: >>> The remoteproc core must not allow function rproc_shutdown() to >>> proceed if currently synchronising with a remote processor and >>> the synchronisation operations of that remote processor does not >>> support it. Also part of the process is to set the synchronisation >>> flag so that the remoteproc core can make the right decisions when >>> restarting the system. >>> >>> Signed-off-by: Mathieu Poirier <[email protected]> >>> --- >>> drivers/remoteproc/remoteproc_core.c | 32 ++++++++++++++++++++++++ >>> drivers/remoteproc/remoteproc_internal.h | 7 ++++++ >>> 2 files changed, 39 insertions(+) >>> >>> diff --git a/drivers/remoteproc/remoteproc_core.c >>> b/drivers/remoteproc/remoteproc_core.c >>> index 3a84a38ba37b..48afa1f80a8f 100644 >>> --- a/drivers/remoteproc/remoteproc_core.c >>> +++ b/drivers/remoteproc/remoteproc_core.c >>> @@ -1849,6 +1849,27 @@ int rproc_boot(struct rproc *rproc) >>> } >>> EXPORT_SYMBOL(rproc_boot); >>> >>> +static bool rproc_can_shutdown(struct rproc *rproc) >>> +{ >>> + /* >>> + * The remoteproc core is the lifecycle manager, no problem >>> + * calling for a shutdown. >>> + */ >>> + if (!rproc_needs_syncing(rproc)) >>> + return true; >>> + >>> + /* >>> + * The remoteproc has been loaded by another entity (as per above >>> + * condition) and the platform code has given us the capability >>> + * of stopping it. >>> + */ >>> + if (rproc->sync_ops->stop) >>> + return true; >> >> This means that if rproc->sync_ops->stop is null rproc_stop_subdevices will >> not >> be called? seems not symmetric with the start sequence. > > If rproc->sync_ops->stop is not provided then the remoteproc core can't stop > the > remote processor at all after it has synchronised with it. If a usecase > requires some kind of soft reset then a stop() function that uses a mailbox > notification or some other mechanism can be provided to tell the remote > processor to put itself back in startup mode again. > > Is this fine with you or there is still something I don't get? My point here is more around the subdevices. But perhaps i missed something... In rproc_start rproc_start_subdevices is called, even if sync_start is null. But in rproc_shutdown rproc_stop is not called, if sync_ops->stop is null. So rproc_stop_subdevices is not called in this case. Then if sync_flags.after_stop is false, it looks like that something will go wrong at next start. > >> Probably not useful to test it here as condition is already handled in >> rproc_stop_device... >> >> Regards >> Arnaud >>> + >>> + /* Any other condition should not be allowed */ >>> + return false; >>> +} >>> + >>> /** >>> * rproc_shutdown() - power off the remote processor >>> * @rproc: the remote processor >>> @@ -1879,6 +1900,9 @@ void rproc_shutdown(struct rproc *rproc) >>> return; >>> } >>> >>> + if (!rproc_can_shutdown(rproc)) >>> + goto out; >>> + >>> /* if the remote proc is still needed, bail out */ >>> if (!atomic_dec_and_test(&rproc->power)) >>> goto out; >>> @@ -1898,6 +1922,14 @@ void rproc_shutdown(struct rproc *rproc) >>> kfree(rproc->cached_table); >>> rproc->cached_table = NULL; >>> rproc->table_ptr = NULL; >>> + >>> + /* >>> + * The remote processor has been switched off - tell the core what >>> + * operation to use from hereon, i.e whether an external entity will >>> + * reboot the remote processor or it is now the remoteproc core's >>> + * responsability. >>> + */ >>> + rproc_set_sync_flag(rproc, RPROC_SYNC_STATE_SHUTDOWN); >>> out: >>> mutex_unlock(&rproc->lock); >>> } >>> diff --git a/drivers/remoteproc/remoteproc_internal.h >>> b/drivers/remoteproc/remoteproc_internal.h >>> index 61500981155c..7dcc0a26892b 100644 >>> --- a/drivers/remoteproc/remoteproc_internal.h >>> +++ b/drivers/remoteproc/remoteproc_internal.h >>> @@ -27,6 +27,9 @@ struct rproc_debug_trace { >>> /* >>> * enum rproc_sync_states - remote processsor sync states >>> * >>> + * @RPROC_SYNC_STATE_SHUTDOWN state to use after the remoteproc core >>> + * has shutdown (rproc_shutdown()) the >>> + * remote processor. >>> * @RPROC_SYNC_STATE_CRASHED state to use after the remote processor >>> * has crashed but has not been recovered by >>> * the remoteproc core yet. >>> @@ -36,6 +39,7 @@ struct rproc_debug_trace { >>> * operation to use. >>> */ >>> enum rproc_sync_states { >>> + RPROC_SYNC_STATE_SHUTDOWN, >>> RPROC_SYNC_STATE_CRASHED, >>> }; >>> >>> @@ -43,6 +47,9 @@ static inline void rproc_set_sync_flag(struct rproc >>> *rproc, >>> enum rproc_sync_states state) >>> { >>> switch (state) { >>> + case RPROC_SYNC_STATE_SHUTDOWN: >>> + rproc->sync_with_rproc = rproc->sync_flags.after_stop; >>> + break; >>> case RPROC_SYNC_STATE_CRASHED: >>> rproc->sync_with_rproc = rproc->sync_flags.after_crash; >>> break; >>>
