Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 1 Jun 2021 11:15:08 -0600
From:   Mathieu Poirier <mathieu.poirier@linaro.org>
To:     Suman Anna <s-anna@ti.com>
Cc:     Bjorn Andersson <bjorn.andersson@linaro.org>,
        Lokesh Vutla <lokeshvutla@ti.com>,
        Tero Kristo <kristo@kernel.org>,
        linux-remoteproc@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/6] remoteproc: Add support for detach-only during
 shutdown
Message-ID: <20210601171508.GC1759269@xps15>
References: <20210522000309.26134-1-s-anna@ti.com>
 <20210522000309.26134-3-s-anna@ti.com>
 <YLBtbHevzyxT4RTK@builder.lan>
 <da468002-580c-de1a-dcf4-275d57bb7ac7@ti.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <da468002-580c-de1a-dcf4-275d57bb7ac7@ti.com>
Precedence: bulk

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@ti.com>
> >> ---
> >>  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
> >>
>