[PATCH v2 0/8] PCI: pciehp: Rework hotplug interrupt routine

[PATCH v2 0/8] PCI: pciehp: Rework hotplug interrupt routine

[Bjorn Helgaas]

This is mostly Mayurkumar's work from [1] and [2].  I split [2] into two
patches and reworked it to keep the enclosing loop around the pciehp ISR.

The patches I added are trivial ones to clarify variable names, make dmesg
messages consistent, and remove useless code.

[1] 1471554479-42083-1-git-send-email-mayurkumar.patel@intel.com
[2] 92EBB4272BF81E4089A7126EC1E7B28466598F35@IRSMSX101.ger.corp.intel.com

---

Bjorn Helgaas (5):
      PCI: pciehp: Rename pcie_isr() locals for clarity
      PCI: pciehp: Return IRQ_NONE when we can't read interrupt status
      PCI: pciehp: Remove unnecessary guard
      PCI: pciehp: Clean up dmesg "Slot(%s)" messages
      PCI: pciehp: Remove useless pciehp_get_latch_status() calls

Mayurkumar Patel (3):
      PCI: pciehp: Process all hotplug events before looking for new ones
      PCI: pciehp: Don't re-read Slot Status when queuing hotplug event
      PCI: pciehp: Don't re-read Slot Status when handling surprise event


 drivers/pci/hotplug/pciehp_ctrl.c |   83 ++++++++++++---------------------
 drivers/pci/hotplug/pciehp_hpc.c  |   94 ++++++++++++++++++++-----------------
 2 files changed, 82 insertions(+), 95 deletions(-)

[PATCH v2 1/8] PCI: pciehp: Rename pcie_isr() locals for clarity

[Bjorn Helgaas]

Rename "detected" and "intr_loc" to "status" and "events" for clarity.
"status" is the value we read from the Slot Status register; "events" is
the set of hot-plug events we need to process.  No functional change
intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/hotplug/pciehp_hpc.c |   46 +++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 21 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 08e84d6..264df36 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -542,7 +542,7 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
 	struct pci_bus *subordinate = pdev->subordinate;
 	struct pci_dev *dev;
 	struct slot *slot = ctrl->slot;
-	u16 detected, intr_loc;
+	u16 status, events;
 	u8 present;
 	bool link;
 
@@ -555,31 +555,35 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
 	 * serviced, we need to re-inspect Slot Status register after
 	 * clearing what is presumed to be the last pending interrupt.
 	 */
-	intr_loc = 0;
+	events = 0;
 	do {
-		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &detected);
-		if (detected == (u16) ~0) {
+		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &status);
+		if (status == (u16) ~0) {
 			ctrl_info(ctrl, "%s: no response from device\n",
 				  __func__);
 			return IRQ_HANDLED;
 		}
 
-		detected &= (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
-			     PCI_EXP_SLTSTA_PDC |
-			     PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC);
-		detected &= ~intr_loc;
-		intr_loc |= detected;
-		if (!intr_loc)
+		/*
+		 * Slot Status contains plain status bits as well as event
+		 * notification bits; right now we only want the event bits.
+		 */
+		status &= (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
+			   PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_CC |
+			   PCI_EXP_SLTSTA_DLLSC);
+		status &= ~events;
+		events |= status;
+		if (!events)
 			return IRQ_NONE;
-		if (detected)
+		if (status)
 			pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
-						   intr_loc);
-	} while (detected);
+						   events);
+	} while (status);
 
-	ctrl_dbg(ctrl, "pending interrupts %#06x from Slot Status\n", intr_loc);
+	ctrl_dbg(ctrl, "pending interrupts %#06x from Slot Status\n", events);
 
 	/* Check Command Complete Interrupt Pending */
-	if (intr_loc & PCI_EXP_SLTSTA_CC) {
+	if (events & PCI_EXP_SLTSTA_CC) {
 		ctrl->cmd_busy = 0;
 		smp_mb();
 		wake_up(&ctrl->queue);
@@ -589,24 +593,24 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
 		list_for_each_entry(dev, &subordinate->devices, bus_list) {
 			if (dev->ignore_hotplug) {
 				ctrl_dbg(ctrl, "ignoring hotplug event %#06x (%s requested no hotplug)\n",
-					 intr_loc, pci_name(dev));
+					 events, pci_name(dev));
 				return IRQ_HANDLED;
 			}
 		}
 	}
 
-	if (!(intr_loc & ~PCI_EXP_SLTSTA_CC))
+	if (!(events & ~PCI_EXP_SLTSTA_CC))
 		return IRQ_HANDLED;
 
 	/* Check Attention Button Pressed */
-	if (intr_loc & PCI_EXP_SLTSTA_ABP) {
+	if (events & PCI_EXP_SLTSTA_ABP) {
 		ctrl_info(ctrl, "Button pressed on Slot(%s)\n",
 			  slot_name(slot));
 		pciehp_queue_interrupt_event(slot, INT_BUTTON_PRESS);
 	}
 
 	/* Check Presence Detect Changed */
-	if (intr_loc & PCI_EXP_SLTSTA_PDC) {
+	if (events & PCI_EXP_SLTSTA_PDC) {
 		pciehp_get_adapter_status(slot, &present);
 		ctrl_info(ctrl, "Card %spresent on Slot(%s)\n",
 			  present ? "" : "not ", slot_name(slot));
@@ -615,13 +619,13 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
 	}
 
 	/* Check Power Fault Detected */
-	if ((intr_loc & PCI_EXP_SLTSTA_PFD) && !ctrl->power_fault_detected) {
+	if ((events & PCI_EXP_SLTSTA_PFD) && !ctrl->power_fault_detected) {
 		ctrl->power_fault_detected = 1;
 		ctrl_err(ctrl, "Power fault on slot %s\n", slot_name(slot));
 		pciehp_queue_interrupt_event(slot, INT_POWER_FAULT);
 	}
 
-	if (intr_loc & PCI_EXP_SLTSTA_DLLSC) {
+	if (events & PCI_EXP_SLTSTA_DLLSC) {
 		link = pciehp_check_link_active(ctrl);
 		ctrl_info(ctrl, "slot(%s): Link %s event\n",
 			  slot_name(slot), link ? "Up" : "Down");

[PATCH v2 2/8] PCI: pciehp: Return IRQ_NONE when we can't read interrupt status

[Bjorn Helgaas]

After 1469d17dd341 ("PCI: pciehp: Handle invalid data when reading from
non-existent devices"), we returned IRQ_HANDLED when we failed to read
interrupt status from the bridge.  I think it's better to return IRQ_NONE,
as we do in other cases where there's no interrupt pending.  This will
facilitate refactoring the loop in pcie_isr(): we'll be able to call the
ISR in a loop as long as it returns IRQ_HANDLED.

Return IRQ_NONE if we couldn't read interrupt status.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/hotplug/pciehp_hpc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 264df36..b8efe1b 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -561,7 +561,7 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
 		if (status == (u16) ~0) {
 			ctrl_info(ctrl, "%s: no response from device\n",
 				  __func__);
-			return IRQ_HANDLED;
+			return IRQ_NONE;
 		}
 
 		/*

[PATCH v2 3/8] PCI: pciehp: Process all hotplug events before looking for new ones

[Bjorn Helgaas]

From: Mayurkumar Patel <mayurkumar.patel@intel.com>

Previously we accumulated hotplug events, then processed them, essentially
like this:

  events = 0
  do {
    status = read(Slot Status)
    status &= EVENT_MASK              # only look at events
    events |= status                  # accumulate events
    write(Slot Status, events)        # clear events
  } while (status)
  process events

The problem is that as soon as we clear events in Slot Status, the hardware
may send notifications for new events, and we lose information about the
first events.  For example, we might see two Presence Detect Changed
events, but lose the fact that the slot was temporarily empty:

  read  PCI_EXP_SLTSTA_PDC set, PCI_EXP_SLTSTA_PDS clear  # slot empty
  write PCI_EXP_SLTSTA_PDC                                # clear PDC event
  read  PCI_EXP_SLTSTA_PDC set, PCI_EXP_SLTSTA_PDS set    # slot occupied

The current code does not process a removal; it only processes the
insertion, which fails because we didn't remove the original device.

To avoid this problem, read Slot Status once and process all the events
before reading it again, like this:

  do {
    read events
    clear events
    process events
  } while (events)

[bhelgaas: changelog, add external loop around pciehp_isr()]
Signed-off-by: Mayurkumar Patel <mayurkumar.patel@intel.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/hotplug/pciehp_hpc.c |   58 +++++++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 26 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index b8efe1b..625fa6a 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -535,7 +535,7 @@ void pciehp_power_off_slot(struct slot *slot)
 		 PCI_EXP_SLTCTL_PWR_OFF);
 }
 
-static irqreturn_t pcie_isr(int irq, void *dev_id)
+static irqreturn_t pciehp_isr(int irq, void *dev_id)
 {
 	struct controller *ctrl = (struct controller *)dev_id;
 	struct pci_dev *pdev = ctrl_dev(ctrl);
@@ -550,36 +550,23 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
 	if (pdev->current_state == PCI_D3cold)
 		return IRQ_NONE;
 
+	pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &status);
+	if (status == (u16) ~0) {
+		ctrl_info(ctrl, "%s: no response from device\n", __func__);
+		return IRQ_NONE;
+	}
+
 	/*
-	 * 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.
+	 * Slot Status contains plain status bits as well as event
+	 * notification bits; right now we only want the event bits.
 	 */
-	events = 0;
-	do {
-		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &status);
-		if (status == (u16) ~0) {
-			ctrl_info(ctrl, "%s: no response from device\n",
-				  __func__);
-			return IRQ_NONE;
-		}
-
-		/*
-		 * Slot Status contains plain status bits as well as event
-		 * notification bits; right now we only want the event bits.
-		 */
-		status &= (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
+	events = status & (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
 			   PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_CC |
 			   PCI_EXP_SLTSTA_DLLSC);
-		status &= ~events;
-		events |= status;
-		if (!events)
-			return IRQ_NONE;
-		if (status)
-			pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
-						   events);
-	} while (status);
+	if (!events)
+		return IRQ_NONE;
 
+	pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, events);
 	ctrl_dbg(ctrl, "pending interrupts %#06x from Slot Status\n", events);
 
 	/* Check Command Complete Interrupt Pending */
@@ -636,6 +623,25 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t pcie_isr(int irq, void *dev_id)
+{
+	irqreturn_t rc, handled = IRQ_NONE;
+
+	/*
+	 * 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.
+	 */
+	do {
+		rc = pciehp_isr(irq, dev_id);
+		if (rc == IRQ_HANDLED)
+			handled = IRQ_HANDLED;
+	} while (rc == IRQ_HANDLED);
+
+	/* Return IRQ_HANDLED if we handled one or more events */
+	return handled;
+}
+
 void pcie_enable_notification(struct controller *ctrl)
 {
 	u16 cmd, mask;

[PATCH v2 4/8] PCI: pciehp: Don't re-read Slot Status when queuing hotplug event

[Bjorn Helgaas]

From: Mayurkumar Patel <mayurkumar.patel@intel.com>

Previously we read Slot Status to learn about hotplug events, then cleared
the events, then re-read Slot Status to find out what happened.  But Slot
Status might have changed before the second read.

Capture the Slot Status once before clearing the events.  Also capture the
Link Status if we had a link status change.

[bhelgaas: changelog, split to separate patch]
Signed-off-by: Mayurkumar Patel <mayurkumar.patel@intel.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/hotplug/pciehp_hpc.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 625fa6a..fe99b45 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -566,6 +566,10 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
 	if (!events)
 		return IRQ_NONE;
 
+	/* Capture link status before clearing interrupts */
+	if (events & PCI_EXP_SLTSTA_DLLSC)
+		link = pciehp_check_link_active(ctrl);
+
 	pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, events);
 	ctrl_dbg(ctrl, "pending interrupts %#06x from Slot Status\n", events);
 
@@ -598,7 +602,7 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
 
 	/* Check Presence Detect Changed */
 	if (events & PCI_EXP_SLTSTA_PDC) {
-		pciehp_get_adapter_status(slot, &present);
+		present = !!(status & PCI_EXP_SLTSTA_PDS);
 		ctrl_info(ctrl, "Card %spresent on Slot(%s)\n",
 			  present ? "" : "not ", slot_name(slot));
 		pciehp_queue_interrupt_event(slot, present ? INT_PRESENCE_ON :
@@ -613,7 +617,6 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
 	}
 
 	if (events & PCI_EXP_SLTSTA_DLLSC) {
-		link = pciehp_check_link_active(ctrl);
 		ctrl_info(ctrl, "slot(%s): Link %s event\n",
 			  slot_name(slot), link ? "Up" : "Down");
 		pciehp_queue_interrupt_event(slot, link ? INT_LINK_UP :

[PATCH v2 5/8] PCI: pciehp: Don't re-read Slot Status when handling surprise event

[Bjorn Helgaas]

From: Mayurkumar Patel <mayurkumar.patel@intel.com>

Previously we read Slot Status when handling a surprise event.  But Slot
Status might have changed since we identified the event, and the event_type
already tells us whether to enable or disable the slot, so there's no need
to read it again.

Remove handle_surprise_event() and queue the power work directly.

[bhelgaas: changelog]
Signed-off-by: Mayurkumar Patel <mayurkumar.patel@intel.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Acked-by: Rajat Jain <rajatxjain@gmail.com>
---
 drivers/pci/hotplug/pciehp_ctrl.c |   18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 7ea3e61..a787684 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -302,20 +302,6 @@ static void handle_button_press_event(struct slot *p_slot)
 /*
  * Note: This function must be called with slot->lock held
  */
-static void handle_surprise_event(struct slot *p_slot)
-{
-	u8 getstatus;
-
-	pciehp_get_adapter_status(p_slot, &getstatus);
-	if (!getstatus)
-		pciehp_queue_power_work(p_slot, DISABLE_REQ);
-	else
-		pciehp_queue_power_work(p_slot, ENABLE_REQ);
-}
-
-/*
- * Note: This function must be called with slot->lock held
- */
 static void handle_link_event(struct slot *p_slot, u32 event)
 {
 	struct controller *ctrl = p_slot->ctrl;
@@ -378,14 +364,14 @@ static void interrupt_event_handler(struct work_struct *work)
 		pciehp_green_led_off(p_slot);
 		break;
 	case INT_PRESENCE_ON:
-		handle_surprise_event(p_slot);
+		pciehp_queue_power_work(p_slot, ENABLE_REQ);
 		break;
 	case INT_PRESENCE_OFF:
 		/*
 		 * Regardless of surprise capability, we need to
 		 * definitely remove a card that has been pulled out!
 		 */
-		handle_surprise_event(p_slot);
+		pciehp_queue_power_work(p_slot, DISABLE_REQ);
 		break;
 	case INT_LINK_UP:
 	case INT_LINK_DOWN:

[PATCH v2 6/8] PCI: pciehp: Remove unnecessary guard

[Bjorn Helgaas]

In pcie_isr(), we return early if no status bits other than
PCI_EXP_SLTSTA_CC are set.  This was introduced by dbd79aed1aea ("pciehp:
fix NULL dereference in interrupt handler"), but it is no longer necessary
because all the subsequent pcie_isr() code is already predicated on a
status bit being set.

Remove the unnecessary test for ~PCI_EXP_SLTSTA_CC.  No functional change
intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/hotplug/pciehp_hpc.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index fe99b45..60e1d55 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -590,9 +590,6 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
 		}
 	}
 
-	if (!(events & ~PCI_EXP_SLTSTA_CC))
-		return IRQ_HANDLED;
-
 	/* Check Attention Button Pressed */
 	if (events & PCI_EXP_SLTSTA_ABP) {
 		ctrl_info(ctrl, "Button pressed on Slot(%s)\n",

[PATCH v2 7/8] PCI: pciehp: Clean up dmesg "Slot(%s)" messages

[Bjorn Helgaas]

Print slot name consistently as "Slot(%s)".  I don't know whether that's
ideal, but we can at least do it the same way all the time.  No functional
change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/hotplug/pciehp_ctrl.c |   56 ++++++++++++++++++-------------------
 drivers/pci/hotplug/pciehp_hpc.c  |   12 ++++----
 2 files changed, 33 insertions(+), 35 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index a787684..bf50f26 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -106,7 +106,7 @@ static int board_added(struct slot *p_slot)
 
 	/* Check for a power fault */
 	if (ctrl->power_fault_detected || pciehp_query_power_fault(p_slot)) {
-		ctrl_err(ctrl, "Power fault on slot %s\n", slot_name(p_slot));
+		ctrl_err(ctrl, "Slot(%s): Power fault\n", slot_name(p_slot));
 		retval = -EIO;
 		goto err_exit;
 	}
@@ -254,11 +254,11 @@ static void handle_button_press_event(struct slot *p_slot)
 		pciehp_get_power_status(p_slot, &getstatus);
 		if (getstatus) {
 			p_slot->state = BLINKINGOFF_STATE;
-			ctrl_info(ctrl, "PCI slot #%s - powering off due to button press\n",
+			ctrl_info(ctrl, "Slot(%s): Powering off due to button press\n",
 				  slot_name(p_slot));
 		} else {
 			p_slot->state = BLINKINGON_STATE;
-			ctrl_info(ctrl, "PCI slot #%s - powering on due to button press\n",
+			ctrl_info(ctrl, "Slot(%s) Powering on due to button press\n",
 				  slot_name(p_slot));
 		}
 		/* blink green LED and turn off amber */
@@ -273,14 +273,14 @@ static void handle_button_press_event(struct slot *p_slot)
 		 * press the attention again before the 5 sec. limit
 		 * expires to cancel hot-add or hot-remove
 		 */
-		ctrl_info(ctrl, "Button cancel on Slot(%s)\n", slot_name(p_slot));
+		ctrl_info(ctrl, "Slot(%s): Button cancel\n", slot_name(p_slot));
 		cancel_delayed_work(&p_slot->work);
 		if (p_slot->state == BLINKINGOFF_STATE)
 			pciehp_green_led_on(p_slot);
 		else
 			pciehp_green_led_off(p_slot);
 		pciehp_set_attention_status(p_slot, 0);
-		ctrl_info(ctrl, "PCI slot #%s - action canceled due to button press\n",
+		ctrl_info(ctrl, "Slot(%s): Action canceled due to button press\n",
 			  slot_name(p_slot));
 		p_slot->state = STATIC_STATE;
 		break;
@@ -291,10 +291,12 @@ static void handle_button_press_event(struct slot *p_slot)
 		 * this means that the previous attention button action
 		 * to hot-add or hot-remove is undergoing
 		 */
-		ctrl_info(ctrl, "Button ignore on Slot(%s)\n", slot_name(p_slot));
+		ctrl_info(ctrl, "Slot(%s): Button ignored\n",
+			  slot_name(p_slot));
 		break;
 	default:
-		ctrl_warn(ctrl, "ignoring invalid state %#x\n", p_slot->state);
+		ctrl_err(ctrl, "Slot(%s): Ignoring invalid state %#x\n",
+			 slot_name(p_slot), p_slot->state);
 		break;
 	}
 }
@@ -317,31 +319,27 @@ static void handle_link_event(struct slot *p_slot, u32 event)
 		break;
 	case POWERON_STATE:
 		if (event == INT_LINK_UP) {
-			ctrl_info(ctrl,
-				  "Link Up event ignored on slot(%s): already powering on\n",
+			ctrl_info(ctrl, "Slot(%s): Link Up event ignored; already powering on\n",
 				  slot_name(p_slot));
 		} else {
-			ctrl_info(ctrl,
-				  "Link Down event queued on slot(%s): currently getting powered on\n",
+			ctrl_info(ctrl, "Slot(%s): Link Down event queued; currently getting powered on\n",
 				  slot_name(p_slot));
 			pciehp_queue_power_work(p_slot, DISABLE_REQ);
 		}
 		break;
 	case POWEROFF_STATE:
 		if (event == INT_LINK_UP) {
-			ctrl_info(ctrl,
-				  "Link Up event queued on slot(%s): currently getting powered off\n",
+			ctrl_info(ctrl, "Slot(%s): Link Up event queued; currently getting powered off\n",
 				  slot_name(p_slot));
 			pciehp_queue_power_work(p_slot, ENABLE_REQ);
 		} else {
-			ctrl_info(ctrl,
-				  "Link Down event ignored on slot(%s): already powering off\n",
+			ctrl_info(ctrl, "Slot(%s): Link Down event ignored; already powering off\n",
 				  slot_name(p_slot));
 		}
 		break;
 	default:
-		ctrl_err(ctrl, "ignoring invalid state %#x on slot(%s)\n",
-			 p_slot->state, slot_name(p_slot));
+		ctrl_err(ctrl, "Slot(%s): Ignoring invalid state %#x\n",
+			 slot_name(p_slot), p_slot->state);
 		break;
 	}
 }
@@ -396,13 +394,13 @@ int pciehp_enable_slot(struct slot *p_slot)
 
 	pciehp_get_adapter_status(p_slot, &getstatus);
 	if (!getstatus) {
-		ctrl_info(ctrl, "No adapter on slot(%s)\n", slot_name(p_slot));
+		ctrl_info(ctrl, "Slot(%s): No adapter\n", slot_name(p_slot));
 		return -ENODEV;
 	}
 	if (MRL_SENS(p_slot->ctrl)) {
 		pciehp_get_latch_status(p_slot, &getstatus);
 		if (getstatus) {
-			ctrl_info(ctrl, "Latch open on slot(%s)\n",
+			ctrl_info(ctrl, "Slot(%s): Latch open\n",
 				  slot_name(p_slot));
 			return -ENODEV;
 		}
@@ -411,7 +409,7 @@ int pciehp_enable_slot(struct slot *p_slot)
 	if (POWER_CTRL(p_slot->ctrl)) {
 		pciehp_get_power_status(p_slot, &getstatus);
 		if (getstatus) {
-			ctrl_info(ctrl, "Already enabled on slot(%s)\n",
+			ctrl_info(ctrl, "Slot(%s): Already enabled\n",
 				  slot_name(p_slot));
 			return -EINVAL;
 		}
@@ -440,7 +438,7 @@ int pciehp_disable_slot(struct slot *p_slot)
 	if (POWER_CTRL(p_slot->ctrl)) {
 		pciehp_get_power_status(p_slot, &getstatus);
 		if (!getstatus) {
-			ctrl_info(ctrl, "Already disabled on slot(%s)\n",
+			ctrl_info(ctrl, "Slot(%s): Already disabled\n",
 				  slot_name(p_slot));
 			return -EINVAL;
 		}
@@ -468,17 +466,17 @@ int pciehp_sysfs_enable_slot(struct slot *p_slot)
 		p_slot->state = STATIC_STATE;
 		break;
 	case POWERON_STATE:
-		ctrl_info(ctrl, "Slot %s is already in powering on state\n",
+		ctrl_info(ctrl, "Slot(%s): Already in powering on state\n",
 			  slot_name(p_slot));
 		break;
 	case BLINKINGOFF_STATE:
 	case POWEROFF_STATE:
-		ctrl_info(ctrl, "Already enabled on slot %s\n",
+		ctrl_info(ctrl, "Slot(%s): Already enabled\n",
 			  slot_name(p_slot));
 		break;
 	default:
-		ctrl_err(ctrl, "invalid state %#x on slot %s\n",
-			 p_slot->state, slot_name(p_slot));
+		ctrl_err(ctrl, "Slot(%s): Invalid state %#x\n",
+			 slot_name(p_slot), p_slot->state);
 		break;
 	}
 	mutex_unlock(&p_slot->lock);
@@ -505,17 +503,17 @@ int pciehp_sysfs_disable_slot(struct slot *p_slot)
 		p_slot->state = STATIC_STATE;
 		break;
 	case POWEROFF_STATE:
-		ctrl_info(ctrl, "Slot %s is already in powering off state\n",
+		ctrl_info(ctrl, "Slot(%s): Already in powering off state\n",
 			  slot_name(p_slot));
 		break;
 	case BLINKINGON_STATE:
 	case POWERON_STATE:
-		ctrl_info(ctrl, "Already disabled on slot %s\n",
+		ctrl_info(ctrl, "Slot(%s): Already disabled\n",
 			  slot_name(p_slot));
 		break;
 	default:
-		ctrl_err(ctrl, "invalid state %#x on slot %s\n",
-			 p_slot->state, slot_name(p_slot));
+		ctrl_err(ctrl, "Slot(%s): Invalid state %#x\n",
+			 slot_name(p_slot), p_slot->state);
 		break;
 	}
 	mutex_unlock(&p_slot->lock);
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 60e1d55..4582fdf 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -592,7 +592,7 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
 
 	/* Check Attention Button Pressed */
 	if (events & PCI_EXP_SLTSTA_ABP) {
-		ctrl_info(ctrl, "Button pressed on Slot(%s)\n",
+		ctrl_info(ctrl, "Slot(%s): Attention button pressed\n",
 			  slot_name(slot));
 		pciehp_queue_interrupt_event(slot, INT_BUTTON_PRESS);
 	}
@@ -600,8 +600,8 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
 	/* Check Presence Detect Changed */
 	if (events & PCI_EXP_SLTSTA_PDC) {
 		present = !!(status & PCI_EXP_SLTSTA_PDS);
-		ctrl_info(ctrl, "Card %spresent on Slot(%s)\n",
-			  present ? "" : "not ", slot_name(slot));
+		ctrl_info(ctrl, "Slot(%s): Card %spresent\n", slot_name(slot),
+			  present ? "" : "not ");
 		pciehp_queue_interrupt_event(slot, present ? INT_PRESENCE_ON :
 					     INT_PRESENCE_OFF);
 	}
@@ -609,13 +609,13 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
 	/* Check Power Fault Detected */
 	if ((events & PCI_EXP_SLTSTA_PFD) && !ctrl->power_fault_detected) {
 		ctrl->power_fault_detected = 1;
-		ctrl_err(ctrl, "Power fault on slot %s\n", slot_name(slot));
+		ctrl_err(ctrl, "Slot(%s): Power fault\n", slot_name(slot));
 		pciehp_queue_interrupt_event(slot, INT_POWER_FAULT);
 	}
 
 	if (events & PCI_EXP_SLTSTA_DLLSC) {
-		ctrl_info(ctrl, "slot(%s): Link %s event\n",
-			  slot_name(slot), link ? "Up" : "Down");
+		ctrl_info(ctrl, "Slot(%s): Link %s\n", slot_name(slot),
+			  link ? "Up" : "Down");
 		pciehp_queue_interrupt_event(slot, link ? INT_LINK_UP :
 					     INT_LINK_DOWN);
 	}

[PATCH v2 8/8] PCI: pciehp: Remove useless pciehp_get_latch_status() calls

[Bjorn Helgaas]

Long ago, we updated a "switch_save" field based on the latch status.  But
switch_save was unused, and ed6cbcf2ac70 ("[PATCH] pciehp: miscellaneous
cleanups") removed it.

We no longer use the latch status, so remove calls to
pciehp_get_latch_status().  No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/hotplug/pciehp_ctrl.c |    9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index bf50f26..efe69e8 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -389,7 +389,6 @@ static void interrupt_event_handler(struct work_struct *work)
 int pciehp_enable_slot(struct slot *p_slot)
 {
 	u8 getstatus = 0;
-	int rc;
 	struct controller *ctrl = p_slot->ctrl;
 
 	pciehp_get_adapter_status(p_slot, &getstatus);
@@ -415,13 +414,7 @@ int pciehp_enable_slot(struct slot *p_slot)
 		}
 	}
 
-	pciehp_get_latch_status(p_slot, &getstatus);
-
-	rc = board_added(p_slot);
-	if (rc)
-		pciehp_get_latch_status(p_slot, &getstatus);
-
-	return rc;
+	return board_added(p_slot);
 }
 
 /*

RE: [PATCH v2 1/8] PCI: pciehp: Rename pcie_isr() locals for clarity

[Patel, Mayurkumar]

 
> Rename "detected" and "intr_loc" to "status" and "events" for clarity.
> "status" is the value we read from the Slot Status register; "events" is
> the set of hot-plug events we need to process.  No functional change
> intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/hotplug/pciehp_hpc.c |   46 +++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 08e84d6..264df36 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -542,7 +542,7 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
>  	struct pci_bus *subordinate = pdev->subordinate;
>  	struct pci_dev *dev;
>  	struct slot *slot = ctrl->slot;
> -	u16 detected, intr_loc;
> +	u16 status, events;
>  	u8 present;
>  	bool link;
> 
> @@ -555,31 +555,35 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
>  	 * serviced, we need to re-inspect Slot Status register after
>  	 * clearing what is presumed to be the last pending interrupt.
>  	 */
> -	intr_loc = 0;
> +	events = 0;
>  	do {
> -		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &detected);
> -		if (detected == (u16) ~0) {
> +		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &status);
> +		if (status == (u16) ~0) {
>  			ctrl_info(ctrl, "%s: no response from device\n",
>  				  __func__);
>  			return IRQ_HANDLED;
>  		}
> 
> -		detected &= (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
> -			     PCI_EXP_SLTSTA_PDC |
> -			     PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC);
> -		detected &= ~intr_loc;
> -		intr_loc |= detected;
> -		if (!intr_loc)
> +		/*
> +		 * Slot Status contains plain status bits as well as event
> +		 * notification bits; right now we only want the event bits.
> +		 */
> +		status &= (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
> +			   PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_CC |
> +			   PCI_EXP_SLTSTA_DLLSC);
> +		status &= ~events;
> +		events |= status;
> +		if (!events)
>  			return IRQ_NONE;
> -		if (detected)
> +		if (status)
>  			pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
> -						   intr_loc);
> -	} while (detected);
> +						   events);
> +	} while (status);
> 
> -	ctrl_dbg(ctrl, "pending interrupts %#06x from Slot Status\n", intr_loc);
> +	ctrl_dbg(ctrl, "pending interrupts %#06x from Slot Status\n", events);
> 
>  	/* Check Command Complete Interrupt Pending */
> -	if (intr_loc & PCI_EXP_SLTSTA_CC) {
> +	if (events & PCI_EXP_SLTSTA_CC) {
>  		ctrl->cmd_busy = 0;
>  		smp_mb();
>  		wake_up(&ctrl->queue);
> @@ -589,24 +593,24 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
>  		list_for_each_entry(dev, &subordinate->devices, bus_list) {
>  			if (dev->ignore_hotplug) {
>  				ctrl_dbg(ctrl, "ignoring hotplug event %#06x (%s requested no hotplug)\n",
> -					 intr_loc, pci_name(dev));
> +					 events, pci_name(dev));
>  				return IRQ_HANDLED;

Does it make sense here also to return IRQ_NONE? 

>  			}
>  		}
>  	}
> 
> -	if (!(intr_loc & ~PCI_EXP_SLTSTA_CC))
> +	if (!(events & ~PCI_EXP_SLTSTA_CC))
>  		return IRQ_HANDLED;
> 
>  	/* Check Attention Button Pressed */
> -	if (intr_loc & PCI_EXP_SLTSTA_ABP) {
> +	if (events & PCI_EXP_SLTSTA_ABP) {
>  		ctrl_info(ctrl, "Button pressed on Slot(%s)\n",
>  			  slot_name(slot));
>  		pciehp_queue_interrupt_event(slot, INT_BUTTON_PRESS);
>  	}
> 
>  	/* Check Presence Detect Changed */
> -	if (intr_loc & PCI_EXP_SLTSTA_PDC) {
> +	if (events & PCI_EXP_SLTSTA_PDC) {
>  		pciehp_get_adapter_status(slot, &present);
>  		ctrl_info(ctrl, "Card %spresent on Slot(%s)\n",
>  			  present ? "" : "not ", slot_name(slot));
> @@ -615,13 +619,13 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
>  	}
> 
>  	/* Check Power Fault Detected */
> -	if ((intr_loc & PCI_EXP_SLTSTA_PFD) && !ctrl->power_fault_detected) {
> +	if ((events & PCI_EXP_SLTSTA_PFD) && !ctrl->power_fault_detected) {
>  		ctrl->power_fault_detected = 1;
>  		ctrl_err(ctrl, "Power fault on slot %s\n", slot_name(slot));
>  		pciehp_queue_interrupt_event(slot, INT_POWER_FAULT);
>  	}
> 
> -	if (intr_loc & PCI_EXP_SLTSTA_DLLSC) {
> +	if (events & PCI_EXP_SLTSTA_DLLSC) {
>  		link = pciehp_check_link_active(ctrl);
>  		ctrl_info(ctrl, "slot(%s): Link %s event\n",
>  			  slot_name(slot), link ? "Up" : "Down");

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH v2 1/8] PCI: pciehp: Rename pcie_isr() locals for clarity

[Bjorn Helgaas]

On Tue, Sep 13, 2016 at 10:05:31AM +0000, Patel, Mayurkumar wrote:
>  
> > Rename "detected" and "intr_loc" to "status" and "events" for clarity.
> > "status" is the value we read from the Slot Status register; "events" is
> > the set of hot-plug events we need to process.  No functional change
> > intended.
> > 
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  drivers/pci/hotplug/pciehp_hpc.c |   46 +++++++++++++++++++++-----------------
> >  1 file changed, 25 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> > index 08e84d6..264df36 100644
> > --- a/drivers/pci/hotplug/pciehp_hpc.c
> > +++ b/drivers/pci/hotplug/pciehp_hpc.c
> > @@ -542,7 +542,7 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
> >  	struct pci_bus *subordinate = pdev->subordinate;
> >  	struct pci_dev *dev;
> >  	struct slot *slot = ctrl->slot;
> > -	u16 detected, intr_loc;
> > +	u16 status, events;
> >  	u8 present;
> >  	bool link;
> > 
> > @@ -555,31 +555,35 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
> >  	 * serviced, we need to re-inspect Slot Status register after
> >  	 * clearing what is presumed to be the last pending interrupt.
> >  	 */
> > -	intr_loc = 0;
> > +	events = 0;
> >  	do {
> > -		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &detected);
> > -		if (detected == (u16) ~0) {
> > +		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &status);
> > +		if (status == (u16) ~0) {
> >  			ctrl_info(ctrl, "%s: no response from device\n",
> >  				  __func__);
> >  			return IRQ_HANDLED;
> >  		}
> > 
> > -		detected &= (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
> > -			     PCI_EXP_SLTSTA_PDC |
> > -			     PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC);
> > -		detected &= ~intr_loc;
> > -		intr_loc |= detected;
> > -		if (!intr_loc)
> > +		/*
> > +		 * Slot Status contains plain status bits as well as event
> > +		 * notification bits; right now we only want the event bits.
> > +		 */
> > +		status &= (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
> > +			   PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_CC |
> > +			   PCI_EXP_SLTSTA_DLLSC);
> > +		status &= ~events;
> > +		events |= status;
> > +		if (!events)
> >  			return IRQ_NONE;
> > -		if (detected)
> > +		if (status)
> >  			pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
> > -						   intr_loc);
> > -	} while (detected);
> > +						   events);
> > +	} while (status);
> > 
> > -	ctrl_dbg(ctrl, "pending interrupts %#06x from Slot Status\n", intr_loc);
> > +	ctrl_dbg(ctrl, "pending interrupts %#06x from Slot Status\n", events);
> > 
> >  	/* Check Command Complete Interrupt Pending */
> > -	if (intr_loc & PCI_EXP_SLTSTA_CC) {
> > +	if (events & PCI_EXP_SLTSTA_CC) {
> >  		ctrl->cmd_busy = 0;
> >  		smp_mb();
> >  		wake_up(&ctrl->queue);
> > @@ -589,24 +593,24 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
> >  		list_for_each_entry(dev, &subordinate->devices, bus_list) {
> >  			if (dev->ignore_hotplug) {
> >  				ctrl_dbg(ctrl, "ignoring hotplug event %#06x (%s requested no hotplug)\n",
> > -					 intr_loc, pci_name(dev));
> > +					 events, pci_name(dev));
> >  				return IRQ_HANDLED;
> 
> Does it make sense here also to return IRQ_NONE? 

I don't think so.  Here's my reasoning; see if it makes sense:
IRQ_NONE means "I don't see any indication that my device needs
service."  If every handler for the IRQ returns IRQ_NONE, the
interrupt was spurious, and if we see enough spurious interrupts,
we'll disable that IRQ completely.  In this case, our device
definitely *did* request service; it's just that a driver wants us to
ignore hotplug events.  From the point of view of the kernel, we did
handle the IRQ (by ignoring it and clearing the interrupt request).

> >  			}
> >  		}
> >  	}
> > 
> > -	if (!(intr_loc & ~PCI_EXP_SLTSTA_CC))
> > +	if (!(events & ~PCI_EXP_SLTSTA_CC))
> >  		return IRQ_HANDLED;
> > 
> >  	/* Check Attention Button Pressed */
> > -	if (intr_loc & PCI_EXP_SLTSTA_ABP) {
> > +	if (events & PCI_EXP_SLTSTA_ABP) {
> >  		ctrl_info(ctrl, "Button pressed on Slot(%s)\n",
> >  			  slot_name(slot));
> >  		pciehp_queue_interrupt_event(slot, INT_BUTTON_PRESS);
> >  	}
> > 
> >  	/* Check Presence Detect Changed */
> > -	if (intr_loc & PCI_EXP_SLTSTA_PDC) {
> > +	if (events & PCI_EXP_SLTSTA_PDC) {
> >  		pciehp_get_adapter_status(slot, &present);
> >  		ctrl_info(ctrl, "Card %spresent on Slot(%s)\n",
> >  			  present ? "" : "not ", slot_name(slot));
> > @@ -615,13 +619,13 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
> >  	}
> > 
> >  	/* Check Power Fault Detected */
> > -	if ((intr_loc & PCI_EXP_SLTSTA_PFD) && !ctrl->power_fault_detected) {
> > +	if ((events & PCI_EXP_SLTSTA_PFD) && !ctrl->power_fault_detected) {
> >  		ctrl->power_fault_detected = 1;
> >  		ctrl_err(ctrl, "Power fault on slot %s\n", slot_name(slot));
> >  		pciehp_queue_interrupt_event(slot, INT_POWER_FAULT);
> >  	}
> > 
> > -	if (intr_loc & PCI_EXP_SLTSTA_DLLSC) {
> > +	if (events & PCI_EXP_SLTSTA_DLLSC) {
> >  		link = pciehp_check_link_active(ctrl);
> >  		ctrl_info(ctrl, "slot(%s): Link %s event\n",
> >  			  slot_name(slot), link ? "Up" : "Down");
> 
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de
> Managing Directors: Christin Eisenschmid, Christian Lamprechter
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH v2 1/8] PCI: pciehp: Rename pcie_isr() locals for clarity

[Patel, Mayurkumar]

> 
> On Tue, Sep 13, 2016 at 10:05:31AM +0000, Patel, Mayurkumar wrote:
> >
> > > Rename "detected" and "intr_loc" to "status" and "events" for clarity.
> > > "status" is the value we read from the Slot Status register; "events" is
> > > the set of hot-plug events we need to process.  No functional change
> > > intended.
> > >
> > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > ---
> > >  drivers/pci/hotplug/pciehp_hpc.c |   46 +++++++++++++++++++++-----------------
> > >  1 file changed, 25 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> > > index 08e84d6..264df36 100644
> > > --- a/drivers/pci/hotplug/pciehp_hpc.c
> > > +++ b/drivers/pci/hotplug/pciehp_hpc.c
> > > @@ -542,7 +542,7 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
> > >  	struct pci_bus *subordinate = pdev->subordinate;
> > >  	struct pci_dev *dev;
> > >  	struct slot *slot = ctrl->slot;
> > > -	u16 detected, intr_loc;
> > > +	u16 status, events;
> > >  	u8 present;
> > >  	bool link;
> > >
> > > @@ -555,31 +555,35 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
> > >  	 * serviced, we need to re-inspect Slot Status register after
> > >  	 * clearing what is presumed to be the last pending interrupt.
> > >  	 */
> > > -	intr_loc = 0;
> > > +	events = 0;
> > >  	do {
> > > -		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &detected);
> > > -		if (detected == (u16) ~0) {
> > > +		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &status);
> > > +		if (status == (u16) ~0) {
> > >  			ctrl_info(ctrl, "%s: no response from device\n",
> > >  				  __func__);
> > >  			return IRQ_HANDLED;
> > >  		}
> > >
> > > -		detected &= (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
> > > -			     PCI_EXP_SLTSTA_PDC |
> > > -			     PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC);
> > > -		detected &= ~intr_loc;
> > > -		intr_loc |= detected;
> > > -		if (!intr_loc)
> > > +		/*
> > > +		 * Slot Status contains plain status bits as well as event
> > > +		 * notification bits; right now we only want the event bits.
> > > +		 */
> > > +		status &= (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
> > > +			   PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_CC |
> > > +			   PCI_EXP_SLTSTA_DLLSC);
> > > +		status &= ~events;
> > > +		events |= status;
> > > +		if (!events)
> > >  			return IRQ_NONE;
> > > -		if (detected)
> > > +		if (status)
> > >  			pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
> > > -						   intr_loc);
> > > -	} while (detected);
> > > +						   events);
> > > +	} while (status);
> > >
> > > -	ctrl_dbg(ctrl, "pending interrupts %#06x from Slot Status\n", intr_loc);
> > > +	ctrl_dbg(ctrl, "pending interrupts %#06x from Slot Status\n", events);
> > >
> > >  	/* Check Command Complete Interrupt Pending */
> > > -	if (intr_loc & PCI_EXP_SLTSTA_CC) {
> > > +	if (events & PCI_EXP_SLTSTA_CC) {
> > >  		ctrl->cmd_busy = 0;
> > >  		smp_mb();
> > >  		wake_up(&ctrl->queue);
> > > @@ -589,24 +593,24 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
> > >  		list_for_each_entry(dev, &subordinate->devices, bus_list) {
> > >  			if (dev->ignore_hotplug) {
> > >  				ctrl_dbg(ctrl, "ignoring hotplug event %#06x (%s requested no hotplug)\n",
> > > -					 intr_loc, pci_name(dev));
> > > +					 events, pci_name(dev));
> > >  				return IRQ_HANDLED;
> >
> > Does it make sense here also to return IRQ_NONE?
> 
> I don't think so.  Here's my reasoning; see if it makes sense:
> IRQ_NONE means "I don't see any indication that my device needs
> service."  If every handler for the IRQ returns IRQ_NONE, the
> interrupt was spurious, and if we see enough spurious interrupts,
> we'll disable that IRQ completely.  In this case, our device
> definitely *did* request service; it's just that a driver wants us to
> ignore hotplug events.  From the point of view of the kernel, we did
> handle the IRQ (by ignoring it and clearing the interrupt request).
> 

Yes it does. Thanks a lot for the clarifications.

> > >  			}
> > >  		}
> > >  	}
> > >
> > > -	if (!(intr_loc & ~PCI_EXP_SLTSTA_CC))
> > > +	if (!(events & ~PCI_EXP_SLTSTA_CC))
> > >  		return IRQ_HANDLED;
> > >
> > >  	/* Check Attention Button Pressed */
> > > -	if (intr_loc & PCI_EXP_SLTSTA_ABP) {
> > > +	if (events & PCI_EXP_SLTSTA_ABP) {
> > >  		ctrl_info(ctrl, "Button pressed on Slot(%s)\n",
> > >  			  slot_name(slot));
> > >  		pciehp_queue_interrupt_event(slot, INT_BUTTON_PRESS);
> > >  	}
> > >
> > >  	/* Check Presence Detect Changed */
> > > -	if (intr_loc & PCI_EXP_SLTSTA_PDC) {
> > > +	if (events & PCI_EXP_SLTSTA_PDC) {
> > >  		pciehp_get_adapter_status(slot, &present);
> > >  		ctrl_info(ctrl, "Card %spresent on Slot(%s)\n",
> > >  			  present ? "" : "not ", slot_name(slot));
> > > @@ -615,13 +619,13 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
> > >  	}
> > >
> > >  	/* Check Power Fault Detected */
> > > -	if ((intr_loc & PCI_EXP_SLTSTA_PFD) && !ctrl->power_fault_detected) {
> > > +	if ((events & PCI_EXP_SLTSTA_PFD) && !ctrl->power_fault_detected) {
> > >  		ctrl->power_fault_detected = 1;
> > >  		ctrl_err(ctrl, "Power fault on slot %s\n", slot_name(slot));
> > >  		pciehp_queue_interrupt_event(slot, INT_POWER_FAULT);
> > >  	}
> > >
> > > -	if (intr_loc & PCI_EXP_SLTSTA_DLLSC) {
> > > +	if (events & PCI_EXP_SLTSTA_DLLSC) {
> > >  		link = pciehp_check_link_active(ctrl);
> > >  		ctrl_info(ctrl, "slot(%s): Link %s event\n",
> > >  			  slot_name(slot), link ? "Up" : "Down");
> >
> > Intel Deutschland GmbH
> > Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
> > Tel: +49 89 99 8853-0, www.intel.de
> > Managing Directors: Christin Eisenschmid, Christian Lamprechter
> > Chairperson of the Supervisory Board: Nicole Lau
> > Registered Office: Munich
> > Commercial Register: Amtsgericht Muenchen HRB 186928
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH v2 0/8] PCI: pciehp: Rework hotplug interrupt routine

[Lukas Wunner]

On Mon, Sep 12, 2016 at 04:08:47PM -0500, Bjorn Helgaas wrote:
> This is mostly Mayurkumar's work from [1] and [2].  I split [2] into two
> patches and reworked it to keep the enclosing loop around the pciehp ISR.
> 
> The patches I added are trivial ones to clarify variable names, make dmesg
> messages consistent, and remove useless code.
> 
> [1] 1471554479-42083-1-git-send-email-mayurkumar.patel@intel.com
> [2] 92EBB4272BF81E4089A7126EC1E7B28466598F35@IRSMSX101.ger.corp.intel.com

I've tested this series with Thunderbolt on the Mac, seems to work fine,
no regressions. I'll keep it on my branch and will shout if I do come
across issues over time.

Tested-by: Lukas Wunner <lukas@wunner.de>


Thanks,

Lukas

Re: [PATCH v2 0/8] PCI: pciehp: Rework hotplug interrupt routine

[Bjorn Helgaas]

On Tue, Sep 13, 2016 at 06:20:52PM +0200, Lukas Wunner wrote:
> On Mon, Sep 12, 2016 at 04:08:47PM -0500, Bjorn Helgaas wrote:
> > This is mostly Mayurkumar's work from [1] and [2].  I split [2] into two
> > patches and reworked it to keep the enclosing loop around the pciehp ISR.
> > 
> > The patches I added are trivial ones to clarify variable names, make dmesg
> > messages consistent, and remove useless code.
> > 
> > [1] 1471554479-42083-1-git-send-email-mayurkumar.patel@intel.com
> > [2] 92EBB4272BF81E4089A7126EC1E7B28466598F35@IRSMSX101.ger.corp.intel.com
> 
> I've tested this series with Thunderbolt on the Mac, seems to work fine,
> no regressions. I'll keep it on my branch and will shout if I do come
> across issues over time.
> 
> Tested-by: Lukas Wunner <lukas@wunner.de>

Thanks a lot for testing this, Lukas!  I added your Tested-by to the
series.

Re: [PATCH v2 0/8] PCI: pciehp: Rework hotplug interrupt routine

[Mika Westerberg]

On Mon, Sep 12, 2016 at 04:08:47PM -0500, Bjorn Helgaas wrote:
> This is mostly Mayurkumar's work from [1] and [2].  I split [2] into two
> patches and reworked it to keep the enclosing loop around the pciehp ISR.
> 
> The patches I added are trivial ones to clarify variable names, make dmesg
> messages consistent, and remove useless code.
> 
> [1] 1471554479-42083-1-git-send-email-mayurkumar.patel@intel.com
> [2] 92EBB4272BF81E4089A7126EC1E7B28466598F35@IRSMSX101.ger.corp.intel.com
> 
> ---
> 
> Bjorn Helgaas (5):
>       PCI: pciehp: Rename pcie_isr() locals for clarity
>       PCI: pciehp: Return IRQ_NONE when we can't read interrupt status
>       PCI: pciehp: Remove unnecessary guard
>       PCI: pciehp: Clean up dmesg "Slot(%s)" messages
>       PCI: pciehp: Remove useless pciehp_get_latch_status() calls
> 
> Mayurkumar Patel (3):
>       PCI: pciehp: Process all hotplug events before looking for new ones
>       PCI: pciehp: Don't re-read Slot Status when queuing hotplug event
>       PCI: pciehp: Don't re-read Slot Status when handling surprise event
> 
> 
>  drivers/pci/hotplug/pciehp_ctrl.c |   83 ++++++++++++---------------------
>  drivers/pci/hotplug/pciehp_hpc.c  |   94 ++++++++++++++++++++-----------------
>  2 files changed, 82 insertions(+), 95 deletions(-)

I don't have any machines with native PCIe hotplug support enabled but
regardless the series looks good to me,

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v2 0/8] PCI: pciehp: Rework hotplug interrupt routine

[Bjorn Helgaas]

On Wed, Sep 14, 2016 at 11:41:17AM +0300, Mika Westerberg wrote:
> On Mon, Sep 12, 2016 at 04:08:47PM -0500, Bjorn Helgaas wrote:
> > This is mostly Mayurkumar's work from [1] and [2].  I split [2] into two
> > patches and reworked it to keep the enclosing loop around the pciehp ISR.
> > 
> > The patches I added are trivial ones to clarify variable names, make dmesg
> > messages consistent, and remove useless code.
> > 
> > [1] 1471554479-42083-1-git-send-email-mayurkumar.patel@intel.com
> > [2] 92EBB4272BF81E4089A7126EC1E7B28466598F35@IRSMSX101.ger.corp.intel.com
> > 
> > ---
> > 
> > Bjorn Helgaas (5):
> >       PCI: pciehp: Rename pcie_isr() locals for clarity
> >       PCI: pciehp: Return IRQ_NONE when we can't read interrupt status
> >       PCI: pciehp: Remove unnecessary guard
> >       PCI: pciehp: Clean up dmesg "Slot(%s)" messages
> >       PCI: pciehp: Remove useless pciehp_get_latch_status() calls
> > 
> > Mayurkumar Patel (3):
> >       PCI: pciehp: Process all hotplug events before looking for new ones
> >       PCI: pciehp: Don't re-read Slot Status when queuing hotplug event
> >       PCI: pciehp: Don't re-read Slot Status when handling surprise event
> > 
> > 
> >  drivers/pci/hotplug/pciehp_ctrl.c |   83 ++++++++++++---------------------
> >  drivers/pci/hotplug/pciehp_hpc.c  |   94 ++++++++++++++++++++-----------------
> >  2 files changed, 82 insertions(+), 95 deletions(-)
> 
> I don't have any machines with native PCIe hotplug support enabled but
> regardless the series looks good to me,
> 
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Thanks a lot for taking the time to review it!  I added your Reviewed-by.

Re: [PATCH v2 0/8] PCI: pciehp: Rework hotplug interrupt routine

[Bjorn Helgaas]

On Mon, Sep 12, 2016 at 04:08:47PM -0500, Bjorn Helgaas wrote:
> This is mostly Mayurkumar's work from [1] and [2].  I split [2] into two
> patches and reworked it to keep the enclosing loop around the pciehp ISR.
> 
> The patches I added are trivial ones to clarify variable names, make dmesg
> messages consistent, and remove useless code.
> 
> [1] 1471554479-42083-1-git-send-email-mayurkumar.patel@intel.com
> [2] 92EBB4272BF81E4089A7126EC1E7B28466598F35@IRSMSX101.ger.corp.intel.com
> 
> ---
> 
> Bjorn Helgaas (5):
>       PCI: pciehp: Rename pcie_isr() locals for clarity
>       PCI: pciehp: Return IRQ_NONE when we can't read interrupt status
>       PCI: pciehp: Remove unnecessary guard
>       PCI: pciehp: Clean up dmesg "Slot(%s)" messages
>       PCI: pciehp: Remove useless pciehp_get_latch_status() calls
> 
> Mayurkumar Patel (3):
>       PCI: pciehp: Process all hotplug events before looking for new ones
>       PCI: pciehp: Don't re-read Slot Status when queuing hotplug event
>       PCI: pciehp: Don't re-read Slot Status when handling surprise event
> 
> 
>  drivers/pci/hotplug/pciehp_ctrl.c |   83 ++++++++++++---------------------
>  drivers/pci/hotplug/pciehp_hpc.c  |   94 ++++++++++++++++++++-----------------
>  2 files changed, 82 insertions(+), 95 deletions(-)

I applied these on pci/hotplug for v4.9.