| Message ID | 20240621150058.319524-2-richard.genoud@bootlin.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | remoteproc: k3-r5: Introduce suspend to ram support | expand |
Good day, On Fri, Jun 21, 2024 at 05:00:55PM +0200, Richard Genoud wrote: > ret variable was used to test reset status, get from > reset_control_status() call. But this variable was overwritten by > ti_sci_proc_get_status() a few lines bellow. > And as ti_sci_proc_get_status() returns 0 or a negative value (in this > latter case, followed by a return), the expression !ret was always true, > > Clearly, this was not what was intended: > In the comment above it's said that "requires both local and module > resets to be deasserted"; if reset_control_status() returns 0 it means > that the reset line is deasserted. > So, it's pretty clear that the return value of reset_control_status() > was intended to be used instead of ti_sci_proc_get_status() return > value. > > This could lead in an incorrect IPC-only mode detection if reset line is > asserted (so reset_control_status() return > 0) and c_state != 0 and > halted == 0. > In this case, the old code would have detected an IPC-only mode instead > of a mismatched mode. > Your assessment seems to be correct. That said I'd like to have an RB or a TB from someone in the TI delegation - guys please have a look. Thanks, Mathieu > Fixes: 1168af40b1ad ("remoteproc: k3-r5: Add support for IPC-only mode for all R5Fs") > Signed-off-by: Richard Genoud <richard.genoud@bootlin.com> > --- > drivers/remoteproc/ti_k3_r5_remoteproc.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c > index 50e486bcfa10..39a47540c590 100644 > --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c > +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c > @@ -1144,6 +1144,7 @@ static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc) > u32 atcm_enable, btcm_enable, loczrama; > struct k3_r5_core *core0; > enum cluster_mode mode = cluster->mode; > + int reset_ctrl_status; > int ret; > > core0 = list_first_entry(&cluster->cores, struct k3_r5_core, elem); > @@ -1160,11 +1161,11 @@ static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc) > r_state, c_state); > } > > - ret = reset_control_status(core->reset); > - if (ret < 0) { > + reset_ctrl_status = reset_control_status(core->reset); > + if (reset_ctrl_status < 0) { > dev_err(cdev, "failed to get initial local reset status, ret = %d\n", > - ret); > - return ret; > + reset_ctrl_status); > + return reset_ctrl_status; > } > > /* > @@ -1199,7 +1200,7 @@ static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc) > * irrelevant if module reset is asserted (POR value has local reset > * deasserted), and is deemed as remoteproc mode > */ > - if (c_state && !ret && !halted) { > + if (c_state && !reset_ctrl_status && !halted) { > dev_info(cdev, "configured R5F for IPC-only mode\n"); > kproc->rproc->state = RPROC_DETACHED; > ret = 1; > @@ -1217,7 +1218,7 @@ static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc) > ret = 0; > } else { > dev_err(cdev, "mismatched mode: local_reset = %s, module_reset = %s, core_state = %s\n", > - !ret ? "deasserted" : "asserted", > + !reset_ctrl_status ? "deasserted" : "asserted", > c_state ? "deasserted" : "asserted", > halted ? "halted" : "unhalted"); > ret = -EINVAL;
Nishanth, Vignesh, Hari and Andrew - please have a look at this patch. Thanks, Mathieu On Fri, 28 Jun 2024 at 13:53, Mathieu Poirier <mathieu.poirier@linaro.org> wrote: > > Good day, > > On Fri, Jun 21, 2024 at 05:00:55PM +0200, Richard Genoud wrote: > > ret variable was used to test reset status, get from > > reset_control_status() call. But this variable was overwritten by > > ti_sci_proc_get_status() a few lines bellow. > > And as ti_sci_proc_get_status() returns 0 or a negative value (in this > > latter case, followed by a return), the expression !ret was always true, > > > > Clearly, this was not what was intended: > > In the comment above it's said that "requires both local and module > > resets to be deasserted"; if reset_control_status() returns 0 it means > > that the reset line is deasserted. > > So, it's pretty clear that the return value of reset_control_status() > > was intended to be used instead of ti_sci_proc_get_status() return > > value. > > > > This could lead in an incorrect IPC-only mode detection if reset line is > > asserted (so reset_control_status() return > 0) and c_state != 0 and > > halted == 0. > > In this case, the old code would have detected an IPC-only mode instead > > of a mismatched mode. > > > > Your assessment seems to be correct. That said I'd like to have an RB or a TB > from someone in the TI delegation - guys please have a look. > > Thanks, > Mathieu > > > Fixes: 1168af40b1ad ("remoteproc: k3-r5: Add support for IPC-only mode for all R5Fs") > > Signed-off-by: Richard Genoud <richard.genoud@bootlin.com> > > --- > > drivers/remoteproc/ti_k3_r5_remoteproc.c | 13 +++++++------ > > 1 file changed, 7 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c > > index 50e486bcfa10..39a47540c590 100644 > > --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c > > +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c > > @@ -1144,6 +1144,7 @@ static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc) > > u32 atcm_enable, btcm_enable, loczrama; > > struct k3_r5_core *core0; > > enum cluster_mode mode = cluster->mode; > > + int reset_ctrl_status; > > int ret; > > > > core0 = list_first_entry(&cluster->cores, struct k3_r5_core, elem); > > @@ -1160,11 +1161,11 @@ static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc) > > r_state, c_state); > > } > > > > - ret = reset_control_status(core->reset); > > - if (ret < 0) { > > + reset_ctrl_status = reset_control_status(core->reset); > > + if (reset_ctrl_status < 0) { > > dev_err(cdev, "failed to get initial local reset status, ret = %d\n", > > - ret); > > - return ret; > > + reset_ctrl_status); > > + return reset_ctrl_status; > > } > > > > /* > > @@ -1199,7 +1200,7 @@ static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc) > > * irrelevant if module reset is asserted (POR value has local reset > > * deasserted), and is deemed as remoteproc mode > > */ > > - if (c_state && !ret && !halted) { > > + if (c_state && !reset_ctrl_status && !halted) { > > dev_info(cdev, "configured R5F for IPC-only mode\n"); > > kproc->rproc->state = RPROC_DETACHED; > > ret = 1; > > @@ -1217,7 +1218,7 @@ static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc) > > ret = 0; > > } else { > > dev_err(cdev, "mismatched mode: local_reset = %s, module_reset = %s, core_state = %s\n", > > - !ret ? "deasserted" : "asserted", > > + !reset_ctrl_status ? "deasserted" : "asserted", > > c_state ? "deasserted" : "asserted", > > halted ? "halted" : "unhalted"); > > ret = -EINVAL;
On 6/28/24 14:58, Mathieu Poirier wrote: >> This could lead in an incorrect IPC-only mode detection if reset line is >> asserted (so reset_control_status() return > 0) and c_state != 0 and >> halted == 0. >> In this case, the old code would have detected an IPC-only mode instead >> of a mismatched mode. >> > Your assessment seems to be correct. That said I'd like to have an RB or a TB > from someone in the TI delegation - guys please have a look. Agree with Richard's assessment, and the proposed fix looks good. Reviewed-by: Hari Nagalla <hnagalla@ti.com>
On Mon, Jul 01, 2024 at 04:13:00AM -0500, Hari Nagalla wrote: > On 6/28/24 14:58, Mathieu Poirier wrote: > > > This could lead in an incorrect IPC-only mode detection if reset line is > > > asserted (so reset_control_status() return > 0) and c_state != 0 and > > > halted == 0. > > > In this case, the old code would have detected an IPC-only mode instead > > > of a mismatched mode. > > > > > Your assessment seems to be correct. That said I'd like to have an RB or a TB > > from someone in the TI delegation - guys please have a look. > Agree with Richard's assessment, and the proposed fix looks good. > > Reviewed-by: > Hari Nagalla <hnagalla@ti.com> I have applied this patch, no need to send it again. Thanks, Mathieu
diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c index 50e486bcfa10..39a47540c590 100644 --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c @@ -1144,6 +1144,7 @@ static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc) u32 atcm_enable, btcm_enable, loczrama; struct k3_r5_core *core0; enum cluster_mode mode = cluster->mode; + int reset_ctrl_status; int ret; core0 = list_first_entry(&cluster->cores, struct k3_r5_core, elem); @@ -1160,11 +1161,11 @@ static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc) r_state, c_state); } - ret = reset_control_status(core->reset); - if (ret < 0) { + reset_ctrl_status = reset_control_status(core->reset); + if (reset_ctrl_status < 0) { dev_err(cdev, "failed to get initial local reset status, ret = %d\n", - ret); - return ret; + reset_ctrl_status); + return reset_ctrl_status; } /* @@ -1199,7 +1200,7 @@ static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc) * irrelevant if module reset is asserted (POR value has local reset * deasserted), and is deemed as remoteproc mode */ - if (c_state && !ret && !halted) { + if (c_state && !reset_ctrl_status && !halted) { dev_info(cdev, "configured R5F for IPC-only mode\n"); kproc->rproc->state = RPROC_DETACHED; ret = 1; @@ -1217,7 +1218,7 @@ static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc) ret = 0; } else { dev_err(cdev, "mismatched mode: local_reset = %s, module_reset = %s, core_state = %s\n", - !ret ? "deasserted" : "asserted", + !reset_ctrl_status ? "deasserted" : "asserted", c_state ? "deasserted" : "asserted", halted ? "halted" : "unhalted"); ret = -EINVAL;
ret variable was used to test reset status, get from reset_control_status() call. But this variable was overwritten by ti_sci_proc_get_status() a few lines bellow. And as ti_sci_proc_get_status() returns 0 or a negative value (in this latter case, followed by a return), the expression !ret was always true, Clearly, this was not what was intended: In the comment above it's said that "requires both local and module resets to be deasserted"; if reset_control_status() returns 0 it means that the reset line is deasserted. So, it's pretty clear that the return value of reset_control_status() was intended to be used instead of ti_sci_proc_get_status() return value. This could lead in an incorrect IPC-only mode detection if reset line is asserted (so reset_control_status() return > 0) and c_state != 0 and halted == 0. In this case, the old code would have detected an IPC-only mode instead of a mismatched mode. Fixes: 1168af40b1ad ("remoteproc: k3-r5: Add support for IPC-only mode for all R5Fs") Signed-off-by: Richard Genoud <richard.genoud@bootlin.com> --- drivers/remoteproc/ti_k3_r5_remoteproc.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)