Linux-OMAP Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/3] PRU firmware event/interrupt mapping fixes
@ 2021-03-23 22:38 Suman Anna
  2021-03-23 22:38 ` [PATCH 1/3] remoteproc: pru: Fixup interrupt-parent logic for fw events Suman Anna
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Suman Anna @ 2021-03-23 22:38 UTC (permalink / raw)
  To: Bjorn Andersson, Mathieu Poirier
  Cc: Grzegorz Jaszczyk, Jan Kiszka, Vignesh Raghavendra, Lokesh Vutla,
	linux-remoteproc, linux-omap, linux-arm-kernel, linux-kernel

Hi,

The following series includes fixes for various different issues
associated with the PRU firmware event/interrupt mapping configuration
logic added in the same commit c75c9fdac66e ("remoteproc: pru: Add
support for PRU specific interrupt configuration"). The fixes are
agnostic of SoC family.

Following is the summary of issues and fixes:
 - Patch #1 fixes the interrupt node finding logic to always use the
   inherent sibling relationship between a PRU/RTU/Tx_PRU node and its
   corresponding PRUSS INTC node. This fixes the firmware event mappings
   for cases when the PRU nodes do not have an 'interrupt-parent' property
   (this is the norm, the property is neither required nor added in the DT
   nodes normally).
 - Patch #2 fixes a minor issue with returning a success value to the
   caller on a fw event mapping failure.
 - Patch #3 fixes a kernel crash due to switching of firmwares between
   consecutive runs, the first one with events and the second one without
   events. There are no issues when the same firmwares are run or if they
   are run in reverse order.

Patches should apply cleanly on top of the current rproc-fixes branch
commit 9afeefcf06fc ("remoteproc: pru: Fix firmware loading crashes on K3 SoCs")

regards
Suman

Suman Anna (3):
  remoteproc: pru: Fixup interrupt-parent logic for fw events
  remoteproc: pru: Fix wrong success return value for fw events
  remoteproc: pru: Fix and cleanup firmware interrupt mapping logic

 drivers/remoteproc/pru_rproc.c | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

-- 
2.30.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/3] remoteproc: pru: Fixup interrupt-parent logic for fw events
  2021-03-23 22:38 [PATCH 0/3] PRU firmware event/interrupt mapping fixes Suman Anna
@ 2021-03-23 22:38 ` Suman Anna
  2021-04-06 23:28   ` Mathieu Poirier
  2021-03-23 22:38 ` [PATCH 2/3] remoteproc: pru: Fix wrong success return value " Suman Anna
  2021-03-23 22:38 ` [PATCH 3/3] remoteproc: pru: Fix and cleanup firmware interrupt mapping logic Suman Anna
  2 siblings, 1 reply; 9+ messages in thread
From: Suman Anna @ 2021-03-23 22:38 UTC (permalink / raw)
  To: Bjorn Andersson, Mathieu Poirier
  Cc: Grzegorz Jaszczyk, Jan Kiszka, Vignesh Raghavendra, Lokesh Vutla,
	linux-remoteproc, linux-omap, linux-arm-kernel, linux-kernel

The PRU firmware interrupt mapping logic in pru_handle_intrmap() uses
of_irq_find_parent() with PRU device node to get a handle to the PRUSS
Interrupt Controller at present. This logic however requires that the
PRU nodes always define a interrupt-parent property. This property is
neither a required/defined property as per the PRU remoteproc binding,
nor is relevant from a DT node point of view without any associated
interrupts. The curret logic finds a wrong interrupt controller and
fails to perform proper mapping without any interrupt-parent property
in the PRU nodes.

Fix this logic to always find and use the sibling interrupt controller.
Also, while at this, fix the acquired interrupt controller device node
reference properly.

Fixes: c75c9fdac66e ("remoteproc: pru: Add support for PRU specific interrupt configuration")
Signed-off-by: Suman Anna <s-anna@ti.com>
---
 drivers/remoteproc/pru_rproc.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
index 16979c1cd2f4..a9d07c0751be 100644
--- a/drivers/remoteproc/pru_rproc.c
+++ b/drivers/remoteproc/pru_rproc.c
@@ -284,7 +284,7 @@ static int pru_handle_intrmap(struct rproc *rproc)
 	struct pru_rproc *pru = rproc->priv;
 	struct pru_irq_rsc *rsc = pru->pru_interrupt_map;
 	struct irq_fwspec fwspec;
-	struct device_node *irq_parent;
+	struct device_node *parent, *irq_parent;
 	int i, ret = 0;
 
 	/* not having pru_interrupt_map is not an error */
@@ -312,9 +312,16 @@ static int pru_handle_intrmap(struct rproc *rproc)
 
 	/*
 	 * parse and fill in system event to interrupt channel and
-	 * channel-to-host mapping
+	 * channel-to-host mapping. The interrupt controller to be used
+	 * for these mappings for a given PRU remoteproc is always its
+	 * corresponding sibling PRUSS INTC node.
 	 */
-	irq_parent = of_irq_find_parent(pru->dev->of_node);
+	parent = of_get_parent(dev_of_node(pru->dev));
+	if (!parent)
+		return -ENODEV;
+
+	irq_parent = of_get_child_by_name(parent, "interrupt-controller");
+	of_node_put(parent);
 	if (!irq_parent) {
 		kfree(pru->mapped_irq);
 		return -ENODEV;
@@ -337,11 +344,13 @@ static int pru_handle_intrmap(struct rproc *rproc)
 			goto map_fail;
 		}
 	}
+	of_node_put(irq_parent);
 
 	return ret;
 
 map_fail:
 	pru_dispose_irq_mapping(pru);
+	of_node_put(irq_parent);
 
 	return ret;
 }
-- 
2.30.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/3] remoteproc: pru: Fix wrong success return value for fw events
  2021-03-23 22:38 [PATCH 0/3] PRU firmware event/interrupt mapping fixes Suman Anna
  2021-03-23 22:38 ` [PATCH 1/3] remoteproc: pru: Fixup interrupt-parent logic for fw events Suman Anna
@ 2021-03-23 22:38 ` Suman Anna
  2021-04-06 23:33   ` Mathieu Poirier
  2021-03-23 22:38 ` [PATCH 3/3] remoteproc: pru: Fix and cleanup firmware interrupt mapping logic Suman Anna
  2 siblings, 1 reply; 9+ messages in thread
From: Suman Anna @ 2021-03-23 22:38 UTC (permalink / raw)
  To: Bjorn Andersson, Mathieu Poirier
  Cc: Grzegorz Jaszczyk, Jan Kiszka, Vignesh Raghavendra, Lokesh Vutla,
	linux-remoteproc, linux-omap, linux-arm-kernel, linux-kernel

The irq_create_fwspec_mapping() returns a proper virq value on success
and 0 upon any failure. The pru_handle_intrmap() treats this as an error
and disposes all firmware event mappings correctly, but is returning
this incorrect value as is, letting the pru_rproc_start() interpret it
as a success and boot the PRU.

Fix this by returning an error value back upon any such failure. While
at this, revise the error trace to print some meaningful info about the
failed event.

Fixes: c75c9fdac66e ("remoteproc: pru: Add support for PRU specific interrupt configuration")
Signed-off-by: Suman Anna <s-anna@ti.com>
---
 drivers/remoteproc/pru_rproc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
index a9d07c0751be..87b43976c51b 100644
--- a/drivers/remoteproc/pru_rproc.c
+++ b/drivers/remoteproc/pru_rproc.c
@@ -339,8 +339,10 @@ static int pru_handle_intrmap(struct rproc *rproc)
 
 		pru->mapped_irq[i] = irq_create_fwspec_mapping(&fwspec);
 		if (!pru->mapped_irq[i]) {
-			dev_err(dev, "failed to get virq\n");
-			ret = pru->mapped_irq[i];
+			dev_err(dev, "failed to get virq for fw mapping %d: event %d chnl %d host %d\n",
+				i, fwspec.param[0], fwspec.param[1],
+				fwspec.param[2]);
+			ret = -EINVAL;
 			goto map_fail;
 		}
 	}
-- 
2.30.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 3/3] remoteproc: pru: Fix and cleanup firmware interrupt mapping logic
  2021-03-23 22:38 [PATCH 0/3] PRU firmware event/interrupt mapping fixes Suman Anna
  2021-03-23 22:38 ` [PATCH 1/3] remoteproc: pru: Fixup interrupt-parent logic for fw events Suman Anna
  2021-03-23 22:38 ` [PATCH 2/3] remoteproc: pru: Fix wrong success return value " Suman Anna
@ 2021-03-23 22:38 ` Suman Anna
  2021-04-06 23:47   ` Mathieu Poirier
  2 siblings, 1 reply; 9+ messages in thread
From: Suman Anna @ 2021-03-23 22:38 UTC (permalink / raw)
  To: Bjorn Andersson, Mathieu Poirier
  Cc: Grzegorz Jaszczyk, Jan Kiszka, Vignesh Raghavendra, Lokesh Vutla,
	linux-remoteproc, linux-omap, linux-arm-kernel, linux-kernel

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;
 		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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/3] remoteproc: pru: Fixup interrupt-parent logic for fw events
  2021-03-23 22:38 ` [PATCH 1/3] remoteproc: pru: Fixup interrupt-parent logic for fw events Suman Anna
@ 2021-04-06 23:28   ` Mathieu Poirier
  2021-04-07 14:32     ` Suman Anna
  0 siblings, 1 reply; 9+ messages in thread
From: Mathieu Poirier @ 2021-04-06 23:28 UTC (permalink / raw)
  To: Suman Anna
  Cc: Bjorn Andersson, Grzegorz Jaszczyk, Jan Kiszka,
	Vignesh Raghavendra, Lokesh Vutla, linux-remoteproc, linux-omap,
	linux-arm-kernel, linux-kernel

On Tue, Mar 23, 2021 at 05:38:37PM -0500, Suman Anna wrote:
> The PRU firmware interrupt mapping logic in pru_handle_intrmap() uses
> of_irq_find_parent() with PRU device node to get a handle to the PRUSS
> Interrupt Controller at present. This logic however requires that the
> PRU nodes always define a interrupt-parent property. This property is
> neither a required/defined property as per the PRU remoteproc binding,
> nor is relevant from a DT node point of view without any associated
> interrupts. The curret logic finds a wrong interrupt controller and
> fails to perform proper mapping without any interrupt-parent property
> in the PRU nodes.
> 
> Fix this logic to always find and use the sibling interrupt controller.
> Also, while at this, fix the acquired interrupt controller device node
> reference properly.
> 
> Fixes: c75c9fdac66e ("remoteproc: pru: Add support for PRU specific interrupt configuration")
> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
>  drivers/remoteproc/pru_rproc.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
> index 16979c1cd2f4..a9d07c0751be 100644
> --- a/drivers/remoteproc/pru_rproc.c
> +++ b/drivers/remoteproc/pru_rproc.c
> @@ -284,7 +284,7 @@ static int pru_handle_intrmap(struct rproc *rproc)
>  	struct pru_rproc *pru = rproc->priv;
>  	struct pru_irq_rsc *rsc = pru->pru_interrupt_map;
>  	struct irq_fwspec fwspec;
> -	struct device_node *irq_parent;
> +	struct device_node *parent, *irq_parent;
>  	int i, ret = 0;
>  
>  	/* not having pru_interrupt_map is not an error */
> @@ -312,9 +312,16 @@ static int pru_handle_intrmap(struct rproc *rproc)
>  
>  	/*
>  	 * parse and fill in system event to interrupt channel and
> -	 * channel-to-host mapping
> +	 * channel-to-host mapping. The interrupt controller to be used
> +	 * for these mappings for a given PRU remoteproc is always its
> +	 * corresponding sibling PRUSS INTC node.
>  	 */
> -	irq_parent = of_irq_find_parent(pru->dev->of_node);

If I understand correctly when an interrupt controller node wasn't speficied in
the parent this was unwinding until it found one...

> +	parent = of_get_parent(dev_of_node(pru->dev));
> +	if (!parent)
> +		return -ENODEV;
> +
> +	irq_parent = of_get_child_by_name(parent, "interrupt-controller");
> +	of_node_put(parent);
>  	if (!irq_parent) {
>  		kfree(pru->mapped_irq);
>  		return -ENODEV;
> @@ -337,11 +344,13 @@ static int pru_handle_intrmap(struct rproc *rproc)
>  			goto map_fail;
>  		}
>  	}
> +	of_node_put(irq_parent);
>  
>  	return ret;
>  
>  map_fail:
>  	pru_dispose_irq_mapping(pru);
> +	of_node_put(irq_parent);

Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

>  
>  	return ret;
>  }
> -- 
> 2.30.1
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] remoteproc: pru: Fix wrong success return value for fw events
  2021-03-23 22:38 ` [PATCH 2/3] remoteproc: pru: Fix wrong success return value " Suman Anna
@ 2021-04-06 23:33   ` Mathieu Poirier
  0 siblings, 0 replies; 9+ messages in thread
From: Mathieu Poirier @ 2021-04-06 23:33 UTC (permalink / raw)
  To: Suman Anna
  Cc: Bjorn Andersson, Grzegorz Jaszczyk, Jan Kiszka,
	Vignesh Raghavendra, Lokesh Vutla, linux-remoteproc, linux-omap,
	linux-arm-kernel, linux-kernel

On Tue, Mar 23, 2021 at 05:38:38PM -0500, Suman Anna wrote:
> The irq_create_fwspec_mapping() returns a proper virq value on success
> and 0 upon any failure. The pru_handle_intrmap() treats this as an error
> and disposes all firmware event mappings correctly, but is returning
> this incorrect value as is, letting the pru_rproc_start() interpret it
> as a success and boot the PRU.
> 

Very subtle...  I had to look twice to make sure.

Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

> Fix this by returning an error value back upon any such failure. While
> at this, revise the error trace to print some meaningful info about the
> failed event.
> 
> Fixes: c75c9fdac66e ("remoteproc: pru: Add support for PRU specific interrupt configuration")
> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
>  drivers/remoteproc/pru_rproc.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
> index a9d07c0751be..87b43976c51b 100644
> --- a/drivers/remoteproc/pru_rproc.c
> +++ b/drivers/remoteproc/pru_rproc.c
> @@ -339,8 +339,10 @@ static int pru_handle_intrmap(struct rproc *rproc)
>  
>  		pru->mapped_irq[i] = irq_create_fwspec_mapping(&fwspec);
>  		if (!pru->mapped_irq[i]) {
> -			dev_err(dev, "failed to get virq\n");
> -			ret = pru->mapped_irq[i];
> +			dev_err(dev, "failed to get virq for fw mapping %d: event %d chnl %d host %d\n",
> +				i, fwspec.param[0], fwspec.param[1],
> +				fwspec.param[2]);
> +			ret = -EINVAL;
>  			goto map_fail;
>  		}
>  	}
> -- 
> 2.30.1
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] remoteproc: pru: Fix and cleanup firmware interrupt mapping logic
  2021-03-23 22:38 ` [PATCH 3/3] remoteproc: pru: Fix and cleanup firmware interrupt mapping logic Suman Anna
@ 2021-04-06 23:47   ` Mathieu Poirier
  2021-04-07 14:34     ` Suman Anna
  0 siblings, 1 reply; 9+ messages in thread
From: Mathieu Poirier @ 2021-04-06 23:47 UTC (permalink / raw)
  To: Suman Anna
  Cc: Bjorn Andersson, Grzegorz Jaszczyk, Jan Kiszka,
	Vignesh Raghavendra, Lokesh Vutla, linux-remoteproc, linux-omap,
	linux-arm-kernel, linux-kernel

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
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/3] remoteproc: pru: Fixup interrupt-parent logic for fw events
  2021-04-06 23:28   ` Mathieu Poirier
@ 2021-04-07 14:32     ` Suman Anna
  0 siblings, 0 replies; 9+ messages in thread
From: Suman Anna @ 2021-04-07 14:32 UTC (permalink / raw)
  To: Mathieu Poirier
  Cc: Bjorn Andersson, Grzegorz Jaszczyk, Jan Kiszka,
	Vignesh Raghavendra, Lokesh Vutla, linux-remoteproc, linux-omap,
	linux-arm-kernel, linux-kernel

On 4/6/21 6:28 PM, Mathieu Poirier wrote:
> On Tue, Mar 23, 2021 at 05:38:37PM -0500, Suman Anna wrote:
>> The PRU firmware interrupt mapping logic in pru_handle_intrmap() uses
>> of_irq_find_parent() with PRU device node to get a handle to the PRUSS
>> Interrupt Controller at present. This logic however requires that the
>> PRU nodes always define a interrupt-parent property. This property is
>> neither a required/defined property as per the PRU remoteproc binding,
>> nor is relevant from a DT node point of view without any associated
>> interrupts. The curret logic finds a wrong interrupt controller and
>> fails to perform proper mapping without any interrupt-parent property
>> in the PRU nodes.
>>
>> Fix this logic to always find and use the sibling interrupt controller.
>> Also, while at this, fix the acquired interrupt controller device node
>> reference properly.
>>
>> Fixes: c75c9fdac66e ("remoteproc: pru: Add support for PRU specific interrupt configuration")
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> ---
>>  drivers/remoteproc/pru_rproc.c | 15 ++++++++++++---
>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
>> index 16979c1cd2f4..a9d07c0751be 100644
>> --- a/drivers/remoteproc/pru_rproc.c
>> +++ b/drivers/remoteproc/pru_rproc.c
>> @@ -284,7 +284,7 @@ static int pru_handle_intrmap(struct rproc *rproc)
>>  	struct pru_rproc *pru = rproc->priv;
>>  	struct pru_irq_rsc *rsc = pru->pru_interrupt_map;
>>  	struct irq_fwspec fwspec;
>> -	struct device_node *irq_parent;
>> +	struct device_node *parent, *irq_parent;
>>  	int i, ret = 0;
>>  
>>  	/* not having pru_interrupt_map is not an error */
>> @@ -312,9 +312,16 @@ static int pru_handle_intrmap(struct rproc *rproc)
>>  
>>  	/*
>>  	 * parse and fill in system event to interrupt channel and
>> -	 * channel-to-host mapping
>> +	 * channel-to-host mapping. The interrupt controller to be used
>> +	 * for these mappings for a given PRU remoteproc is always its
>> +	 * corresponding sibling PRUSS INTC node.
>>  	 */
>> -	irq_parent = of_irq_find_parent(pru->dev->of_node);
> 
> If I understand correctly when an interrupt controller node wasn't speficied in
> the parent this was unwinding until it found one...

Correct if not specified in each PRU node, and ends up finding the complete
wrong interrupt controller (GIC) as it walks up the tree.

> 
>> +	parent = of_get_parent(dev_of_node(pru->dev));
>> +	if (!parent)
>> +		return -ENODEV;
>> +
>> +	irq_parent = of_get_child_by_name(parent, "interrupt-controller");
>> +	of_node_put(parent);
>>  	if (!irq_parent) {
>>  		kfree(pru->mapped_irq);
>>  		return -ENODEV;
>> @@ -337,11 +344,13 @@ static int pru_handle_intrmap(struct rproc *rproc)
>>  			goto map_fail;
>>  		}
>>  	}
>> +	of_node_put(irq_parent);
>>  
>>  	return ret;
>>  
>>  map_fail:
>>  	pru_dispose_irq_mapping(pru);
>> +	of_node_put(irq_parent);
> 
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

Thanks,
Suman

> 
>>  
>>  	return ret;
>>  }
>> -- 
>> 2.30.1
>>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] remoteproc: pru: Fix and cleanup firmware interrupt mapping logic
  2021-04-06 23:47   ` Mathieu Poirier
@ 2021-04-07 14:34     ` Suman Anna
  0 siblings, 0 replies; 9+ messages in thread
From: Suman Anna @ 2021-04-07 14:34 UTC (permalink / raw)
  To: Mathieu Poirier
  Cc: Bjorn Andersson, Grzegorz Jaszczyk, Jan Kiszka,
	Vignesh Raghavendra, Lokesh Vutla, linux-remoteproc, linux-omap,
	linux-arm-kernel, linux-kernel

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
>>


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-23 22:38 [PATCH 0/3] PRU firmware event/interrupt mapping fixes Suman Anna
2021-03-23 22:38 ` [PATCH 1/3] remoteproc: pru: Fixup interrupt-parent logic for fw events Suman Anna
2021-04-06 23:28   ` Mathieu Poirier
2021-04-07 14:32     ` Suman Anna
2021-03-23 22:38 ` [PATCH 2/3] remoteproc: pru: Fix wrong success return value " Suman Anna
2021-04-06 23:33   ` Mathieu Poirier
2021-03-23 22:38 ` [PATCH 3/3] remoteproc: pru: Fix and cleanup firmware interrupt mapping logic Suman Anna
2021-04-06 23:47   ` Mathieu Poirier
2021-04-07 14:34     ` Suman Anna

Linux-OMAP Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-omap/0 linux-omap/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-omap linux-omap/ https://lore.kernel.org/linux-omap \
		linux-omap@vger.kernel.org
	public-inbox-index linux-omap

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-omap


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git