Commit:     0ac49527318bc388a881152d60f49d7951606024
Parent:     f7ab697d328b0a417d9e3cb891d45693ea89e83d
Author:     Maciej W. Rozycki <[EMAIL PROTECTED]>
AuthorDate: Fri Sep 28 22:42:14 2007 -0700
Committer:  David S. Miller <[EMAIL PROTECTED]>
CommitDate: Wed Oct 10 16:53:55 2007 -0700

    PHYLIB: IRQ event workqueue handling fixes
    Keep track of disable_irq_nosync() invocations and call enable_irq() the
    right number of times if work has been cancelled that would include them.
    Now that the call to flush_work_keventd() (problematic because of
    rtnl_mutex being held) has been replaced by cancel_work_sync() another
    issue has arisen and been left unresolved.  As the MDIO bus cannot be
    accessed from the interrupt context the PHY interrupt handler uses
    disable_irq_nosync() to prevent from looping and schedules some work to be
    done as a softirq, which, apart from handling the state change of the
    originating PHY, is responsible for reenabling the interrupt.  Now if the
    interrupt line is shared by another device and a call to the softirq
    handler has been cancelled, that call to enable_irq() never happens and the
    other device cannot use its interrupt anymore as its stuck disabled.
    I decided to use a counter rather than a flag because there may be more
    than one call to phy_change() cancelled in the queue -- a real one and a
    fake one triggered by free_irq() if DEBUG_SHIRQ is used, if nothing else.
    Therefore because of its nesting property enable_irq() has to be called the
    right number of times to match the number disable_irq_nosync() was called
    and restore the original state.  This DEBUG_SHIRQ feature is also the
    reason why free_irq() has to be called before cancel_work_sync().
    While at it I updated the comment about phy_stop_interrupts() being called
    from `keventd' -- this is no longer relevant as the use of
    cancel_work_sync() makes such an approach unnecessary.  OTOH a similar
    comment referring to flush_scheduled_work() in phy_stop() still applies as
    using cancel_work_sync() there would be dangerous.
    Checked with and at the run time (with and without
    Signed-off-by: Maciej W. Rozycki <[EMAIL PROTECTED]>
    Cc: Andy Fleming <[EMAIL PROTECTED]>
    Cc: Jeff Garzik <[EMAIL PROTECTED]>
    Signed-off-by: Andrew Morton <[EMAIL PROTECTED]>
    Signed-off-by: Jeff Garzik <[EMAIL PROTECTED]>
 drivers/net/phy/phy.c |   24 +++++++++++++++++++-----
 include/linux/phy.h   |    3 +++
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 4da993d..5a314ed 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -7,7 +7,7 @@
  * Author: Andy Fleming
  * Copyright (c) 2004 Freescale Semiconductor, Inc.
- * Copyright (c) 2006  Maciej W. Rozycki
+ * Copyright (c) 2006, 2007  Maciej W. Rozycki
  * This program is free software; you can redistribute  it and/or modify it
  * under  the terms of  the GNU General  Public License as published by the
@@ -35,6 +35,7 @@
 #include <linux/timer.h>
 #include <linux/workqueue.h>
+#include <asm/atomic.h>
 #include <asm/io.h>
 #include <asm/irq.h>
 #include <asm/uaccess.h>
@@ -562,6 +563,7 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
         * queue will write the PHY to disable and clear the
         * interrupt, and then reenable the irq line. */
+       atomic_inc(&phydev->irq_disable);
@@ -632,6 +634,7 @@ int phy_start_interrupts(struct phy_device *phydev)
        INIT_WORK(&phydev->phy_queue, phy_change);
+       atomic_set(&phydev->irq_disable, 0);
        if (request_irq(phydev->irq, phy_interrupt,
@@ -662,13 +665,22 @@ int phy_stop_interrupts(struct phy_device *phydev)
        if (err)
+       free_irq(phydev->irq, phydev);
-        * Finish any pending work; we might have been scheduled to be called
-        * from keventd ourselves, but cancel_work_sync() handles that.
+        * Cannot call flush_scheduled_work() here as desired because
+        * of rtnl_lock(), but we do not really care about what would
+        * be done, except from enable_irq(), so cancel any work
+        * possibly pending and take care of the matter below.
-       free_irq(phydev->irq, phydev);
+       /*
+        * If work indeed has been cancelled, disable_irq() will have
+        * been left unbalanced from phy_interrupt() and enable_irq()
+        * has to be called so that other devices on the line work.
+        */
+       while (atomic_dec_return(&phydev->irq_disable) >= 0)
+               enable_irq(phydev->irq);
        return err;
@@ -695,6 +707,7 @@ static void phy_change(struct work_struct *work)
                phydev->state = PHY_CHANGELINK;
+       atomic_dec(&phydev->irq_disable);
        /* Reenable interrupts */
@@ -708,6 +721,7 @@ static void phy_change(struct work_struct *work)
+       atomic_inc(&phydev->irq_disable);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 2a65978..f0742b6 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -25,6 +25,8 @@
 #include <linux/timer.h>
 #include <linux/workqueue.h>
+#include <asm/atomic.h>
 #define PHY_BASIC_FEATURES     (SUPPORTED_10baseT_Half | \
                                 SUPPORTED_10baseT_Full | \
                                 SUPPORTED_100baseT_Half | \
@@ -281,6 +283,7 @@ struct phy_device {
        /* Interrupt and Polling infrastructure */
        struct work_struct phy_queue;
        struct timer_list phy_timer;
+       atomic_t irq_disable;
        spinlock_t lock;
To unsubscribe from this list: send the line "unsubscribe git-commits-head" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at

Reply via email to