| Message ID | 1232460114-13469-1-git-send-email-steve.glendinning@smsc.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Steve Glendinning <steve.glendinning@smsc.com> Date: Tue, 20 Jan 2009 14:01:54 +0000 > This patch adds a new spinlock to protect read-modify-writes to the > INT_EN register when enabling and disabling interrupt sources. > > I haven't actually seen any devices lock up, but I think there's a > possibility and I'd like to eliminate it. > > Should I add this new spinlock, or should I extend the mac_lock > to also cover these sections (renaming it to simply "lock")? > > Signed-off-by: Steve Glendinning <steve.glendinning@smsc.com> If you didn't have all of these RX multicast workaround cases, things would be much easier. The POLL and normal interrupt paths are already completely atomic for you already. And since POLL cannot happen until open() completes that path would be safe too. -- 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 Tue, Jan 20, 2009 at 6:01 AM, Steve Glendinning <steve.glendinning@smsc.com> wrote: > This patch adds a new spinlock to protect read-modify-writes to the > INT_EN register when enabling and disabling interrupt sources. > > I haven't actually seen any devices lock up, but I think there's a > possibility and I'd like to eliminate it. I'm attempting to use this driver on a new Gumstix Overo daughtercard that uses an SMCS9221 and have been having some interrupt issues that may or may not be related. I am using the version in linux-omap 2.6.29-rc2. Here's what I see at boot: smsc911x: Driver version 2008-10-21. eth%d: smsc911x_init: Driver Parameters: eth%d: smsc911x_init: LAN base: 0xD086E000 eth%d: smsc911x_init: IRQ: 336 eth%d: smsc911x_init: PHY will be autodetected. eth%d: smsc911x_init: BYTE_TEST: 0x87654321 eth%d: smsc911x_init: LAN911x identified, idrev: 0x92210000, generation: 4 eth0: smsc911x_drv_probe: Network interface: "eth0" eth0: smsc911x_mii_init: External PHY is not supported, using internal PHY smsc911x-mdio: probed eth0: smsc911x_mii_probe: PHY 1: addr 1, phy_id 0x0007C0C3 eth0: attached PHY driver [SMSC LAN8700] (mii_bus:phy_addr=ffffffff:01, irq=-1) eth0: smsc911x_phy_check_loopbackpkt: Successfully verified loopback packet eth0: smsc911x_mii_probe: Passed Loop Back Test eth0: smsc911x_mii_probe: phy initialised succesfully eth0: smsc911x_drv_probe: MAC Address is set to random_ether_addr net eth0: MAC Address: ae:b1:a7:f7:89:c2 <snip> Configuring network interfaces... eth0: smsc911x_open: irq polarity: active low eth0: smsc911x_open: irq type: open drain eth0: smsc911x_open: Testing irq handler using IRQ 336 net eth0: ISR failed signaling test (IRQ 336) I have a scope probe hanging on the IRQ signal. I can see that IRQ is high at power on, then I see a flurry of interrupts, then at around the time the "testing irq" message comes out I see IRQ go low and remain low. This behavior is 100% reproducible. I tried with and without this patch, same behavior. Any ideas what might be going wrong? Steve -- 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
David Miller <davem@davemloft.net> wrote on 26/01/2009 01:47:36: > From: Steve Glendinning <steve.glendinning@smsc.com> > Date: Tue, 20 Jan 2009 14:01:54 +0000 > > > This patch adds a new spinlock to protect read-modify-writes to the > > INT_EN register when enabling and disabling interrupt sources. > > > > I haven't actually seen any devices lock up, but I think there's a > > possibility and I'd like to eliminate it. > > > > Should I add this new spinlock, or should I extend the mac_lock > > to also cover these sections (renaming it to simply "lock")? > > > > Signed-off-by: Steve Glendinning <steve.glendinning@smsc.com> > > If you didn't have all of these RX multicast workaround > cases, things would be much easier. > > The POLL and normal interrupt paths are already completely atomic for > you already. And since POLL cannot happen until open() completes > that path would be safe too. > Thanks for looking this over David. It looks like I can do away with the need for this if I leave RXSTOP_INT permanently enabled and use a flag in pdata to indicate whether the ISR should do a multicast filter update. Does this sound reasonable? Please drop this patch. Steve -- 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/smsc911x.c b/drivers/net/smsc911x.c index f513bdf..3f52d23 100644 --- a/drivers/net/smsc911x.c +++ b/drivers/net/smsc911x.c @@ -88,6 +88,9 @@ struct smsc911x_data { * unused with a 32-bit bus */ spinlock_t dev_lock; + /* spinlock to protect INT_EN read-modify-writes */ + spinlock_t int_lock; + struct phy_device *phy_dev; struct mii_bus *mii_bus; int phy_irq[PHY_MAX_ADDR]; @@ -981,13 +984,16 @@ static int smsc911x_poll(struct napi_struct *napi, int budget) if (!rxstat) { unsigned int temp; + unsigned long flags; /* We processed all packets available. Tell NAPI it can * stop polling then re-enable rx interrupts */ smsc911x_reg_write(pdata, INT_STS, INT_STS_RSFL_); netif_rx_complete(napi); + spin_lock_irqsave(&pdata->int_lock, flags); temp = smsc911x_reg_read(pdata, INT_EN); temp |= INT_EN_RSFL_EN_; smsc911x_reg_write(pdata, INT_EN, temp); + spin_unlock_irqrestore(&pdata->int_lock, flags); break; } @@ -1178,7 +1184,9 @@ static int smsc911x_open(struct net_device *dev) spin_unlock_irq(&pdata->mac_lock); /* Initialise irqs, but leave all sources disabled */ + spin_lock_irq(&pdata->int_lock); smsc911x_reg_write(pdata, INT_EN, 0); + spin_unlock_irq(&pdata->int_lock); smsc911x_reg_write(pdata, INT_STS, 0xFFFFFFFF); /* Set interrupt deassertion to 100uS */ @@ -1204,9 +1212,11 @@ static int smsc911x_open(struct net_device *dev) pdata->software_irq_signal = 0; smp_wmb(); + spin_lock_irq(&pdata->int_lock); temp = smsc911x_reg_read(pdata, INT_EN); temp |= INT_EN_SW_INT_EN_; smsc911x_reg_write(pdata, INT_EN, temp); + spin_unlock_irq(&pdata->int_lock); timeout = 1000; while (timeout--) { @@ -1245,9 +1255,11 @@ static int smsc911x_open(struct net_device *dev) /* enable NAPI polling before enabling RX interrupts */ napi_enable(&pdata->napi); + spin_lock_irq(&pdata->int_lock); temp = smsc911x_reg_read(pdata, INT_EN); temp |= (INT_EN_TDFA_EN_ | INT_EN_RSFL_EN_); smsc911x_reg_write(pdata, INT_EN, temp); + spin_unlock_irq(&pdata->int_lock); spin_lock_irq(&pdata->mac_lock); temp = smsc911x_mac_read(pdata, MAC_CR); @@ -1419,9 +1431,11 @@ static void smsc911x_set_multicast_list(struct net_device *dev) /* Request the hardware to stop, then perform the * update when we get an RX_STOP interrupt */ smsc911x_reg_write(pdata, INT_STS, INT_STS_RXSTOP_INT_); + spin_lock(&pdata->int_lock); temp = smsc911x_reg_read(pdata, INT_EN); temp |= INT_EN_RXSTOP_INT_EN_; smsc911x_reg_write(pdata, INT_EN, temp); + spin_unlock(&pdata->int_lock); temp = smsc911x_mac_read(pdata, MAC_CR); temp &= ~(MAC_CR_RXEN_); @@ -1445,12 +1459,15 @@ static irqreturn_t smsc911x_irqhandler(int irq, void *dev_id) u32 intsts = smsc911x_reg_read(pdata, INT_STS); u32 inten = smsc911x_reg_read(pdata, INT_EN); int serviced = IRQ_NONE; + unsigned long flags; u32 temp; if (unlikely(intsts & inten & INT_STS_SW_INT_)) { + spin_lock_irqsave(&pdata->int_lock, flags); temp = smsc911x_reg_read(pdata, INT_EN); temp &= (~INT_EN_SW_INT_EN_); smsc911x_reg_write(pdata, INT_EN, temp); + spin_unlock_irqrestore(&pdata->int_lock, flags); smsc911x_reg_write(pdata, INT_STS, INT_STS_SW_INT_); pdata->software_irq_signal = 1; smp_wmb(); @@ -1461,9 +1478,11 @@ static irqreturn_t smsc911x_irqhandler(int irq, void *dev_id) /* Called when there is a multicast update scheduled and * it is now safe to complete the update */ SMSC_TRACE(INTR, "RX Stop interrupt"); + spin_lock_irqsave(&pdata->int_lock, flags); temp = smsc911x_reg_read(pdata, INT_EN); temp &= (~INT_EN_RXSTOP_INT_EN_); smsc911x_reg_write(pdata, INT_EN, temp); + spin_unlock_irqrestore(&pdata->int_lock, flags); smsc911x_reg_write(pdata, INT_STS, INT_STS_RXSTOP_INT_); smsc911x_rx_multicast_update_workaround(pdata); serviced = IRQ_HANDLED; @@ -1487,9 +1506,11 @@ static irqreturn_t smsc911x_irqhandler(int irq, void *dev_id) if (likely(intsts & inten & INT_STS_RSFL_)) { if (likely(netif_rx_schedule_prep(&pdata->napi))) { /* Disable Rx interrupts */ + spin_lock_irqsave(&pdata->int_lock, flags); temp = smsc911x_reg_read(pdata, INT_EN); temp &= (~INT_EN_RSFL_EN_); smsc911x_reg_write(pdata, INT_EN, temp); + spin_unlock_irqrestore(&pdata->int_lock, flags); /* Schedule a NAPI poll */ __netif_rx_schedule(&pdata->napi); } else { @@ -1760,6 +1781,7 @@ static int __devinit smsc911x_init(struct net_device *dev) SMSC_TRACE(PROBE, "PHY will be autodetected."); spin_lock_init(&pdata->dev_lock); + spin_lock_init(&pdata->int_lock); if (pdata->ioaddr == 0) { SMSC_WARNING(PROBE, "pdata->ioaddr: 0x00000000");
This patch adds a new spinlock to protect read-modify-writes to the INT_EN register when enabling and disabling interrupt sources. I haven't actually seen any devices lock up, but I think there's a possibility and I'd like to eliminate it. Should I add this new spinlock, or should I extend the mac_lock to also cover these sections (renaming it to simply "lock")? Signed-off-by: Steve Glendinning <steve.glendinning@smsc.com> --- drivers/net/smsc911x.c | 22 ++++++++++++++++++++++ 1 files changed, 22 insertions(+), 0 deletions(-)