PHYLIB: Locking fixes for PHY I/O potentially sleeping

PHY read/write functions can potentially sleep (e.g., a PHY accessed
via I2C).  The following changes were made to account for this:

    * Change spin locks to mutex locks
    * Add a BUG_ON() to phy_read() phy_write() to warn against
      calling them from an interrupt context.
    * Use work queue for PHY state machine handling since
      it can potentially sleep
    * Change phydev lock from spinlock to mutex

Signed-off-by: Nate Case <ncase@xes-inc.com>
Acked-by: Andy Fleming <afleming@freescale.com>
Signed-off-by: Jeff Garzik <jeff@garzik.org>
Signed-off-by: David S. Miller <davem@davemloft.net>

4 files changed, 55 insertions, 31 deletions

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index c30196d0ad16..6e9f619c491f 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -49,7 +49,7 @@ int mdiobus_register(struct mii_bus *bus)
         int i;
         int err = 0;
 
-        spin_lock_init(&bus->mdio_lock);
+        mutex_init(&bus->mdio_lock);
 
         if (NULL == bus || NULL == bus->name ||
                         NULL == bus->read ||
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 7c9e6e349503..12fccb1c76dc 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -26,7 +26,6 @@
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
 #include <linux/skbuff.h>
-#include <linux/spinlock.h>
 #include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/mii.h>
@@ -72,9 +71,11 @@ int phy_read(struct phy_device *phydev, u16 regnum)
         int retval;
         struct mii_bus *bus = phydev->bus;
 
-        spin_lock_bh(&bus->mdio_lock);
+        BUG_ON(in_interrupt());
+
+        mutex_lock(&bus->mdio_lock);
         retval = bus->read(bus, phydev->addr, regnum);
-        spin_unlock_bh(&bus->mdio_lock);
+        mutex_unlock(&bus->mdio_lock);
 
         return retval;
 }
@@ -95,9 +96,11 @@ int phy_write(struct phy_device *phydev, u16 regnum, u16 val)
         int err;
         struct mii_bus *bus = phydev->bus;
 
-        spin_lock_bh(&bus->mdio_lock);
+        BUG_ON(in_interrupt());
+
+        mutex_lock(&bus->mdio_lock);
         err = bus->write(bus, phydev->addr, regnum, val);
-        spin_unlock_bh(&bus->mdio_lock);
+        mutex_unlock(&bus->mdio_lock);
 
         return err;
 }
@@ -428,7 +431,7 @@ int phy_start_aneg(struct phy_device *phydev)
 {
         int err;
 
-        spin_lock_bh(&phydev->lock);
+        mutex_lock(&phydev->lock);
 
         if (AUTONEG_DISABLE == phydev->autoneg)
                 phy_sanitize_settings(phydev);
@@ -449,13 +452,14 @@ int phy_start_aneg(struct phy_device *phydev)
         }
 
 out_unlock:
-        spin_unlock_bh(&phydev->lock);
+        mutex_unlock(&phydev->lock);
         return err;
 }
 EXPORT_SYMBOL(phy_start_aneg);
 
 
 static void phy_change(struct work_struct *work);
+static void phy_state_machine(struct work_struct *work);
 static void phy_timer(unsigned long data);
 
 /**
@@ -476,6 +480,7 @@ void phy_start_machine(struct phy_device *phydev,
 {
         phydev->adjust_state = handler;
 
+        INIT_WORK(&phydev->state_queue, phy_state_machine);
         init_timer(&phydev->phy_timer);
         phydev->phy_timer.function = &phy_timer;
         phydev->phy_timer.data = (unsigned long) phydev;
@@ -493,11 +498,12 @@ void phy_start_machine(struct phy_device *phydev,
 void phy_stop_machine(struct phy_device *phydev)
 {
         del_timer_sync(&phydev->phy_timer);
+        cancel_work_sync(&phydev->state_queue);
 
-        spin_lock_bh(&phydev->lock);
+        mutex_lock(&phydev->lock);
         if (phydev->state > PHY_UP)
                 phydev->state = PHY_UP;
-        spin_unlock_bh(&phydev->lock);
+        mutex_unlock(&phydev->lock);
 
         phydev->adjust_state = NULL;
 }
@@ -541,9 +547,9 @@ static void phy_force_reduction(struct phy_device *phydev)
  */
 void phy_error(struct phy_device *phydev)
 {
-        spin_lock_bh(&phydev->lock);
+        mutex_lock(&phydev->lock);
         phydev->state = PHY_HALTED;
-        spin_unlock_bh(&phydev->lock);
+        mutex_unlock(&phydev->lock);
 }
 
 /**
@@ -705,10 +711,10 @@ static void phy_change(struct work_struct *work)
         if (err)
                 goto phy_err;
 
-        spin_lock_bh(&phydev->lock);
+        mutex_lock(&phydev->lock);
         if ((PHY_RUNNING == phydev->state) || (PHY_NOLINK == phydev->state))
                 phydev->state = PHY_CHANGELINK;
-        spin_unlock_bh(&phydev->lock);
+        mutex_unlock(&phydev->lock);
 
         atomic_dec(&phydev->irq_disable);
         enable_irq(phydev->irq);
@@ -735,7 +741,7 @@ phy_err:
  */
 void phy_stop(struct phy_device *phydev)
 {
-        spin_lock_bh(&phydev->lock);
+        mutex_lock(&phydev->lock);
 
         if (PHY_HALTED == phydev->state)
                 goto out_unlock;
@@ -751,7 +757,7 @@ void phy_stop(struct phy_device *phydev)
         phydev->state = PHY_HALTED;
 
 out_unlock:
-        spin_unlock_bh(&phydev->lock);
+        mutex_unlock(&phydev->lock);
 
         /*
          * Cannot call flush_scheduled_work() here as desired because
@@ -773,7 +779,7 @@ out_unlock:
  */
 void phy_start(struct phy_device *phydev)
 {
-        spin_lock_bh(&phydev->lock);
+        mutex_lock(&phydev->lock);
 
         switch (phydev->state) {
                 case PHY_STARTING:
@@ -787,19 +793,26 @@ void phy_start(struct phy_device *phydev)
                 default:
                         break;
         }
-        spin_unlock_bh(&phydev->lock);
+        mutex_unlock(&phydev->lock);
 }
 EXPORT_SYMBOL(phy_stop);
 EXPORT_SYMBOL(phy_start);
 
-/* PHY timer which handles the state machine */
-static void phy_timer(unsigned long data)
+/**
+ * phy_state_machine - Handle the state machine
+ * @work: work_struct that describes the work to be done
+ *
+ * Description: Scheduled by the state_queue workqueue each time
+ *   phy_timer is triggered.
+ */
+static void phy_state_machine(struct work_struct *work)
 {
-        struct phy_device *phydev = (struct phy_device *)data;
+        struct phy_device *phydev =
+                        container_of(work, struct phy_device, state_queue);
         int needs_aneg = 0;
         int err = 0;
 
-        spin_lock_bh(&phydev->lock);
+        mutex_lock(&phydev->lock);
 
         if (phydev->adjust_state)
                 phydev->adjust_state(phydev->attached_dev);
@@ -965,7 +978,7 @@ static void phy_timer(unsigned long data)
                         break;
         }
 
-        spin_unlock_bh(&phydev->lock);
+        mutex_unlock(&phydev->lock);
 
         if (needs_aneg)
                 err = phy_start_aneg(phydev);
@@ -976,3 +989,14 @@ static void phy_timer(unsigned long data)
         mod_timer(&phydev->phy_timer, jiffies + PHY_STATE_TIME * HZ);
 }
 
+/* PHY timer which schedules the state machine work */
+static void phy_timer(unsigned long data)
+{
+        struct phy_device *phydev = (struct phy_device *)data;
+
+        /*
+         * PHY I/O operations can potentially sleep so we ensure that
+         * it's done from a process context
+         */
+        schedule_work(&phydev->state_queue);
+}
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 5b9e1751e1b4..f4c4fd85425f 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -25,7 +25,6 @@
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
 #include <linux/skbuff.h>
-#include <linux/spinlock.h>
 #include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/mii.h>
@@ -80,7 +79,7 @@ struct phy_device* phy_device_create(struct mii_bus *bus, int addr, int phy_id)
 
         dev->state = PHY_DOWN;
 
-        spin_lock_init(&dev->lock);
+        mutex_init(&dev->lock);
 
         return dev;
 }
@@ -656,7 +655,7 @@ static int phy_probe(struct device *dev)
         if (!(phydrv->flags & PHY_HAS_INTERRUPT))
                 phydev->irq = PHY_POLL;
 
-        spin_lock_bh(&phydev->lock);
+        mutex_lock(&phydev->lock);
 
         /* Start out supporting everything. Eventually,
          * a controller will attach, and may modify one
@@ -670,7 +669,7 @@ static int phy_probe(struct device *dev)
         if (phydev->drv->probe)
                 err = phydev->drv->probe(phydev);
 
-        spin_unlock_bh(&phydev->lock);
+        mutex_unlock(&phydev->lock);
 
         return err;
 
@@ -682,9 +681,9 @@ static int phy_remove(struct device *dev)
 
         phydev = to_phy_device(dev);
 
-        spin_lock_bh(&phydev->lock);
+        mutex_lock(&phydev->lock);
         phydev->state = PHY_DOWN;
-        spin_unlock_bh(&phydev->lock);
+        mutex_unlock(&phydev->lock);
 
         if (phydev->drv->remove)
                 phydev->drv->remove(phydev);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 554836edd915..5e43ae751412 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -88,7 +88,7 @@ struct mii_bus {
 
         /* A lock to ensure that only one thing can read/write
          * the MDIO bus at a time */
-        spinlock_t mdio_lock;
+        struct mutex mdio_lock;
 
         struct device *dev;
 
@@ -284,10 +284,11 @@ struct phy_device {
 
         /* Interrupt and Polling infrastructure */
         struct work_struct phy_queue;
+        struct work_struct state_queue;
         struct timer_list phy_timer;
         atomic_t irq_disable;
 
-        spinlock_t lock;
+        struct mutex lock;
 
         struct net_device *attached_dev;