Message ID | CAOMZO5CRQeuF_ERZ0v2uocuADXRWscKLN0KqdDHfu+0Ub2MEhw@mail.gmail.com |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
2013/2/19 Fabio Estevam <festevam@gmail.com>: > On Mon, Feb 18, 2013 at 10:54 PM, Frank Li <lznuaa@gmail.com> wrote: > >> This is not correct fix. >> Read, OR and write is not atomic. Need lock. >> But if use hw_lock, it cause dead lock. >> I spend some time to remove such lock. > > Ok, could you please try the patch below? It avoids the kernel crash > on my mx53qsb: This is similar with my first implement. It just protect irq and init function. Many place write FEC_IMASK directly. > > drivers/net/ethernet/freescale/fec.c | 23 ++++++++++++++++++++--- > 1 file changed, 20 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec.c > b/drivers/net/ethernet/freescale/fec.c > index 0fe68c4..4df6518 100644 > --- a/drivers/net/ethernet/freescale/fec.c > +++ b/drivers/net/ethernet/freescale/fec.c > @@ -804,6 +804,24 @@ rx_processing_done: > return pkt_received; > } > > +static void fec_control_rx(struct net_device *ndev, bool enable) > +{ > + struct fec_enet_private *fep = netdev_priv(ndev); > + int reg; > + unsigned long flags; > + > + spin_lock_irqsave(&fep->hw_lock, flags); > + reg = readl(fep->hwp + FEC_IMASK); > + if (enable) > + reg |= FEC_ENET_RXF; > + else > + reg &= ~FEC_ENET_RXF; > + > + writel(reg, fep->hwp + FEC_IMASK); > + > + spin_unlock_irqrestore(&fep->hw_lock, flags); > +} > + > static irqreturn_t > fec_enet_interrupt(int irq, void *dev_id) > { > @@ -821,8 +839,7 @@ fec_enet_interrupt(int irq, void *dev_id) > > /* Disable the RX interrupt */ > if (napi_schedule_prep(&fep->napi)) { > - writel(FEC_RX_DISABLED_IMASK, > - fep->hwp + FEC_IMASK); > + fec_control_rx(ndev, false); > __napi_schedule(&fep->napi); > } > } > @@ -1628,7 +1645,7 @@ static int fec_enet_init(struct net_device *ndev) > ndev->netdev_ops = &fec_netdev_ops; > ndev->ethtool_ops = &fec_enet_ethtool_ops; > > - writel(FEC_RX_DISABLED_IMASK, fep->hwp + FEC_IMASK); > + fec_control_rx(ndev, false); I think this issue was caused by enable irq before register irq handler. writel(0, fep->hwp + FEC_IMASK) should fix mx53's problem. best regards Frank Li > netif_napi_add(ndev, &fep->napi, fec_enet_rx_napi, FEC_NAPI_WEIGHT); > > /* Initialize the receive buffer descriptors. */ > -- > 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 18, 2013 at 11:47 PM, Frank Li <lznuaa@gmail.com> wrote: > This is similar with my first implement. I checked your original implementation and spin_lock_irqsave/spin_unlock_irqrestore was very far apart. > It just protect irq and init function. Many place write FEC_IMASK directly. Yes, but the main issue with current code is that it does not clear the only rx irq as it should. > I think this issue was caused by enable irq before register irq handler. > writel(0, fep->hwp + FEC_IMASK) should fix mx53's problem. No, it does not solve the issue. I don't have a mx6q board handy. If you could try this patch and let me know if you see any deadlock, it would be nice. Thanks, Fabio Estevam -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2013/2/19 Fabio Estevam <festevam@gmail.com>: > On Mon, Feb 18, 2013 at 11:47 PM, Frank Li <lznuaa@gmail.com> wrote: > >> This is similar with my first implement. > > I checked your original implementation and > spin_lock_irqsave/spin_unlock_irqrestore was very far apart. > >> It just protect irq and init function. Many place write FEC_IMASK directly. > > Yes, but the main issue with current code is that it does not clear > the only rx irq as it should. The key is Read OR and write need lock. I try reduce FEC lock. FEC_IMASK should be two state, one is FEC_DEFAULT_IMASK, the other is FEC_RX_DISABLED_IMASK. FEC_RX_DISABLED_IMASK is equal as clean rx irq only. I will found mx53 to found the real root cause > >> I think this issue was caused by enable irq before register irq handler. >> writel(0, fep->hwp + FEC_IMASK) should fix mx53's problem. > > No, it does not solve the issue. > > I don't have a mx6q board handy. If you could try this patch and let > me know if you see any deadlock, it would be nice. > > Thanks, > > Fabio Estevam -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c index 0fe68c4..4df6518 100644 --- a/drivers/net/ethernet/freescale/fec.c +++ b/drivers/net/ethernet/freescale/fec.c @@ -804,6 +804,24 @@ rx_processing_done: return pkt_received; } +static void fec_control_rx(struct net_device *ndev, bool enable) +{ + struct fec_enet_private *fep = netdev_priv(ndev); + int reg; + unsigned long flags; + + spin_lock_irqsave(&fep->hw_lock, flags); + reg = readl(fep->hwp + FEC_IMASK); + if (enable) + reg |= FEC_ENET_RXF; + else + reg &= ~FEC_ENET_RXF; + + writel(reg, fep->hwp + FEC_IMASK); + + spin_unlock_irqrestore(&fep->hw_lock, flags); +} + static irqreturn_t fec_enet_interrupt(int irq, void *dev_id) { @@ -821,8 +839,7 @@ fec_enet_interrupt(int irq, void *dev_id) /* Disable the RX interrupt */ if (napi_schedule_prep(&fep->napi)) { - writel(FEC_RX_DISABLED_IMASK, - fep->hwp + FEC_IMASK); + fec_control_rx(ndev, false); __napi_schedule(&fep->napi); } } @@ -1628,7 +1645,7 @@ static int fec_enet_init(struct net_device *ndev) ndev->netdev_ops = &fec_netdev_ops; ndev->ethtool_ops = &fec_enet_ethtool_ops; - writel(FEC_RX_DISABLED_IMASK, fep->hwp + FEC_IMASK); + fec_control_rx(ndev, false); netif_napi_add(ndev, &fep->napi, fec_enet_rx_napi, FEC_NAPI_WEIGHT); /* Initialize the receive buffer descriptors. */