pciehp: Fix interrupt event handlig

Current pciehp implementation disables and re-enables hotplug interrupts
in its interrupt handler. This operation might be intend to guarantee
that interrupts for the events newly occured during previous events are
being handled will be successfully generated. But current implementaion
has the following prolems.

  - Current interrupt service routin clears status changes without
    waiting command completion. Because of this, events might not be
    cleared properly.
  - Current interrupt service routine clears status changes caused by
    disabling or enabling hotplug interrupts itself. This will lose new
    events that occurs during previous interrupts are being handled.
  - Current implementation doesn't have any serialization mechanism
    between the code to wait for command completion and the interrupt
    handler that clears the command completion events caused by itself.
    There is clearly race conditions between them, and it may cause
    the problem that waiting for command completion doesn't work for
    example.

To fix those problems, this patch stops disabling/re-enabling hotplug
interrupts in interrupt service routine. Instead of this, this patch
re-inspects Slot Status register after clearing what is presumed to
be the last bending interrupt in order to guarantee that all interrupt
events are serviced.

Signed-off-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
Signed-off-by:  Kristen Carlson Accardi <kristen.c.accardi@intel.com>
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index f14267e197dd7677ca3d70bc9293f970fa35a3fe..9dd132925ffa33695cd1a5fb62542afc74ad01f1 100644 (file)
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -97,7 +97,6 @@ struct controller {
        u8 cap_base;
        struct timer_list poll_timer;
        volatile int cmd_busy;
-       spinlock_t lock;
 };
 
 #define INT_BUTTON_IGNORE              0

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index b4bbd07d1e3902ff756a2280c01480c2ffb73320..51a5055f69658f89d5204c4b0424165e9ffec39c 100644 (file)
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -252,7 +252,6 @@ static int pcie_write_cmd(struct slot *slot, u16 cmd, u16 mask)
        int retval = 0;
        u16 slot_status;
        u16 slot_ctrl;
-       unsigned long flags;
 
        mutex_lock(&ctrl->ctrl_lock);
 
@@ -270,11 +269,10 @@ static int pcie_write_cmd(struct slot *slot, u16 cmd, u16 mask)
                    __func__);
        }
 
-       spin_lock_irqsave(&ctrl->lock, flags);
        retval = pciehp_readw(ctrl, SLOTCTRL, &slot_ctrl);
        if (retval) {
                err("%s: Cannot read SLOTCTRL register\n", __func__);
-               goto out_spin_unlock;
+               goto out;
        }
 
        slot_ctrl &= ~mask;
@@ -285,9 +283,6 @@ static int pcie_write_cmd(struct slot *slot, u16 cmd, u16 mask)
        if (retval)
                err("%s: Cannot write to SLOTCTRL register\n", __func__);
 
- out_spin_unlock:
-       spin_unlock_irqrestore(&ctrl->lock, flags);
-
        /*
         * Wait for command completion.
         */
@@ -733,139 +728,55 @@ static int hpc_power_off_slot(struct slot * slot)
 static irqreturn_t pcie_isr(int irq, void *dev_id)
 {
        struct controller *ctrl = (struct controller *)dev_id;
-       u16 slot_status, intr_detect, intr_loc;
-       u16 temp_word;
-       int hp_slot = 0;        /* only 1 slot per PCI Express port */
-       int rc = 0;
-       unsigned long flags;
-
-       rc = pciehp_readw(ctrl, SLOTSTATUS, &slot_status);
-       if (rc) {
-               err("%s: Cannot read SLOTSTATUS register\n", __func__);
-               return IRQ_NONE;
-       }
+       u16 detected, intr_loc;
 
-       intr_detect = (ATTN_BUTTN_PRESSED | PWR_FAULT_DETECTED |
-                      MRL_SENS_CHANGED | PRSN_DETECT_CHANGED | CMD_COMPLETED);
-
-       intr_loc = slot_status & intr_detect;
-
-       /* Check to see if it was our interrupt */
-       if ( !intr_loc )
-               return IRQ_NONE;
-
-       dbg("%s: intr_loc %x\n", __func__, intr_loc);
-       /* Mask Hot-plug Interrupt Enable */
-       if (!pciehp_poll_mode) {
-               spin_lock_irqsave(&ctrl->lock, flags);
-               rc = pciehp_readw(ctrl, SLOTCTRL, &temp_word);
-               if (rc) {
-                       err("%s: Cannot read SLOT_CTRL register\n",
-                           __func__);
-                       spin_unlock_irqrestore(&ctrl->lock, flags);
+       /*
+        * In order to guarantee that all interrupt events are
+        * serviced, we need to re-inspect Slot Status register after
+        * clearing what is presumed to be the last pending interrupt.
+        */
+       intr_loc = 0;
+       do {
+               if (pciehp_readw(ctrl, SLOTSTATUS, &detected)) {
+                       err("%s: Cannot read SLOTSTATUS\n", __func__);
                        return IRQ_NONE;
                }
 
-               dbg("%s: pciehp_readw(SLOTCTRL) with value %x\n",
-                   __func__, temp_word);
-               temp_word = (temp_word & ~HP_INTR_ENABLE &
-                            ~CMD_CMPL_INTR_ENABLE) | 0x00;
-               rc = pciehp_writew(ctrl, SLOTCTRL, temp_word);
-               if (rc) {
-                       err("%s: Cannot write to SLOTCTRL register\n",
-                           __func__);
-                       spin_unlock_irqrestore(&ctrl->lock, flags);
+               detected &= (ATTN_BUTTN_PRESSED | PWR_FAULT_DETECTED |
+                            MRL_SENS_CHANGED | PRSN_DETECT_CHANGED |
+                            CMD_COMPLETED);
+               intr_loc |= detected;
+               if (!intr_loc)
                        return IRQ_NONE;
-               }
-               spin_unlock_irqrestore(&ctrl->lock, flags);
-
-               rc = pciehp_readw(ctrl, SLOTSTATUS, &slot_status);
-               if (rc) {
-                       err("%s: Cannot read SLOT_STATUS register\n",
-                           __func__);
+               if (pciehp_writew(ctrl, SLOTSTATUS, detected)) {
+                       err("%s: Cannot write to SLOTSTATUS\n", __func__);
                        return IRQ_NONE;
                }
-               dbg("%s: pciehp_readw(SLOTSTATUS) with value %x\n",
-                   __func__, slot_status);
+       } while (detected);
 
-               /* Clear command complete interrupt caused by this write */
-               temp_word = 0x1f;
-               rc = pciehp_writew(ctrl, SLOTSTATUS, temp_word);
-               if (rc) {
-                       err("%s: Cannot write to SLOTSTATUS register\n",
-                           __func__);
-                       return IRQ_NONE;
-               }
-       }
+       dbg("%s: intr_loc %x\n", __FUNCTION__, intr_loc);
 
+       /* Check Command Complete Interrupt Pending */
        if (intr_loc & CMD_COMPLETED) {
-               /*
-                * Command Complete Interrupt Pending
-                */
                ctrl->cmd_busy = 0;
                wake_up_interruptible(&ctrl->queue);
        }
 
+       /* Check MRL Sensor Changed */
        if (intr_loc & MRL_SENS_CHANGED)
-               pciehp_handle_switch_change(hp_slot, ctrl);
+               pciehp_handle_switch_change(0, ctrl);
 
+       /* Check Attention Button Pressed */
        if (intr_loc & ATTN_BUTTN_PRESSED)
-               pciehp_handle_attention_button(hp_slot, ctrl);
+               pciehp_handle_attention_button(0, ctrl);
 
+       /* Check Presence Detect Changed */
        if (intr_loc & PRSN_DETECT_CHANGED)
-               pciehp_handle_presence_change(hp_slot, ctrl);
+               pciehp_handle_presence_change(0, ctrl);
 
+       /* Check Power Fault Detected */
        if (intr_loc & PWR_FAULT_DETECTED)
-               pciehp_handle_power_fault(hp_slot, ctrl);
-
-       /* Clear all events after serving them */
-       temp_word = 0x1F;
-       rc = pciehp_writew(ctrl, SLOTSTATUS, temp_word);
-       if (rc) {
-               err("%s: Cannot write to SLOTSTATUS register\n", __func__);
-               return IRQ_NONE;
-       }
-       /* Unmask Hot-plug Interrupt Enable */
-       if (!pciehp_poll_mode) {
-               spin_lock_irqsave(&ctrl->lock, flags);
-               rc = pciehp_readw(ctrl, SLOTCTRL, &temp_word);
-               if (rc) {
-                       err("%s: Cannot read SLOTCTRL register\n",
-                           __func__);
-                       spin_unlock_irqrestore(&ctrl->lock, flags);
-                       return IRQ_NONE;
-               }
-
-               dbg("%s: Unmask Hot-plug Interrupt Enable\n", __func__);
-               temp_word = (temp_word & ~HP_INTR_ENABLE) | HP_INTR_ENABLE;
-
-               rc = pciehp_writew(ctrl, SLOTCTRL, temp_word);
-               if (rc) {
-                       err("%s: Cannot write to SLOTCTRL register\n",
-                           __func__);
-                       spin_unlock_irqrestore(&ctrl->lock, flags);
-                       return IRQ_NONE;
-               }
-               spin_unlock_irqrestore(&ctrl->lock, flags);
-
-               rc = pciehp_readw(ctrl, SLOTSTATUS, &slot_status);
-               if (rc) {
-                       err("%s: Cannot read SLOT_STATUS register\n",
-                           __func__);
-                       return IRQ_NONE;
-               }
-
-               /* Clear command complete interrupt caused by this write */
-               temp_word = 0x1F;
-               rc = pciehp_writew(ctrl, SLOTSTATUS, temp_word);
-               if (rc) {
-                       err("%s: Cannot write to SLOTSTATUS failed\n",
-                           __func__);
-                       return IRQ_NONE;
-               }
-               dbg("%s: pciehp_writew(SLOTSTATUS) with value %x\n",
-                   __func__, temp_word);
-       }
+               pciehp_handle_power_fault(0, ctrl);
 
        return IRQ_HANDLED;
 }
@@ -1334,7 +1245,6 @@ int pcie_init(struct controller *ctrl, struct pcie_device *dev)
 
        mutex_init(&ctrl->crit_sect);
        mutex_init(&ctrl->ctrl_lock);
-       spin_lock_init(&ctrl->lock);
 
        /* setup wait queue */
        init_waitqueue_head(&ctrl->queue);