[PATCH 1/2] net: ethernet: cpsw: don't claim IRQs with devm_request_irq()

[PATCH 1/2] net: ethernet: cpsw: don't claim IRQs with devm_request_irq()

[Daniel Mack]

Julia Lawall spotted a problem with aa1a15e ("net: ethernet: cpsw:
switch to devres allocations") which introduced a race condition in
cpsw_probe() by removing explicit interrupt disable calls before
calling free_netdev(). Hence, an interrupt can fire after free_netdev()
was called. The same problem exists in cpsw_remove().

Fix this by reverting the IRQ part of the aforementioned patch and
handle those resources explicitly.

Reported-by: Julia Lawall <julia.lawall@lip6.fr>
Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 drivers/net/ethernet/ti/cpsw.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 999fb72..cdbbb58 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2233,10 +2233,10 @@ static int cpsw_probe(struct platform_device *pdev)
 
 	while ((res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k))) {
 		for (i = res->start; i <= res->end; i++) {
-			if (devm_request_irq(&pdev->dev, i, cpsw_interrupt, 0,
-					     dev_name(&pdev->dev), priv)) {
+			if (request_irq(i, cpsw_interrupt, 0,
+					dev_name(&pdev->dev), priv)) {
 				dev_err(priv->dev, "error attaching irq\n");
-				goto clean_ale_ret;
+				goto clean_irq_ret;
 			}
 			priv->irqs_table[k] = i;
 			priv->num_irqs = k + 1;
@@ -2256,7 +2256,7 @@ static int cpsw_probe(struct platform_device *pdev)
 	if (ret) {
 		dev_err(priv->dev, "error registering net device\n");
 		ret = -ENODEV;
-		goto clean_ale_ret;
+		goto clean_irq_ret;
 	}
 
 	cpsw_notice(priv, probe, "initialized device (regs %pa, irq %d)\n",
@@ -2266,12 +2266,15 @@ static int cpsw_probe(struct platform_device *pdev)
 		ret = cpsw_probe_dual_emac(pdev, priv);
 		if (ret) {
 			cpsw_err(priv, probe, "error probe slave 2 emac interface\n");
-			goto clean_ale_ret;
+			goto clean_irq_ret;
 		}
 	}
 
 	return 0;
 
+clean_irq_ret:
+	for (i = 0; i < priv->num_irqs; i++)
+		free_irq(priv->irqs_table[i], priv);
 clean_ale_ret:
 	cpsw_ale_destroy(priv->ale);
 clean_dma_ret:
@@ -2289,11 +2292,15 @@ static int cpsw_remove(struct platform_device *pdev)
 {
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct cpsw_priv *priv = netdev_priv(ndev);
+	int i;
 
 	if (priv->data.dual_emac)
 		unregister_netdev(cpsw_get_slave_ndev(priv, 1));
 	unregister_netdev(ndev);
 
+	for (i = 0; i < priv->num_irqs; i++)
+		free_irq(priv->irqs_table[i], priv);
+
 	cpsw_ale_destroy(priv->ale);
 	cpdma_chan_destroy(priv->txch);
 	cpdma_chan_destroy(priv->rxch);
-- 
2.1.0

[PATCH 2/2] net: ethernet: cpsw: fix interrupt lookup logic in cpsw_probe()

[Daniel Mack]

The code in cpsw_probe() currently iterates over the available
interrupt resources and requests each of them.  While doing so, it
keeps track of their indices through priv->irqs_table.

However, the code currently only remembers the last interrupt in
a resource, and will leak the others if there is more than one.
This can only happen for board-file driven platforms and not via DT,
however.

Also, there is currently no bounds check, while priv->irqs_table is a
fixed-size array. If we are passed more than 4 resources, we're in
trouble.

This patch introduces a bounds check and changes the way interrupt
indices are kept. Tested on a Beagle Bone Black board only.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 drivers/net/ethernet/ti/cpsw.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index cdbbb58..e747e55 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2233,13 +2233,18 @@ static int cpsw_probe(struct platform_device *pdev)
 
 	while ((res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k))) {
 		for (i = res->start; i <= res->end; i++) {
+			if (priv->num_irqs >= ARRAY_SIZE(priv->irqs_table)) {
+				ret = -EINVAL;
+				goto clean_irq_ret;
+			}
+
 			if (request_irq(i, cpsw_interrupt, 0,
 					dev_name(&pdev->dev), priv)) {
 				dev_err(priv->dev, "error attaching irq\n");
 				goto clean_irq_ret;
 			}
-			priv->irqs_table[k] = i;
-			priv->num_irqs = k + 1;
+			priv->irqs_table[priv->num_irqs] = i;
+			priv->num_irqs++;
 		}
 		k++;
 	}
-- 
2.1.0

Re: [PATCH 1/2] net: ethernet: cpsw: don't claim IRQs with devm_request_irq()

[Mugunthan V N]

On Tuesday 02 September 2014 10:14 PM, Daniel Mack wrote:
> Julia Lawall spotted a problem with aa1a15e ("net: ethernet: cpsw:
> switch to devres allocations") which introduced a race condition in
> cpsw_probe() by removing explicit interrupt disable calls before
> calling free_netdev(). Hence, an interrupt can fire after free_netdev()
> was called. The same problem exists in cpsw_remove().
> 
> Fix this by reverting the IRQ part of the aforementioned patch and
> handle those resources explicitly.
> 
> Reported-by: Julia Lawall <julia.lawall@lip6.fr>
> Signed-off-by: Daniel Mack <zonque@gmail.com>

CPSW interrupts cannot be triggered as the interrupts are disabled in
priv->wr_regs->tx_en and priv->wr_regs->rx_en inside CPSW module and
these interrupts are enabled only when the device is opened.

In cpsw_remove, CPDMA controller is stopped and interrupts are disabled
in cpsw_ndo_stop(), so there is no chance that an interrupt can occur
during cpsw_remove().

Regards
Mugunthan V N

Re: [PATCH 2/2] net: ethernet: cpsw: fix interrupt lookup logic in cpsw_probe()

[Mugunthan V N]

On Tuesday 02 September 2014 10:14 PM, Daniel Mack wrote:
> The code in cpsw_probe() currently iterates over the available
> interrupt resources and requests each of them.  While doing so, it
> keeps track of their indices through priv->irqs_table.
> 
> However, the code currently only remembers the last interrupt in
> a resource, and will leak the others if there is more than one.
> This can only happen for board-file driven platforms and not via DT,
> however.
> 
> Also, there is currently no bounds check, while priv->irqs_table is a
> fixed-size array. If we are passed more than 4 resources, we're in
> trouble.
> 
> This patch introduces a bounds check and changes the way interrupt
> indices are kept. Tested on a Beagle Bone Black board only.
> 
> Signed-off-by: Daniel Mack <zonque@gmail.com>

The drivers is not supported for non-DT platforms as all the platforms
which uses CPSW are DT only platforms.

Regards
Mugunthan V N

Re: [PATCH 2/2] net: ethernet: cpsw: fix interrupt lookup logic in cpsw_probe()

[Daniel Mack]

On 09/03/2014 09:28 AM, Mugunthan V N wrote:
> On Tuesday 02 September 2014 10:14 PM, Daniel Mack wrote:
>> The code in cpsw_probe() currently iterates over the available
>> interrupt resources and requests each of them.  While doing so, it
>> keeps track of their indices through priv->irqs_table.
>>
>> However, the code currently only remembers the last interrupt in
>> a resource, and will leak the others if there is more than one.
>> This can only happen for board-file driven platforms and not via DT,
>> however.
>>
>> Also, there is currently no bounds check, while priv->irqs_table is a
>> fixed-size array. If we are passed more than 4 resources, we're in
>> trouble.
>>
>> This patch introduces a bounds check and changes the way interrupt
>> indices are kept. Tested on a Beagle Bone Black board only.
>>
>> Signed-off-by: Daniel Mack <zonque@gmail.com>
> 
> The drivers is not supported for non-DT platforms as all the platforms
> which uses CPSW are DT only platforms.

Ok, thanks for explaining.

But then we can remove the iteration then and simplify the code, right?
The bounds check should also be done.


Thanks,
Daniel

Re: [PATCH 2/2] net: ethernet: cpsw: fix interrupt lookup logic in cpsw_probe()

[Mugunthan V N]

On Wednesday 03 September 2014 01:00 PM, Daniel Mack wrote:
> On 09/03/2014 09:28 AM, Mugunthan V N wrote:
>> On Tuesday 02 September 2014 10:14 PM, Daniel Mack wrote:
>>> The code in cpsw_probe() currently iterates over the available
>>> interrupt resources and requests each of them.  While doing so, it
>>> keeps track of their indices through priv->irqs_table.
>>>
>>> However, the code currently only remembers the last interrupt in
>>> a resource, and will leak the others if there is more than one.
>>> This can only happen for board-file driven platforms and not via DT,
>>> however.
>>>
>>> Also, there is currently no bounds check, while priv->irqs_table is a
>>> fixed-size array. If we are passed more than 4 resources, we're in
>>> trouble.
>>>
>>> This patch introduces a bounds check and changes the way interrupt
>>> indices are kept. Tested on a Beagle Bone Black board only.
>>>
>>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>>
>> The drivers is not supported for non-DT platforms as all the platforms
>> which uses CPSW are DT only platforms.
> 
> Ok, thanks for explaining.
> 
> But then we can remove the iteration then and simplify the code, right?
> The bounds check should also be done.
> 

Right, we can simplify the code.

Regards
Mugunthan V N

Re: [PATCH 2/2] net: ethernet: cpsw: fix interrupt lookup logic in cpsw_probe()

[Daniel Mack]

On 09/03/2014 10:22 AM, Mugunthan V N wrote:
> On Wednesday 03 September 2014 01:00 PM, Daniel Mack wrote:
>> On 09/03/2014 09:28 AM, Mugunthan V N wrote:
>>> On Tuesday 02 September 2014 10:14 PM, Daniel Mack wrote:
>>>> The code in cpsw_probe() currently iterates over the available
>>>> interrupt resources and requests each of them.  While doing so, it
>>>> keeps track of their indices through priv->irqs_table.
>>>>
>>>> However, the code currently only remembers the last interrupt in
>>>> a resource, and will leak the others if there is more than one.
>>>> This can only happen for board-file driven platforms and not via DT,
>>>> however.
>>>>
>>>> Also, there is currently no bounds check, while priv->irqs_table is a
>>>> fixed-size array. If we are passed more than 4 resources, we're in
>>>> trouble.
>>>>
>>>> This patch introduces a bounds check and changes the way interrupt
>>>> indices are kept. Tested on a Beagle Bone Black board only.
>>>>
>>>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>>>
>>> The drivers is not supported for non-DT platforms as all the platforms
>>> which uses CPSW are DT only platforms.
>>
>> Ok, thanks for explaining.
>>
>> But then we can remove the iteration then and simplify the code, right?
>> The bounds check should also be done.
>>
> 
> Right, we can simplify the code.

Ok, I'll cook up a new patch.


Thanks!
Daniel