[PATCH v2 0/3] PRU firmware event/interrupt mapping fixes

[PATCH v2 0/3] PRU firmware event/interrupt mapping fixes

[Suman Anna]

Hi All,

The following is a minor revised version of the series [1] that includes fixes
for various different issues associated with the PRU firmware event/interrupt
mapping configuration logic. Please see the v1 cover-letter [1] for additional
details. 

There are currently no in-kernel dts nodes yet in mainline kernel (first
nodes will appear in v5.13-rc1) so these can be picked up for either v5.13
merge window or the current -rc cycle.

Changes in v2:
 - Picked up Reviewed-by tags on Patches 1 and 2
 - Revised Patch 3 to address additional error cleanup paths as
   pointed out by Mathieu.

regards
Suman

[1] https://patchwork.kernel.org/project/linux-remoteproc/cover/20210323223839.17464-1-s-anna@ti.com/

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 | 41 ++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 9 deletions(-)

-- 
2.30.1

[PATCH v2 1/3] remoteproc: pru: Fixup interrupt-parent logic for fw events

[Suman Anna]

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 current 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>
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
v2:
 - Fixed one minor typo ((%s/curret/current) in patch description 
 - Picked up Reviewed-by tag
v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20210323223839.17464-2-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

[PATCH v2 2/3] remoteproc: pru: Fix wrong success return value for fw events

[Suman Anna]

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>
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
v2:
 - No changes, picked up Reviewed-by tag
v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20210323223839.17464-3-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

[PATCH v2 3/3] remoteproc: pru: Fix and cleanup firmware interrupt mapping logic

[Suman Anna]

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>
---
v2:
 - Fixed two additional cleanup paths in pru_handle_intrmap()
   addressing Mathieu's review comment
v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20210323223839.17464-4-s-anna@ti.com/

 drivers/remoteproc/pru_rproc.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
index 87b43976c51b..04863bf23db8 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;
 }
 
 /*
@@ -307,8 +312,10 @@ static int pru_handle_intrmap(struct rproc *rproc)
 	pru->evt_count = rsc->num_evts;
 	pru->mapped_irq = kcalloc(pru->evt_count, sizeof(unsigned int),
 				  GFP_KERNEL);
-	if (!pru->mapped_irq)
+	if (!pru->mapped_irq) {
+		pru->evt_count = 0;
 		return -ENOMEM;
+	}
 
 	/*
 	 * parse and fill in system event to interrupt channel and
@@ -317,13 +324,19 @@ static int pru_handle_intrmap(struct rproc *rproc)
 	 * corresponding sibling PRUSS INTC node.
 	 */
 	parent = of_get_parent(dev_of_node(pru->dev));
-	if (!parent)
+	if (!parent) {
+		kfree(pru->mapped_irq);
+		pru->mapped_irq = NULL;
+		pru->evt_count = 0;
 		return -ENODEV;
+	}
 
 	irq_parent = of_get_child_by_name(parent, "interrupt-controller");
 	of_node_put(parent);
 	if (!irq_parent) {
 		kfree(pru->mapped_irq);
+		pru->mapped_irq = NULL;
+		pru->evt_count = 0;
 		return -ENODEV;
 	}
 
@@ -398,8 +411,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

Re: [PATCH v2 3/3] remoteproc: pru: Fix and cleanup firmware interrupt mapping logic

[Mathieu Poirier]

On Wed, Apr 07, 2021 at 10:56:41AM -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>
> ---
> v2:
>  - Fixed two additional cleanup paths in pru_handle_intrmap()
>    addressing Mathieu's review comment
> v1: https://patchwork.kernel.org/project/linux-remoteproc/patch/20210323223839.17464-4-s-anna@ti.com/
> 
>  drivers/remoteproc/pru_rproc.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
>

Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
 
> diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
> index 87b43976c51b..04863bf23db8 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;
>  }
>  
>  /*
> @@ -307,8 +312,10 @@ static int pru_handle_intrmap(struct rproc *rproc)
>  	pru->evt_count = rsc->num_evts;
>  	pru->mapped_irq = kcalloc(pru->evt_count, sizeof(unsigned int),
>  				  GFP_KERNEL);
> -	if (!pru->mapped_irq)
> +	if (!pru->mapped_irq) {
> +		pru->evt_count = 0;
>  		return -ENOMEM;
> +	}
>  
>  	/*
>  	 * parse and fill in system event to interrupt channel and
> @@ -317,13 +324,19 @@ static int pru_handle_intrmap(struct rproc *rproc)
>  	 * corresponding sibling PRUSS INTC node.
>  	 */
>  	parent = of_get_parent(dev_of_node(pru->dev));
> -	if (!parent)
> +	if (!parent) {
> +		kfree(pru->mapped_irq);
> +		pru->mapped_irq = NULL;
> +		pru->evt_count = 0;
>  		return -ENODEV;
> +	}
>  
>  	irq_parent = of_get_child_by_name(parent, "interrupt-controller");
>  	of_node_put(parent);
>  	if (!irq_parent) {
>  		kfree(pru->mapped_irq);
> +		pru->mapped_irq = NULL;
> +		pru->evt_count = 0;
>  		return -ENODEV;
>  	}
>  
> @@ -398,8 +411,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
> 

Re: [PATCH v2 0/3] PRU firmware event/interrupt mapping fixes

[Suman Anna]

Hi Bjorn,

On 4/7/21 10:56 AM, Suman Anna wrote:
> Hi All,
> 
> The following is a minor revised version of the series [1] that includes fixes
> for various different issues associated with the PRU firmware event/interrupt
> mapping configuration logic. Please see the v1 cover-letter [1] for additional
> details. 
> 
> There are currently no in-kernel dts nodes yet in mainline kernel (first
> nodes will appear in v5.13-rc1) so these can be picked up for either v5.13
> merge window or the current -rc cycle.
> 
> Changes in v2:
>  - Picked up Reviewed-by tags on Patches 1 and 2
>  - Revised Patch 3 to address additional error cleanup paths as
>    pointed out by Mathieu.

All patches are reviewed now, so can you please pick these up for the next merge
window if not already in your queue.

regards
Suman

> 
> regards
> Suman
> 
> [1] https://patchwork.kernel.org/project/linux-remoteproc/cover/20210323223839.17464-1-s-anna@ti.com/
> 
> 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 | 41 ++++++++++++++++++++++++++--------
>  1 file changed, 32 insertions(+), 9 deletions(-)
>