[PATCH] net: fec: Ensure that initialization is done prior to request_irq()

[PATCH] net: fec: Ensure that initialization is done prior to request_irq()

[Fabio Estevam]

Currently request_irq() is called prior to fec_enet_init() and fec_ptp_init(), 
which causes the following crash on a mx53qsb:

Unable to handle kernel NULL pointer dereference at virtual address 00000002
pgd = 80004000
[00000002] *pgd=00000000
Internal error: Oops: 5 [#1] SMP ARM
Modules linked in:
CPU: 0    Not tainted  (3.8.0-rc7-next-20130215+ #346)
PC is at fec_enet_interrupt+0xd0/0x348
LR is at fec_enet_interrupt+0xb8/0x348
pc : [<80372b7c>]    lr : [<80372b64>]    psr: 60000193
sp : df855c20  ip : df855c20  fp : df855c74
r10: 00000516  r9 : 1c000000  r8 : 00000000
r7 : 00000000  r6 : 00000000  r5 : 00000000  r4 : df9b7800
r3 : df9b7df4  r2 : 00000000  r1 : 00000000  r0 : df9b7d34

Ensure that such initialization functions are called prior to requesting the 
interrupts, so that all necessary the data structures are in place when the 
irqs occur. 

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 drivers/net/ethernet/freescale/fec.c |   30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
index 29d82cf..2dbb36c 100644
--- a/drivers/net/ethernet/freescale/fec.c
+++ b/drivers/net/ethernet/freescale/fec.c
@@ -1782,6 +1782,15 @@ fec_probe(struct platform_device *pdev)
 		fep->phy_interface = ret;
 	}
 
+	fep->bufdesc_ex =
+		pdev->id_entry->driver_data & FEC_QUIRK_HAS_BUFDESC_EX;
+	if (fep->bufdesc_ex)
+		fec_ptp_init(ndev, pdev);
+
+	ret = fec_enet_init(ndev);
+	if (ret)
+		goto failed_init;
+
 	for (i = 0; i < FEC_IRQ_NUM; i++) {
 		irq = platform_get_irq(pdev, i);
 		if (irq < 0) {
@@ -1819,8 +1828,6 @@ fec_probe(struct platform_device *pdev)
 	}
 
 	fep->clk_ptp = devm_clk_get(&pdev->dev, "ptp");
-	fep->bufdesc_ex =
-		pdev->id_entry->driver_data & FEC_QUIRK_HAS_BUFDESC_EX;
 	if (IS_ERR(fep->clk_ptp)) {
 		ret = PTR_ERR(fep->clk_ptp);
 		fep->bufdesc_ex = 0;
@@ -1843,13 +1850,6 @@ fec_probe(struct platform_device *pdev)
 
 	fec_reset_phy(pdev);
 
-	if (fep->bufdesc_ex)
-		fec_ptp_init(ndev, pdev);
-
-	ret = fec_enet_init(ndev);
-	if (ret)
-		goto failed_init;
-
 	ret = fec_enet_mii_init(pdev);
 	if (ret)
 		goto failed_mii_init;
@@ -1866,7 +1866,6 @@ fec_probe(struct platform_device *pdev)
 failed_register:
 	fec_enet_mii_remove(fep);
 failed_mii_init:
-failed_init:
 failed_regulator:
 	clk_disable_unprepare(fep->clk_ahb);
 	clk_disable_unprepare(fep->clk_ipg);
@@ -1881,6 +1880,7 @@ failed_clk:
 	}
 failed_irq:
 	iounmap(fep->hwp);
+failed_init:
 failed_ioremap:
 	free_netdev(ndev);
 failed_alloc_etherdev:
@@ -1899,17 +1899,17 @@ fec_drv_remove(struct platform_device *pdev)
 
 	unregister_netdev(ndev);
 	fec_enet_mii_remove(fep);
-	for (i = 0; i < FEC_IRQ_NUM; i++) {
-		int irq = platform_get_irq(pdev, i);
-		if (irq > 0)
-			free_irq(irq, ndev);
-	}
 	del_timer_sync(&fep->time_keep);
 	clk_disable_unprepare(fep->clk_ptp);
 	if (fep->ptp_clock)
 		ptp_clock_unregister(fep->ptp_clock);
 	clk_disable_unprepare(fep->clk_ahb);
 	clk_disable_unprepare(fep->clk_ipg);
+	for (i = 0; i < FEC_IRQ_NUM; i++) {
+		int irq = platform_get_irq(pdev, i);
+		if (irq > 0)
+			free_irq(irq, ndev);
+	}
 	iounmap(fep->hwp);
 	free_netdev(ndev);
 
-- 
1.7.9.5

Re: [PATCH] net: fec: Ensure that initialization is done prior to request_irq()

[David Miller]

From: Fabio Estevam <fabio.estevam@freescale.com>
Date: Thu, 21 Feb 2013 16:21:50 -0300

> Currently request_irq() is called prior to fec_enet_init() and fec_ptp_init(), 
> which causes the following crash on a mx53qsb:
> 
> Unable to handle kernel NULL pointer dereference at virtual address 00000002
> pgd = 80004000
> [00000002] *pgd=00000000
> Internal error: Oops: 5 [#1] SMP ARM
> Modules linked in:
> CPU: 0    Not tainted  (3.8.0-rc7-next-20130215+ #346)
> PC is at fec_enet_interrupt+0xd0/0x348
> LR is at fec_enet_interrupt+0xb8/0x348
> pc : [<80372b7c>]    lr : [<80372b64>]    psr: 60000193
> sp : df855c20  ip : df855c20  fp : df855c74
> r10: 00000516  r9 : 1c000000  r8 : 00000000
> r7 : 00000000  r6 : 00000000  r5 : 00000000  r4 : df9b7800
> r3 : df9b7df4  r2 : 00000000  r1 : 00000000  r0 : df9b7d34
> 
> Ensure that such initialization functions are called prior to requesting the 
> interrupts, so that all necessary the data structures are in place when the 
> irqs occur. 
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>

Looks good, did you test this yourself or should we wait for others to
test it out?

Re: [PATCH] net: fec: Ensure that initialization is done prior to request_irq()

[Fabio Estevam]

On Thu, Feb 21, 2013 at 5:27 PM, David Miller <davem@davemloft.net> wrote:

> Looks good, did you test this yourself or should we wait for others to
> test it out?

Yes, I tested it myself. I was the one who reported this bug and this
fixes the problem.

Re: [PATCH] net: fec: Ensure that initialization is done prior to request_irq()

[David Miller]

From: Fabio Estevam <festevam@gmail.com>
Date: Thu, 21 Feb 2013 17:29:46 -0300

> On Thu, Feb 21, 2013 at 5:27 PM, David Miller <davem@davemloft.net> wrote:
> 
>> Looks good, did you test this yourself or should we wait for others to
>> test it out?
> 
> Yes, I tested it myself. I was the one who reported this bug and this
> fixes the problem.

Perfect, applied to 'net', thanks!

Re: [PATCH] net: fec: Ensure that initialization is done prior to request_irq()

[Jim Baxter]

David Miller <davem <at> davemloft.net> writes:

> 
> From: Fabio Estevam <festevam <at> gmail.com>
> Date: Thu, 21 Feb 2013 17:29:46 -0300
> 
> > On Thu, Feb 21, 2013 at 5:27 PM, David Miller <davem <at> davemloft.net> 
wrote:
> > 
> >> Looks good, did you test this yourself or should we wait for others to
> >> test it out?
> > 
> > Yes, I tested it myself. I was the one who reported this bug and this
> > fixes the problem.
> 
> Perfect, applied to 'net', thanks!
> 


There is a divide by zero error.

If I enable these 3 kernel hacking options;
CONFIG_DEBUG_SPINLOCK:   -- Spinlock and rw-lock debugging: basic checks
CONFIG_DEBUG_LOCK_ALLOC: -- Lock debugging: detect incorrect freeing of live 
locks
CONFIG_PROVE_LOCKING:    -- Lock debugging: prove locking correctness


I get this error:


[   17.895723] Division by zero in kernel.
[   17.899571] Backtrace: 
[   17.902094] [<80012564>] (dump_backtrace+0x0/0x10c) from [<8056deec>] 
(dump_stack+0x18/0x1c)
[   17.910539]  r6:bfba8500 r5:8075c950 r4:bfba8000 r3:bfbd0000
[   17.916284] [<8056ded4>] (dump_stack+0x0/0x1c) from [<80012688>] 
(__div0+0x18/0x20)
[   17.923968] [<80012670>] (__div0+0x0/0x20) from [<802829c4>] (Ldiv0+0x8/0x10)
[   17.931140] [<80398534>] (fec_ptp_start_cyclecounter+0x0/0x110) from 
[<80394f64>] (fec_restart+0x6c8/0x754)
[   17.940898] [<8039489c>] (fec_restart+0x0/0x754) from [<803969a0>] 
(fec_enet_adjust_link+0xdc/0x108)
[   17.950046] [<803968c4>] (fec_enet_adjust_link+0x0/0x108) from [<80390bc4>] 
(phy_state_machine+0x178/0x534)
[   17.959791]  r8:00000000 r7:00000000 r6:bf80da5c r5:bf80da04 r4:bf80d800
r3:00000001
[   17.967750] [<80390a4c>] (phy_state_machine+0x0/0x534) from [<8003be40>] 
(process_one_work+0x1c0/0x464)
[   17.977147]  r8:00000000 r7:8150a300 r6:815071c0 r5:bf850c00 r4:bf80da04
r3:bfbd0000
[   17.985097] [<8003bc80>] (process_one_work+0x0/0x464) from [<8003ea64>] 
(worker_thread+0x134/0x3f0)
[   17.994165] [<8003e930>] (worker_thread+0x0/0x3f0) from [<8004420c>] 
(kthread+0xac/0xb8)
[   18.002271] [<80044160>] (kthread+0x0/0xb8) from [<8000ebf0>] 
(ret_from_fork+0x14/0x24)
[   18.010280]  r7:00000000 r6:00000000 r5:80044160 r4:bf875e10

Re: [PATCH] net: fec: Ensure that initialization is done prior to request_irq()

[Frank Li]

This bug should be fixed in

http://git.kernel.org/?p=linux/kernel/git/davem/net-next.git;a=commit;h=365cc174648cd7e9b11e522c3d5b07ccf3c60d99

2013/2/22 Jim Baxter <jim_baxter@mentor.com>:
> here is a divide by zero error.
>
> If I enable these 3 kernel hacking options;
> CONFIG_DEBUG_SPINLOCK:   -- Spinlock and rw-lock debugging: basic checks
> CONFIG_DEBUG_LOCK_ALLOC: -- Lock debugging: detect incorrect freeing of live
> locks
> CONFIG_PROVE_LOCKING:    -- Lock debugging: prove locking correctness

Re: [PATCH] net: fec: Ensure that initialization is done prior to request_irq()

[Fabio Estevam]

On Thu, Feb 21, 2013 at 10:13 PM, Jim Baxter <jim_baxter@mentor.com> wrote:

> There is a divide by zero error.
>
> If I enable these 3 kernel hacking options;
> CONFIG_DEBUG_SPINLOCK:   -- Spinlock and rw-lock debugging: basic checks
> CONFIG_DEBUG_LOCK_ALLOC: -- Lock debugging: detect incorrect freeing of live
> locks
> CONFIG_PROVE_LOCKING:    -- Lock debugging: prove locking correctness

Thanks for reporting this issue, Jim.

I was testing under mx53, which did not show this problem.

I have a fix for this and I have tested it on both mx6q and mx53. Will
submit it shortly.