Re: [BUG] fec_mpc52xx: Don't call mpc52xx_fec_reset() in ISR
From: Wolfgang Grandegger <hidden>
Date: 2008-08-13 14:09:45
Hi Jürgen, Juergen Beisert wrote:
On Donnerstag, 10. Juli 2008, Grant Likely wrote:quoted
On Thu, Jul 10, 2008 at 02:39:09PM +0200, Wolfram Sang wrote:quoted
Hello, today, I was debugging a kernel crash on a board with a MPC5200B using 2.6.26-rc9. I found the following code in drivers/net/fec_mpc52xx.c:<snip>quoted
I assume the proper thing to do is to set a flag in the ISR and handle the soft reset later in some other context. Having never dealt with the network core and its drivers so far, I am not sure which place would be the right one to perform the soft reset. To not make things worse, I hope people with more insight to network stuff can deliver a suitable solution to this problem.Thanks for the bug report. I'll take a look.Some update: Enabling XLB pipelining let occure this error less often. Kernel disables this feature by default yet. The comment talks about "cfr errate 292." that is valid for MPC5200A, but _it_seems_ no longer for MPC5200B. Has anybody experience if we can enabling pipelining on MPC5200B CPUs without triggering this bug?
No, I haven't...
quoted hunk ↗ jump to hunk
We currently are playing with this setting: Index: arch/powerpc/platforms/52xx/mpc52xx_common.c ===================================================================--- arch/powerpc/platforms/52xx/mpc52xx_common.c.orig +++ arch/powerpc/platforms/52xx/mpc52xx_common.c@@ -99,11 +99,11 @@ out_be32(&xlb->master_pri_enable, 0xff); out_be32(&xlb->master_priority, 0x11111111); - /* Disable XLB pipelining - * (cfr errate 292. We could do this only just before ATA PIO - * transaction and re-enable it afterwards ...) + /* + * Enable pipelining, fixes FEC problems. The previous workaround seems + * not needed, as we have an MPC5200B (not A). */ - out_be32(&xlb->config, in_be32(&xlb->config) | MPC52xx_XLB_CFG_PLDIS); + out_be32(&xlb->config, in_be32(&xlb->config) & ~MPC52xx_XLB_CFG_PLDIS); iounmap(xlb); }
...but I prepared a patch to do the reset in the process context. Would be nice if you could give the patch below a try. Wolfgang. =================================================================== From: Wolfgang Grandegger <redacted> Subject: [PATCH] powerpc: fec_mpc52xx: Don't call mpc52xx_fec_reset() in ISR Calling mpc52xx_fec_reset() in the ISR is a bug. This patch puts a task for resetting the FEC into the kernel-global workqueue to be processed lateron savely in process context. Signed-off-by: Wolfgang Grandegger <redacted> --- drivers/net/fec_mpc52xx.c | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-) Index: linux-2.6.26/drivers/net/fec_mpc52xx.c ===================================================================
--- linux-2.6.26.orig/drivers/net/fec_mpc52xx.c
+++ linux-2.6.26/drivers/net/fec_mpc52xx.c@@ -24,6 +24,7 @@ #include <linux/crc32.h> #include <linux/hardirq.h> #include <linux/delay.h> +#include <linux/workqueue.h> #include <linux/of_device.h> #include <linux/of_platform.h>
@@ -57,6 +58,8 @@ struct mpc52xx_fec_priv { struct bcom_task *tx_dmatsk; spinlock_t lock; int msg_enable; + struct work_struct reset_task; + struct net_device *ndev; /* MDIO link details */ int phy_addr;
@@ -83,6 +86,19 @@ static int debug = -1; /* the above defa module_param(debug, int, 0); MODULE_PARM_DESC(debug, "debugging messages level"); +static void mpc52xx_fec_reset_task(struct work_struct *work) +{ + struct mpc52xx_fec_priv *priv = + container_of(work, struct mpc52xx_fec_priv, reset_task); + struct net_device *dev = priv->ndev; + + dev_warn(&dev->dev, "deferred FEC reset due to errors\n"); + + mpc52xx_fec_reset(dev); + + netif_wake_queue(dev); +} + static void mpc52xx_fec_tx_timeout(struct net_device *dev) { dev_warn(&dev->dev, "transmit timed out\n");
@@ -355,6 +371,8 @@ static int mpc52xx_fec_close(struct net_ { struct mpc52xx_fec_priv *priv = netdev_priv(dev); + flush_scheduled_work(); + netif_stop_queue(dev); mpc52xx_fec_stop(dev);
@@ -522,9 +540,9 @@ static irqreturn_t mpc52xx_fec_interrupt if (net_ratelimit() && (ievent & FEC_IEVENT_XFIFO_ERROR)) dev_warn(&dev->dev, "FEC_IEVENT_XFIFO_ERROR\n"); - mpc52xx_fec_reset(dev); - - netif_wake_queue(dev); + netif_stop_queue(dev); + netif_carrier_off(dev); + schedule_work(&priv->reset_task); return IRQ_HANDLED; }
@@ -934,7 +952,9 @@ mpc52xx_fec_probe(struct of_device *op, priv->t_irq = priv->r_irq = ndev->irq = NO_IRQ; /* IRQ are free for now */ - spin_lock_init(&priv->lock); + priv->ndev = ndev; + spin_lock_init(&priv->lock); + INIT_WORK(&priv->reset_task, mpc52xx_fec_reset_task); /* ioremap the zones */ priv->fec = ioremap(mem.start, sizeof(struct mpc52xx_fec));
@@ -1068,6 +1088,9 @@ mpc52xx_fec_remove(struct of_device *op) ndev = dev_get_drvdata(&op->dev); priv = netdev_priv(ndev); + if (netif_running(ndev)) + mpc52xx_fec_close(ndev); + unregister_netdev(ndev); irq_dispose_mapping(ndev->irq);