Re: [RFC v2 13/14] remoteproc: Add automation flags

[Arnaud POULIQUEN <arnaud.pouliquen@st.com>]

Hi Mathieu,

Thanks for initiating the discussion!

Waiting feedback from other, please find my feedback on our proposal below.

On 10/30/20 8:57 PM, Mathieu Poirier wrote:
> Adding flags to dictate how to handle a platform driver being removed
> or the remote processor crashing while in RPROC_ATTACHED state.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  drivers/remoteproc/remoteproc_core.c | 25 +++++++++++++++++++++++++
>  include/linux/remoteproc.h           |  5 +++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 229fa2cad0bd..d024367c63e5 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2227,6 +2227,29 @@ static int rproc_alloc_ops(struct rproc *rproc, const struct rproc_ops *ops)
>  	return 0;
>  }
>  
> +static void rproc_set_automation_flags(struct rproc *rproc)
> +{
> +	struct device *dev = rproc->dev.parent;
> +	struct device_node *np = dev->of_node;
> +	bool core_reboot, remote_crash;
> +
> +	/*
> +	 * When function rproc_cdev_release() or rproc_del() are called and
> +	 * the remote processor has been attached to, it will be detached from
> +	 * (rather than turned off) if "autonomous_on_core_reboot" is specified
> +	 * in the DT.
> +	 */
> +	core_reboot = of_property_read_bool(np, "autonomous_on_core_reboot");
> +	rproc->autonomous_on_core_reboot = core_reboot;
> +
> +	/*
> +	 * When the remote processor crashes it will be detached from, and
> +	 * attached to, if "autonomous_on_remote_crash" is specified in the DT.
> +	 */
> +	remote_crash = of_property_read_bool(np, "autonomous_on_remote_crash");
> +	rproc->autonomous_on_core_reboot = core_reboot;
> +}
> +

I wonder if the naming is not too restrictive.

I think here we probably need first to identify the use cases we want to support
to determine which use cases should be addressed and deduce DT fields.

Please find my view below:

1) Attach to a remote processor on boot.
This is the "attach" you introduced in a previous series. I wonder here if a DT
field should not be introduce for platform which are not able to dynamically
determines the remote processor state. Something like "remote-boot-on" or
"autonomous-boot-on".

2) Detach from a remote processor on Linux kernel shutdown
Two possible actions: shutdown the remote processor or detach from it.
A DT field could be used to determine the expected behavior.

3) Linux core reboot on crash
Two possible actions: shutdown and restart the remote processor or
detach/re-attach from/to it.
Is same DT field than 2) can be used for this . Or should be determine by a
new sysfs recovery option [1]?

[1]
https://elixir.bootlin.com/linux/v5.10-rc2/source/drivers/remoteproc/remoteproc_sysfs.c#L45

4) The remote processor need to reboot on crash.
3 possible actions:
 - shutdown and restart the remote processor
 - detach and re-attach from/to it.
 - Just shutdown, as no recovery possible without a system reset.

5) Detach/re-attach on Linux suspend/resume
Perhaps better to manage this in platform drivers without a generic DT field?

If i try to apply this on the remote proc boot and shutdown sequences:

1) on remoteproc device add:
- Need to determine if the remote processor is already running:
   - started by another entity
   - Linux reboot after crash

2) On remoteproc device release.
- Need to determine if the remote processor need to be shutdown or detached:
   - Linux kernel crash
   - Linux kernel graceful shutdown with remote processor keeping ON.

3) On remote processor crash
- Need to determine if the remote processor will be restarted by an external
entity or by the remoteproc framework, or if simply not possible to recover
without a system reset.

Regarding these use cases here is an alternative proposal(inspired by regulator
framework):
- "remote-boot-on": determine on probe if the remoteproc firmware is already
booted. This field is optional, use by a platform driver which can not
determine the state of the remote processor. Could be dynamically updated by the
platform driver to manage Kernel crash...

- "remote-always-on": means that the detach has to be privileged on
shutdown. Need also to be managed by platform driver as it can be
compared to the remote processor current state.

- "remoteproc-crash-recovery": crash recovery mode:
   possible value: "SHUTDOWN", "DETACH", "DISABLED"


Regards,
Arnaud

>  /**
>   * rproc_alloc() - allocate a remote processor handle
>   * @dev: the underlying device
> @@ -2285,6 +2308,8 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>  	if (rproc_alloc_ops(rproc, ops))
>  		goto put_device;
>  
> +	rproc_set_automation_flags(rproc);
> +
>  	/* Assign a unique device index and name */
>  	rproc->index = ida_simple_get(&rproc_dev_index, 0, 0, GFP_KERNEL);
>  	if (rproc->index < 0) {
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 71d4d4873164..9a6e79ef35d7 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -516,6 +516,9 @@ struct rproc_dump_segment {
>   * @nb_vdev: number of vdev currently handled by rproc
>   * @char_dev: character device of the rproc
>   * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
> + * @autonomous_on_core_reboot: true if the remote processor should be detached from
> + *			       (rather than turned off) when the remoteproc core
> + *			       goes away.
>   */
>  struct rproc {
>  	struct list_head node;
> @@ -554,6 +557,8 @@ struct rproc {
>  	u16 elf_machine;
>  	struct cdev cdev;
>  	bool cdev_put_on_release;
> +	bool autonomous_on_core_reboot	: 1,
> +	     autonomous_on_remote_crash	: 1;
>  };
>  
>  /**
>