linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
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
Date: Tue, 1 Jun 2021 11:15:08 -0600	[thread overview]
Message-ID: <20210601171508.GC1759269@xps15> (raw)
In-Reply-To: <da468002-580c-de1a-dcf4-275d57bb7ac7@ti.com>

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
> >>
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2021-06-01 17:16 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-22  0:03 [PATCH 0/6] K3 R5F & DSP IPC-only mode support Suman Anna
2021-05-22  0:03 ` [PATCH 1/6] remoteproc: Introduce rproc_detach_device() wrapper Suman Anna
2021-05-28  4:17   ` Bjorn Andersson
2021-05-28 16:17     ` Suman Anna
2021-05-22  0:03 ` [PATCH 2/6] remoteproc: Add support for detach-only during shutdown Suman Anna
2021-05-28  4:11   ` Bjorn Andersson
2021-05-28 16:40     ` Suman Anna
2021-06-01 17:15       ` Mathieu Poirier [this message]
2021-05-22  0:03 ` [PATCH 3/6] remoteproc: k3-r5: Refactor mbox request code in start Suman Anna
2021-06-01 17:22   ` Mathieu Poirier
2021-05-22  0:03 ` [PATCH 4/6] remoteproc: k3-r5: Add support for IPC-only mode for all R5Fs Suman Anna
2021-06-01 17:51   ` Mathieu Poirier
2021-06-02 15:53   ` Mathieu Poirier
2021-05-22  0:03 ` [PATCH 5/6] remoteproc: k3-dsp: Refactor mbox request code in start Suman Anna
2021-06-02 16:04   ` Mathieu Poirier
2021-05-22  0:03 ` [PATCH 6/6] remoteproc: k3-dsp: Add support for IPC-only mode for all K3 DSPs Suman Anna
2021-05-28  4:36   ` Bjorn Andersson
2021-05-28 17:04     ` Suman Anna
2021-06-02 16:07   ` Mathieu Poirier
2021-06-03 14:57     ` Suman Anna
2021-06-07 16:33       ` Mathieu Poirier
2021-06-16 15:00         ` Suman Anna
2021-06-22 22:51           ` Mathieu Poirier

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210601171508.GC1759269@xps15 \
    --to=mathieu.poirier@linaro.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=kristo@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-remoteproc@vger.kernel.org \
    --cc=lokeshvutla@ti.com \
    --cc=s-anna@ti.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).