[PATCH] PHYLIB: Spinlock fixes for softirqs

[PATCH] PHYLIB: Spinlock fixes for softirqs

[Maciej W. Rozycki]

 Use spin_lock_bh()/spin_unlock_bh() for the phydev lock throughout as it 
is used in phy_timer() that is called as a softirq and all the other 
operations may happen in the user context.

Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
---
 There has been a change recently that did such a conversion for some of 
the operations on the lock, but some have been left intact.  Many of them, 
perhaps all, may be called in the user context and I was able to trigger 
recursive spinlock acquisition indeed, so I think for the sake of 
long-term maintenance it is best to convert them all, even if 
unnecessarily for one or two -- better safe than sorry.

 Perhaps one in phy_timer() could actually be skipped as only called as a 
softirq -- I can send an update if that sounds like a good idea.

 Checked with checkpatch.pl and at the runtime.

 Please apply,

  Maciej

patch-netdev-2.6.23-rc6-20070913-phy-spinlock-8 
diff -up --recursive --new-file linux-netdev-2.6.23-rc6-20070913.macro/drivers/net/phy/phy.c linux-netdev-2.6.23-rc6-20070913/drivers/net/phy/phy.c
--- linux-netdev-2.6.23-rc6-20070913.macro/drivers/net/phy/phy.c	2007-09-13 17:27:52.000000000 +0000
+++ linux-netdev-2.6.23-rc6-20070913/drivers/net/phy/phy.c	2007-09-16 17:26:02.000000000 +0000
@@ -423,7 +423,7 @@ int phy_start_aneg(struct phy_device *ph
 {
 	int err;
 
-	spin_lock(&phydev->lock);
+	spin_lock_bh(&phydev->lock);
 
 	if (AUTONEG_DISABLE == phydev->autoneg)
 		phy_sanitize_settings(phydev);
@@ -444,7 +444,7 @@ int phy_start_aneg(struct phy_device *ph
 	}
 
 out_unlock:
-	spin_unlock(&phydev->lock);
+	spin_unlock_bh(&phydev->lock);
 	return err;
 }
 EXPORT_SYMBOL(phy_start_aneg);
@@ -489,10 +489,10 @@ void phy_stop_machine(struct phy_device 
 {
 	del_timer_sync(&phydev->phy_timer);
 
-	spin_lock(&phydev->lock);
+	spin_lock_bh(&phydev->lock);
 	if (phydev->state > PHY_UP)
 		phydev->state = PHY_UP;
-	spin_unlock(&phydev->lock);
+	spin_unlock_bh(&phydev->lock);
 
 	phydev->adjust_state = NULL;
 }
@@ -536,9 +536,9 @@ static void phy_force_reduction(struct p
  */
 void phy_error(struct phy_device *phydev)
 {
-	spin_lock(&phydev->lock);
+	spin_lock_bh(&phydev->lock);
 	phydev->state = PHY_HALTED;
-	spin_unlock(&phydev->lock);
+	spin_unlock_bh(&phydev->lock);
 }
 
 /**
@@ -689,10 +689,10 @@ static void phy_change(struct work_struc
 	if (err)
 		goto phy_err;
 
-	spin_lock(&phydev->lock);
+	spin_lock_bh(&phydev->lock);
 	if ((PHY_RUNNING == phydev->state) || (PHY_NOLINK == phydev->state))
 		phydev->state = PHY_CHANGELINK;
-	spin_unlock(&phydev->lock);
+	spin_unlock_bh(&phydev->lock);
 
 	enable_irq(phydev->irq);
 
@@ -717,7 +717,7 @@ phy_err:
  */
 void phy_stop(struct phy_device *phydev)
 {
-	spin_lock(&phydev->lock);
+	spin_lock_bh(&phydev->lock);
 
 	if (PHY_HALTED == phydev->state)
 		goto out_unlock;
@@ -733,7 +733,7 @@ void phy_stop(struct phy_device *phydev)
 	}
 
 out_unlock:
-	spin_unlock(&phydev->lock);
+	spin_unlock_bh(&phydev->lock);
 
 	/*
 	 * Cannot call flush_scheduled_work() here as desired because
@@ -781,7 +781,7 @@ static void phy_timer(unsigned long data
 	int needs_aneg = 0;
 	int err = 0;
 
-	spin_lock(&phydev->lock);
+	spin_lock_bh(&phydev->lock);
 
 	if (phydev->adjust_state)
 		phydev->adjust_state(phydev->attached_dev);
@@ -947,7 +947,7 @@ static void phy_timer(unsigned long data
 			break;
 	}
 
-	spin_unlock(&phydev->lock);
+	spin_unlock_bh(&phydev->lock);
 
 	if (needs_aneg)
 		err = phy_start_aneg(phydev);
diff -up --recursive --new-file linux-netdev-2.6.23-rc6-20070913.macro/drivers/net/phy/phy_device.c linux-netdev-2.6.23-rc6-20070913/drivers/net/phy/phy_device.c
--- linux-netdev-2.6.23-rc6-20070913.macro/drivers/net/phy/phy_device.c	2007-09-13 17:27:52.000000000 +0000
+++ linux-netdev-2.6.23-rc6-20070913/drivers/net/phy/phy_device.c	2007-09-16 17:27:48.000000000 +0000
@@ -670,9 +670,9 @@ static int phy_remove(struct device *dev
 
 	phydev = to_phy_device(dev);
 
-	spin_lock(&phydev->lock);
+	spin_lock_bh(&phydev->lock);
 	phydev->state = PHY_DOWN;
-	spin_unlock(&phydev->lock);
+	spin_unlock_bh(&phydev->lock);
 
 	if (phydev->drv->remove)
 		phydev->drv->remove(phydev);

Re: [PATCH] PHYLIB: Spinlock fixes for softirqs

[Jeff Garzik]

doesn't Andrew have a version of these patches that move the useful 
patch text into the area where it will get copied into the permanent 
kernel changelog?

also, your PHYLIB patches give no notion at all of proper patch order.

Re: [PATCH] PHYLIB: Spinlock fixes for softirqs

[Andrew Morton]

On Sat, 29 Sep 2007 01:31:52 -0400 Jeff Garzik <jgarzik@pobox.com> wrote:

> doesn't Andrew have a version of these patches that move the useful 
> patch text into the area where it will get copied into the permanent 
> kernel changelog?
> 
> also, your PHYLIB patches give no notion at all of proper patch order.
> 

Andrew has patches coming out of his ears.  umm....

Applying order:

phylib-spinlock-fixes-for-softirqs.patch
phylib-irq-event-workqueue-handling-fixes.patch
phylib-fix-an-interrupt-loop-potential-when-halting.patch

ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.23-rc8/2.6.23-rc8-mm2/broken-out/phylib-spinlock-fixes-for-softirqs.patch
ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.23-rc8/2.6.23-rc8-mm2/broken-out/phylib-irq-event-workqueue-handling-fixes.patch
ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.23-rc8/2.6.23-rc8-mm2/broken-out/phylib-fix-an-interrupt-loop-potential-when-halting.patch

Let me unload some netdev patches at you...