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@xxxxxx>
>> Signed-off-by: Suman Anna <s-anna@xxxxxx>
>> ---
>> 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
>>
