| Message ID | 20240621150058.319524-4-richard.genoud@bootlin.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | remoteproc: k3-r5: Introduce suspend to ram support | expand |
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.
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. 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)
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) > >
Le 01/07/2024 à 18:35, Mathieu Poirier a écrit : > 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. Ok, I see your point now. Thanks ! > >> 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) >> >>
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)
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(-)