| Message ID | 20210323223839.17464-4-s-anna@ti.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PRU firmware event/interrupt mapping fixes | expand |
On Tue, Mar 23, 2021 at 05:38:39PM -0500, Suman Anna wrote: > The PRU firmware interrupt mappings are configured and unconfigured in > .start() and .stop() callbacks respectively using the variables 'evt_count' > and a 'mapped_irq' pointer. These variables are modified only during these > callbacks but are not re-initialized/reset properly during unwind or > failure paths. These stale values caused a kernel crash while stopping a > PRU remoteproc running a different firmware with no events on a subsequent > run after a previous run that was running a firmware with events. > > Fix this crash by ensuring that the evt_count is 0 and the mapped_irq > pointer is set to NULL in pru_dispose_irq_mapping(). Also, reset these > variables properly during any failures in the .start() callback. While > at this, the pru_dispose_irq_mapping() callsites are all made to look > the same, moving any conditional logic to inside the function. > > Fixes: c75c9fdac66e ("remoteproc: pru: Add support for PRU specific interrupt configuration") > Reported-by: Vignesh Raghavendra <vigneshr@ti.com> > Signed-off-by: Suman Anna <s-anna@ti.com> > --- > drivers/remoteproc/pru_rproc.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c > index 87b43976c51b..5df19acb90ed 100644 > --- a/drivers/remoteproc/pru_rproc.c > +++ b/drivers/remoteproc/pru_rproc.c > @@ -266,12 +266,17 @@ static void pru_rproc_create_debug_entries(struct rproc *rproc) > > static void pru_dispose_irq_mapping(struct pru_rproc *pru) > { > - while (pru->evt_count--) { > + if (!pru->mapped_irq) > + return; > + > + while (pru->evt_count) { > + pru->evt_count--; > if (pru->mapped_irq[pru->evt_count] > 0) > irq_dispose_mapping(pru->mapped_irq[pru->evt_count]); > } > > kfree(pru->mapped_irq); > + pru->mapped_irq = NULL; > } > > /* > @@ -324,6 +329,8 @@ static int pru_handle_intrmap(struct rproc *rproc) > of_node_put(parent); > if (!irq_parent) { > kfree(pru->mapped_irq); > + pru->mapped_irq = NULL; > + pru->evt_count = 0; Patch 1/3 introduced a check on @parent that doesn't free pru->mapped_irq. I would also expect that error path to do the same has what is done here. And looking further up I see the error path for !pru->mapped_irq doesn't set pru->evt_count to zero. Thanks, Mathieu > return -ENODEV; > } > > @@ -398,8 +405,7 @@ static int pru_rproc_stop(struct rproc *rproc) > pru_control_write_reg(pru, PRU_CTRL_CTRL, val); > > /* dispose irq mapping - new firmware can provide new mapping */ > - if (pru->mapped_irq) > - pru_dispose_irq_mapping(pru); > + pru_dispose_irq_mapping(pru); > > return 0; > } > -- > 2.30.1 >
Hi Mathieu, On 4/6/21 6:47 PM, Mathieu Poirier wrote: > On Tue, Mar 23, 2021 at 05:38:39PM -0500, Suman Anna wrote: >> The PRU firmware interrupt mappings are configured and unconfigured in >> .start() and .stop() callbacks respectively using the variables 'evt_count' >> and a 'mapped_irq' pointer. These variables are modified only during these >> callbacks but are not re-initialized/reset properly during unwind or >> failure paths. These stale values caused a kernel crash while stopping a >> PRU remoteproc running a different firmware with no events on a subsequent >> run after a previous run that was running a firmware with events. >> >> Fix this crash by ensuring that the evt_count is 0 and the mapped_irq >> pointer is set to NULL in pru_dispose_irq_mapping(). Also, reset these >> variables properly during any failures in the .start() callback. While >> at this, the pru_dispose_irq_mapping() callsites are all made to look >> the same, moving any conditional logic to inside the function. >> >> Fixes: c75c9fdac66e ("remoteproc: pru: Add support for PRU specific interrupt configuration") >> Reported-by: Vignesh Raghavendra <vigneshr@ti.com> >> Signed-off-by: Suman Anna <s-anna@ti.com> >> --- >> drivers/remoteproc/pru_rproc.c | 12 +++++++++--- >> 1 file changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c >> index 87b43976c51b..5df19acb90ed 100644 >> --- a/drivers/remoteproc/pru_rproc.c >> +++ b/drivers/remoteproc/pru_rproc.c >> @@ -266,12 +266,17 @@ static void pru_rproc_create_debug_entries(struct rproc *rproc) >> >> static void pru_dispose_irq_mapping(struct pru_rproc *pru) >> { >> - while (pru->evt_count--) { >> + if (!pru->mapped_irq) >> + return; >> + >> + while (pru->evt_count) { >> + pru->evt_count--; >> if (pru->mapped_irq[pru->evt_count] > 0) >> irq_dispose_mapping(pru->mapped_irq[pru->evt_count]); >> } >> >> kfree(pru->mapped_irq); >> + pru->mapped_irq = NULL; >> } >> >> /* >> @@ -324,6 +329,8 @@ static int pru_handle_intrmap(struct rproc *rproc) >> of_node_put(parent); >> if (!irq_parent) { >> kfree(pru->mapped_irq); >> + pru->mapped_irq = NULL; >> + pru->evt_count = 0; > > Patch 1/3 introduced a check on @parent that doesn't free pru->mapped_irq. I > would also expect that error path to do the same has what is done here. And > looking further up I see the error path for !pru->mapped_irq doesn't set > pru->evt_count to zero. Good catch, thank you. I will fix these up in v2. regards Suman > > Thanks, > Mathieu > >> return -ENODEV; >> } >> >> @@ -398,8 +405,7 @@ static int pru_rproc_stop(struct rproc *rproc) >> pru_control_write_reg(pru, PRU_CTRL_CTRL, val); >> >> /* dispose irq mapping - new firmware can provide new mapping */ >> - if (pru->mapped_irq) >> - pru_dispose_irq_mapping(pru); >> + pru_dispose_irq_mapping(pru); >> >> return 0; >> } >> -- >> 2.30.1 >>
diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c index 87b43976c51b..5df19acb90ed 100644 --- a/drivers/remoteproc/pru_rproc.c +++ b/drivers/remoteproc/pru_rproc.c @@ -266,12 +266,17 @@ static void pru_rproc_create_debug_entries(struct rproc *rproc) static void pru_dispose_irq_mapping(struct pru_rproc *pru) { - while (pru->evt_count--) { + if (!pru->mapped_irq) + return; + + while (pru->evt_count) { + pru->evt_count--; if (pru->mapped_irq[pru->evt_count] > 0) irq_dispose_mapping(pru->mapped_irq[pru->evt_count]); } kfree(pru->mapped_irq); + pru->mapped_irq = NULL; } /* @@ -324,6 +329,8 @@ static int pru_handle_intrmap(struct rproc *rproc) of_node_put(parent); if (!irq_parent) { kfree(pru->mapped_irq); + pru->mapped_irq = NULL; + pru->evt_count = 0; return -ENODEV; } @@ -398,8 +405,7 @@ static int pru_rproc_stop(struct rproc *rproc) pru_control_write_reg(pru, PRU_CTRL_CTRL, val); /* dispose irq mapping - new firmware can provide new mapping */ - if (pru->mapped_irq) - pru_dispose_irq_mapping(pru); + pru_dispose_irq_mapping(pru); return 0; }
The PRU firmware interrupt mappings are configured and unconfigured in .start() and .stop() callbacks respectively using the variables 'evt_count' and a 'mapped_irq' pointer. These variables are modified only during these callbacks but are not re-initialized/reset properly during unwind or failure paths. These stale values caused a kernel crash while stopping a PRU remoteproc running a different firmware with no events on a subsequent run after a previous run that was running a firmware with events. Fix this crash by ensuring that the evt_count is 0 and the mapped_irq pointer is set to NULL in pru_dispose_irq_mapping(). Also, reset these variables properly during any failures in the .start() callback. While at this, the pru_dispose_irq_mapping() callsites are all made to look the same, moving any conditional logic to inside the function. Fixes: c75c9fdac66e ("remoteproc: pru: Add support for PRU specific interrupt configuration") Reported-by: Vignesh Raghavendra <vigneshr@ti.com> Signed-off-by: Suman Anna <s-anna@ti.com> --- drivers/remoteproc/pru_rproc.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)