[RFC PATCH] smsc911x: protect INT_EN read-modify-write with spinlock

Subsystems: networking drivers, the rest

STALE6443d

4 messages, 3 authors, 2009-01-26 · open the first message on its own page

[RFC PATCH] smsc911x: protect INT_EN read-modify-write with spinlock

From: Steve Glendinning <hidden>
Date: 2009-01-20 14:01:01

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 <redacted>
---
 drivers/net/smsc911x.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

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");

-- 
1.6.0.6

Re: [RFC PATCH] smsc911x: protect INT_EN read-modify-write with spinlock

From: David Miller <davem@davemloft.net>
Date: 2009-01-26 01:47:34

From: Steve Glendinning <redacted>
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 <redacted>

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.

Re: [RFC PATCH] smsc911x: protect INT_EN read-modify-write with spinlock

From: Steve Sakoman <sakoman@gmail.com>
Date: 2009-01-26 04:43:31

On Tue, Jan 20, 2009 at 6:01 AM, Steve Glendinning
[off-list ref] 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

Re: [RFC PATCH] smsc911x: protect INT_EN read-modify-write with spinlock

From: <hidden>
Date: 2009-01-26 09:58:43

David Miller [off-list ref] wrote on 26/01/2009 01:47:36:

From: Steve Glendinning <redacted>
Date: Tue, 20 Jan 2009 14:01:54 +0000

quoted

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 <redacted>

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help