Hey guys,
On Fri, May 28, 2021 at 11:40:02AM -0500, Suman Anna wrote:
> Hi Bjorn,
>
> On 5/27/21 11:11 PM, Bjorn Andersson wrote:
> > On Fri 21 May 19:03 CDT 2021, Suman Anna wrote:
> >
> >> The remoteproc core has support for both stopping and detaching a
> >> remote processor that was attached to previously, through both the
> >> remoteproc sysfs and cdev interfaces. The rproc_shutdown() though
> >> unconditionally only uses the stop functionality at present. This
> >> may not be the default desired functionality for all the remoteproc
> >> platform drivers.
> >>
> >> Introduce a new rproc state flag 'detach_on_shutdown' that individual
> >> remoteproc drivers can set to only allow detach in rproc_shutdown()
> >> that would have been invoked when the driver is uninstalled, so that
> >> remote processor continues to run undisturbed even after the driver
> >> removal.
> >>
> >
> > I dislike the introduction of knobs for everything and would much rather
> > see that we define some sound defaults. Can we make shutdown just do
> > detach() if that's supported otherwise stop().
> >
>
> I maybe missing your point, but the change in remoteproc_core below exactly does
> that, right? Are you saying drop the checks in remoteproc_cdev and remoteproc_sysfs?
>
> The asymmetry did bug me as well, but it is already existing even before this
> patch. I personally would have preferred a cleaner and symmetrical attach,
> start, stop, detach, but existing code has overloaded attach into start (keys
> off by RPROC_OFFLINE/RPROC_DETACHED) while introducing a separate detach from
> stop. I have retained the meaning of stop as shutdown from userspace interface
> perspective, but enforcing the checks for detach only remoteprocs.
>
> The logic in rproc_shutdown is for driver paths.
>
> > This still allows userspace to explicitly stop the detachable remoteproc
> > before shutdown, if for some reason that's what you want...
>
> This is the existing behavior and the difference between stop and detach. That
> behavior is maintained for remoteprocs not setting the detach_on_shutdown flag.
> I am only restricting the behavior for those that set it.
>
> Mathieu,
> Your thoughts on this?
Introducing knobs in such a way makes the code very difficult to understand and
maintain. It is also a matter of time before another knob is introduced to
modify the behavior of this knob.
Function rproc_detach() is exported and should be used in the platform driver if
the state of the remote processor mandates it. Function rproc_del() calls
rproc_shutdown() but the latter will return immediately because of rproc->power.
So calling rproc_detach() followed by rproc_del() will work as expected. The
real fix is to de-couple rproc_shutdown from rproc_del() and do the right calls
in the platform drivers using them.
With regards to rproc_cdev_write(), the state of the remote processor is
advertised in sysfs. As such it should be easy to write "stop" or "detach" to
the character interface. If a command to stop the remote processor is not
supported in a scenario then rproc_ops::stop should reflect that. If that is
the case then rproc_shutdown() should be modified to return an error code, the
same way rproc_detach() was done.
>
> regards
> Suman
>
>
>
> >
> > Regards,
> > Bjorn
> >
> >> Signed-off-by: Suman Anna <s-anna@xxxxxx>
> >> ---
> >> drivers/remoteproc/remoteproc_cdev.c | 7 +++++++
> >> drivers/remoteproc/remoteproc_core.c | 5 ++++-
> >> drivers/remoteproc/remoteproc_sysfs.c | 6 ++++++
> >> include/linux/remoteproc.h | 3 +++
> >> 4 files changed, 20 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> >> index 0b8a84c04f76..473467711a09 100644
> >> --- a/drivers/remoteproc/remoteproc_cdev.c
> >> +++ b/drivers/remoteproc/remoteproc_cdev.c
> >> @@ -42,6 +42,13 @@ static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_
> >> rproc->state != RPROC_ATTACHED)
> >> return -EINVAL;
> >>
> >> + if (rproc->state == RPROC_ATTACHED &&
> >> + rproc->detach_on_shutdown) {
> >> + dev_err(&rproc->dev,
> >> + "stop not supported for this rproc, use detach\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> rproc_shutdown(rproc);
> >> } else if (!strncmp(cmd, "detach", len)) {
> >> if (rproc->state != RPROC_ATTACHED)
> >> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> >> index 6019f46001c8..e8ab3eb41f00 100644
> >> --- a/drivers/remoteproc/remoteproc_core.c
> >> +++ b/drivers/remoteproc/remoteproc_core.c
> >> @@ -2074,7 +2074,10 @@ void rproc_shutdown(struct rproc *rproc)
> >> if (!atomic_dec_and_test(&rproc->power))
> >> goto out;
> >>
> >> - ret = rproc_stop(rproc, false);
> >> + if (rproc->detach_on_shutdown && rproc->state == RPROC_ATTACHED)
> >> + ret = __rproc_detach(rproc);
> >> + else
> >> + ret = rproc_stop(rproc, false);
> >> if (ret) {
> >> atomic_inc(&rproc->power);
> >> goto out;
> >> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> >> index ea8b89f97d7b..1785fbcb1075 100644
> >> --- a/drivers/remoteproc/remoteproc_sysfs.c
> >> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> >> @@ -206,6 +206,12 @@ static ssize_t state_store(struct device *dev,
> >> rproc->state != RPROC_ATTACHED)
> >> return -EINVAL;
> >>
> >> + if (rproc->state == RPROC_ATTACHED &&
> >> + rproc->detach_on_shutdown) {
> >> + dev_err(&rproc->dev, "stop not supported for this rproc, use detach\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> rproc_shutdown(rproc);
> >> } else if (sysfs_streq(buf, "detach")) {
> >> if (rproc->state != RPROC_ATTACHED)
> >> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> >> index 42a1f30e33a7..35ef921676a1 100644
> >> --- a/include/linux/remoteproc.h
> >> +++ b/include/linux/remoteproc.h
> >> @@ -530,6 +530,8 @@ struct rproc_dump_segment {
> >> * @elf_machine: firmware ELF machine
> >> * @cdev: character device of the rproc
> >> * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
> >> + * @detach_on_shutdown: flag to indicate if remoteproc cannot be shutdown in
> >> + * attached state and _only_ support detach
> >> */
> >> struct rproc {
> >> struct list_head node;
> >> @@ -569,6 +571,7 @@ struct rproc {
> >> u16 elf_machine;
> >> struct cdev cdev;
> >> bool cdev_put_on_release;
> >> + bool detach_on_shutdown;
> >> };
> >>
> >> /**
> >> --
> >> 2.30.1
> >>
>
