From: Mathieu Poirier <mathieu.poirier@linaro.org>
To: Richard GENOUD <richard.genoud@bootlin.com>
Cc: "Bjorn Andersson" <andersson@kernel.org>,
"Philipp Zabel" <p.zabel@pengutronix.de>,
"Suman Anna" <s-anna@ti.com>,
"Thomas Petazzoni" <thomas.petazzoni@bootlin.com>,
"Alexandre Belloni" <alexandre.belloni@bootlin.com>,
"Udit Kumar" <u-kumar1@ti.com>,
"Thomas Richard" <thomas.richard@bootlin.com>,
"Gregory CLEMENT" <gregory.clement@bootlin.com>,
"Hari Nagalla" <hnagalla@ti.com>,
"Théo Lebrun" <theo.lebrun@bootlin.com>,
linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/4] remoteproc: k3-r5: k3_r5_rproc_stop: code reorder
Date: Mon, 1 Jul 2024 10:35:49 -0600 [thread overview]
Message-ID: <ZoLa5WklP5+T/qTO@p14s> (raw)
In-Reply-To: <99496000-a994-4641-861a-10cbb501910e@bootlin.com>
On Mon, Jul 01, 2024 at 10:03:22AM +0200, Richard GENOUD wrote:
> Le 28/06/2024 à 23:18, Mathieu Poirier a écrit :
> > On Fri, Jun 21, 2024 at 05:00:57PM +0200, Richard Genoud wrote:
> > > In the next commit, a RP_MBOX_SHUTDOWN message will be sent in
> > > k3_r5_rproc_stop() to the remote proc (in lockstep on not)
> > > Thus, the sanity check "do not allow core 0 to stop before core 1"
> > > should be moved at the beginning of the function so that the generic case
> > > can be dealt with.
> > >
> > > In order to have an easier patch to review, those actions are broke in
> > > two patches:
> > > - this patch: moving the sanity check at the beginning (No functional
> > > change).
> > > - next patch: doing the real job (sending shutdown messages to remote
> > > procs before halting them).
> > >
> > > Basically, we had:
> > > - cluster_mode actions
> > > - !cluster_mode sanity check
> > > - !cluster_mode actions
> > > And now:
> > > - !cluster_mode sanity check
> > > - cluster_mode actions
> > > - !cluster_mode actions
> > >
> > > Signed-off-by: Richard Genoud <richard.genoud@bootlin.com>
> > > ---
> > > drivers/remoteproc/ti_k3_r5_remoteproc.c | 24 ++++++++++++++----------
> > > 1 file changed, 14 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> > > index 1f18b08618c8..a2ead87952c7 100644
> > > --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
> > > +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> > > @@ -636,16 +636,8 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
> > > struct k3_r5_core *core1, *core = kproc->core;
> > > int ret;
> > > - /* halt all applicable cores */
> > > - if (cluster->mode == CLUSTER_MODE_LOCKSTEP) {
> > > - list_for_each_entry(core, &cluster->cores, elem) {
> > > - ret = k3_r5_core_halt(core);
> > > - if (ret) {
> > > - core = list_prev_entry(core, elem);
> > > - goto unroll_core_halt;
> > > - }
> > > - }
> > > - } else {
> > > +
> > > + if (cluster->mode != CLUSTER_MODE_LOCKSTEP) {
> > > /* do not allow core 0 to stop before core 1 */
> > > core1 = list_last_entry(&cluster->cores, struct k3_r5_core,
> > > elem);
> > > @@ -656,6 +648,18 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
> > > ret = -EPERM;
> > > goto out;
> > > }
> > > + }
> > > +
> > > + /* halt all applicable cores */
> > > + if (cluster->mode == CLUSTER_MODE_LOCKSTEP) {
> > > + list_for_each_entry(core, &cluster->cores, elem) {
> > > + ret = k3_r5_core_halt(core);
> > > + if (ret) {
> > > + core = list_prev_entry(core, elem);
> > > + goto unroll_core_halt;
> > > + }
> > > + }
> > > + } else {
> > > ret = k3_r5_core_halt(core);
> > > if (ret)
> >
> > With this patch, the "else" in this "if" condition is coupled with the "if" from
> > the lockstep mode, making the code extremaly hard to read. The original code
> > has a k3_r5_core_halt() in both "if" conditions, making the condition
> > independent from one another.
> >
> I'm not sure I understand what you mean.
With your patch applied I get the following: https://pastebin.com/yTZ0pKcS
Let's say the R5 is in split mode and k3_r5_rproc_stop() called on core1. The
if() that deal with that condition is on line 10, while the function that halts
the remote processor is online 34, part of the else clause that handles lockstep
mode. The two if() clauses are entangled and nothing good can come out of that.
> Anyway, I'm not happy with this diff, it doesn't reflect what was intended.
> (which is moving the check "core 0 should not be stop before core 1" at the beginning)
>
> Tweaking around with the diff algorithms, I came with something way easier to read I think:
>
> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> index 1f18b08618c8..a2ead87952c7 100644
> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> @@ -636,6 +636,20 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
> struct k3_r5_core *core1, *core = kproc->core;
> int ret;
> +
> + if (cluster->mode != CLUSTER_MODE_LOCKSTEP) {
> + /* do not allow core 0 to stop before core 1 */
> + core1 = list_last_entry(&cluster->cores, struct k3_r5_core,
> + elem);
> + if (core != core1 && core1->rproc->state != RPROC_OFFLINE &&
> + core1->rproc->state != RPROC_SUSPENDED) {
> + dev_err(dev, "%s: can not stop core 0 before core 1\n",
> + __func__);
> + ret = -EPERM;
> + goto out;
> + }
> + }
> +
> /* halt all applicable cores */
> if (cluster->mode == CLUSTER_MODE_LOCKSTEP) {
> list_for_each_entry(core, &cluster->cores, elem) {
> @@ -646,16 +660,6 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
> }
> }
> } else {
> - /* do not allow core 0 to stop before core 1 */
> - core1 = list_last_entry(&cluster->cores, struct k3_r5_core,
> - elem);
> - if (core != core1 && core1->rproc->state != RPROC_OFFLINE &&
> - core1->rproc->state != RPROC_SUSPENDED) {
> - dev_err(dev, "%s: can not stop core 0 before core 1\n",
> - __func__);
> - ret = -EPERM;
> - goto out;
> - }
> ret = k3_r5_core_halt(core);
> if (ret)
>
>
next prev parent reply other threads:[~2024-07-01 16:35 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-06-21 15:00 [PATCH 0/4] remoteproc: k3-r5: Introduce suspend to ram support Richard Genoud
2024-06-21 15:00 ` [PATCH 1/4] remoteproc: k3-r5: Fix IPC-only mode detection Richard Genoud
2024-06-28 19:53 ` Mathieu Poirier
2024-06-28 19:58 ` Mathieu Poirier
2024-07-01 9:13 ` Hari Nagalla
2024-07-01 16:38 ` Mathieu Poirier
2024-06-21 15:00 ` [PATCH 2/4] remoteproc: k3-r5: Introduce PM suspend/resume handlers Richard Genoud
2024-06-28 20:48 ` Mathieu Poirier
2024-07-01 7:30 ` Richard GENOUD
2024-07-01 19:02 ` kernel test robot
2024-06-21 15:00 ` [PATCH 3/4] remoteproc: k3-r5: k3_r5_rproc_stop: code reorder Richard Genoud
2024-06-28 21:18 ` Mathieu Poirier
2024-07-01 8:03 ` Richard GENOUD
2024-07-01 16:35 ` Mathieu Poirier [this message]
2024-07-01 16:49 ` Richard GENOUD
2024-06-21 15:00 ` [PATCH 4/4] remoteproc: k3-r5: support for graceful stop of remote cores Richard Genoud
2024-06-28 21:20 ` Mathieu Poirier
2024-07-01 16:38 ` Richard GENOUD
2024-06-28 22:50 ` Andrew Davis
2024-07-01 16:48 ` Richard GENOUD
2024-07-01 21:55 ` kernel test robot
2024-06-28 21:23 ` [PATCH 0/4] remoteproc: k3-r5: Introduce suspend to ram support Mathieu Poirier
2024-07-01 9:59 ` Hari Nagalla
2024-07-08 7:33 ` Richard GENOUD
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=ZoLa5WklP5+T/qTO@p14s \
--to=mathieu.poirier@linaro.org \
--cc=alexandre.belloni@bootlin.com \
--cc=andersson@kernel.org \
--cc=gregory.clement@bootlin.com \
--cc=hnagalla@ti.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-remoteproc@vger.kernel.org \
--cc=p.zabel@pengutronix.de \
--cc=richard.genoud@bootlin.com \
--cc=s-anna@ti.com \
--cc=theo.lebrun@bootlin.com \
--cc=thomas.petazzoni@bootlin.com \
--cc=thomas.richard@bootlin.com \
--cc=u-kumar1@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 read-only IMAP folder(s) and NNTP newsgroup(s).