linux-kernel.vger.kernel.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>,
	Praneeth Bajjuri <praneeth@ti.com>,
	Hari Nagalla <hnagalla@ti.com>,
	linux-remoteproc@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/5] remoteproc: Add support for detach-only during shutdown
Date: Mon, 2 Aug 2021 12:44:31 -0600	[thread overview]
Message-ID: <20210802184431.GC3051951@p14s> (raw)
In-Reply-To: <20210723220248.6554-2-s-anna@ti.com>

On Fri, Jul 23, 2021 at 05:02:44PM -0500, 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.
> 
> Enhance the remoteproc core logic to key off the presence of the
> .stop() ops and allow the individual remoteproc drivers to continue
> to use the standard rproc_add() and rproc_del() API. This allows
> the remoteproc drivers to only do detach if supported when the driver
> is uninstalled, and the remote processor continues to run undisturbed
> even after the driver removal.
> 
> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
> v2: Addressed various review comments from v1
>  - Reworked the logic to not use remoteproc detach_on_shutdown and
>    rely only on rproc callback ops
>  - Updated the last para of the patch description
> v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20210522000309.26134-3-s-anna@ti.com/
> 
>  drivers/remoteproc/remoteproc_cdev.c  | 7 +++++++
>  drivers/remoteproc/remoteproc_core.c  | 5 ++++-
>  drivers/remoteproc/remoteproc_sysfs.c | 6 ++++++
>  3 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> index 4ad98b0b8caa..16c932beed88 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 &&

This is already checked just above.

> +		    !rproc->ops->stop) {

This is checked in rproc_stop() where -EINVAL is returned if ops::stop has not
been provided. 

> +			dev_err(&rproc->dev,
> +				"stop not supported for this rproc, use detach\n");

The standard error message from the shell should be enough here, the same way it
is enough when the "start" and "stop" scenarios fail.

> +			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 7de5905d276a..ab9e52180b04 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2075,7 +2075,10 @@ void rproc_shutdown(struct rproc *rproc)
>  	if (!atomic_dec_and_test(&rproc->power))
>  		goto out;
>  
> -	ret = rproc_stop(rproc, false);
> +	if (rproc->state == RPROC_ATTACHED && !rproc->ops->stop)
> +		ret = __rproc_detach(rproc);
> +	else
> +		ret = rproc_stop(rproc, false);

As I indicated in my last review I think rproc_shutdown() and rproc_del() should
be decoupled and the right call made in the platform drivers based on the state
of the remote processor.  Conditions such as the above make the core code
brittle, difficult to understand and tedious to maintain.

Thanks,
Mathieu

>  	if (ret) {
>  		atomic_inc(&rproc->power);
>  		goto out;
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> index ea8b89f97d7b..133e766f38d4 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->ops->stop) {
> +			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)
> -- 
> 2.32.0
> 

  reply	other threads:[~2021-08-02 18:44 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-23 22:02 [PATCH v2 0/5] K3 R5F & DSP IPC-only mode support Suman Anna
2021-07-23 22:02 ` [PATCH v2 1/5] remoteproc: Add support for detach-only during shutdown Suman Anna
2021-08-02 18:44   ` Mathieu Poirier [this message]
2021-08-02 23:21     ` Suman Anna
2021-08-03 16:23       ` Mathieu Poirier
2021-08-04 19:17         ` Suman Anna
2021-08-05 17:35           ` Mathieu Poirier
2021-08-05 23:27             ` Suman Anna
2021-08-09 17:00           ` Arnaud POULIQUEN
2021-07-23 22:02 ` [PATCH v2 2/5] remoteproc: k3-r5: Refactor mbox request code in start Suman Anna
2021-07-23 22:02 ` [PATCH v2 3/5] remoteproc: k3-r5: Add support for IPC-only mode for all R5Fs Suman Anna
2021-08-02 18:25   ` Mathieu Poirier
2021-07-23 22:02 ` [PATCH v2 4/5] remoteproc: k3-dsp: Refactor mbox request code in start Suman Anna
2021-07-23 22:02 ` [PATCH v2 5/5] remoteproc: k3-dsp: Add support for IPC-only mode for all K3 DSPs Suman Anna
2021-08-02 18:26   ` Mathieu Poirier
2021-07-26 16:28 ` [PATCH v2 0/5] K3 R5F & DSP IPC-only mode support 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=20210802184431.GC3051951@p14s \
    --to=mathieu.poirier@linaro.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=hnagalla@ti.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-remoteproc@vger.kernel.org \
    --cc=lokeshvutla@ti.com \
    --cc=praneeth@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).