From 8b7c8b46f111ae56df4fd196fcb0fd2495f3b966 Mon Sep 17 00:00:00 2001 From: Keith Busch Date: Wed, 3 Aug 2016 17:29:45 -0600 Subject: [PATCH 01/11] PCI: pciehp: Clear attention LED on device add Clear the LED attention status after a successful device add. It is possible the attention LED was on from a previous power fault or link failure, and a subsequent successful device insert insertion should clear it. Signed-off-by: Keith Busch Signed-off-by: Bjorn Helgaas --- drivers/pci/hotplug/pciehp_ctrl.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c index 880978b6d534c..7ea3e61ea745c 100644 --- a/drivers/pci/hotplug/pciehp_ctrl.c +++ b/drivers/pci/hotplug/pciehp_ctrl.c @@ -120,6 +120,7 @@ static int board_added(struct slot *p_slot) } pciehp_green_led_on(p_slot); + pciehp_set_attention_status(p_slot, 0); return 0; err_exit: From a8499f20d30e0f9632017fd27a8f1d8c146a6a33 Mon Sep 17 00:00:00 2001 From: Bjorn Helgaas Date: Thu, 8 Sep 2016 17:30:38 -0500 Subject: [PATCH 02/11] PCI: pciehp: Rename pcie_isr() locals for clarity 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. Tested-by: Lukas Wunner Signed-off-by: Bjorn Helgaas Reviewed-by: Mika Westerberg --- 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 08e84d61874e1..264df360444ca 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"); From 70e8b40176c75d3544024e7c934720b11a8a11bf Mon Sep 17 00:00:00 2001 From: Bjorn Helgaas Date: Thu, 8 Sep 2016 16:43:40 -0500 Subject: [PATCH 03/11] PCI: pciehp: Return IRQ_NONE when we can't read interrupt status 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. Tested-by: Lukas Wunner Signed-off-by: Bjorn Helgaas Reviewed-by: Mika Westerberg --- 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 264df360444ca..b8efe1bbc4f41 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; } /* From fad214b0aa726ee21adcac4308d388efcb89d6bd Mon Sep 17 00:00:00 2001 From: Mayurkumar Patel Date: Thu, 8 Sep 2016 15:07:56 -0500 Subject: [PATCH 04/11] PCI: pciehp: Process all hotplug events before looking for new ones 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()] Tested-by: Lukas Wunner Signed-off-by: Mayurkumar Patel Signed-off-by: Bjorn Helgaas Reviewed-by: Mika Westerberg --- 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 b8efe1bbc4f41..625fa6a2f3ab3 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; From 0c923d1da394b96727b813d1e64412b72f1dc580 Mon Sep 17 00:00:00 2001 From: Mayurkumar Patel Date: Fri, 9 Sep 2016 09:10:17 -0500 Subject: [PATCH 05/11] PCI: pciehp: Don't re-read Slot Status when queuing hotplug event 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] Tested-by: Lukas Wunner Signed-off-by: Mayurkumar Patel Signed-off-by: Bjorn Helgaas Reviewed-by: Mika Westerberg --- 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 625fa6a2f3ab3..fe99b45c59255 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 : From 69bd3c5b28e7c988886efb3c9a7614a612eef45d Mon Sep 17 00:00:00 2001 From: Mayurkumar Patel Date: Tue, 23 Aug 2016 08:58:51 +0000 Subject: [PATCH 06/11] PCI: pciehp: Don't re-read Slot Status when handling surprise event 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] Tested-by: Lukas Wunner Signed-off-by: Mayurkumar Patel Signed-off-by: Bjorn Helgaas Reviewed-by: Mika Westerberg Acked-by: Rajat Jain --- 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 7ea3e61ea745c..a7876842d5c17 100644 --- a/drivers/pci/hotplug/pciehp_ctrl.c +++ b/drivers/pci/hotplug/pciehp_ctrl.c @@ -299,20 +299,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 */ @@ -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: From 4947793916e31ec0c4c56f979e2aff89d15480bf Mon Sep 17 00:00:00 2001 From: Bjorn Helgaas Date: Thu, 8 Sep 2016 15:15:24 -0500 Subject: [PATCH 07/11] PCI: pciehp: Remove unnecessary guard 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. Tested-by: Lukas Wunner Signed-off-by: Bjorn Helgaas Reviewed-by: Mika Westerberg --- 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 fe99b45c59255..60e1d55b4eefe 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", From 6e49b304e379f93c8aa7ebb164628aec1209f371 Mon Sep 17 00:00:00 2001 From: Bjorn Helgaas Date: Thu, 8 Sep 2016 15:19:58 -0500 Subject: [PATCH 08/11] PCI: pciehp: Clean up dmesg "Slot(%s)" messages 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. Tested-by: Lukas Wunner Signed-off-by: Bjorn Helgaas Reviewed-by: Mika Westerberg --- 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 a7876842d5c17..bf50f26811782 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 60e1d55b4eefe..4582fdf2d8b52 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); } From 29a654e59f3698d70d85e289fc5ce7261493bba2 Mon Sep 17 00:00:00 2001 From: Bjorn Helgaas Date: Wed, 7 Sep 2016 17:50:30 -0500 Subject: [PATCH 09/11] PCI: pciehp: Remove useless pciehp_get_latch_status() calls 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. Tested-by: Lukas Wunner Signed-off-by: Bjorn Helgaas Reviewed-by: Mika Westerberg --- 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 bf50f26811782..efe69e879455c 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); } /* From 576243b3f9eaa47ab568ac49574b3a095c2365f1 Mon Sep 17 00:00:00 2001 From: Keith Busch Date: Tue, 13 Sep 2016 10:31:59 -0600 Subject: [PATCH 10/11] PCI: pciehp: Allow exclusive userspace control of indicators PCIe hotplug supports optional Attention and Power Indicators, which are used internally by pciehp. Users can't control the Power Indicator, but they can control the Attention Indicator by writing to a sysfs "attention" file. The Slot Control register has two bits for each indicator, and the PCIe spec defines the encodings for each as (Reserved/On/Blinking/Off). For sysfs "attention" writes, pciehp_set_attention_status() maps into these encodings, so the only useful write values are 0 (Off), 1 (On), and 2 (Blinking). However, some platforms use all four bits for platform-specific indicators, and they need to allow direct user control of them while preventing pciehp from using them at all. Add a "hotplug_user_indicators" flag to the pci_dev structure. When set, pciehp does not use either the Attention Indicator or the Power Indicator, and the low four bits (values 0x0 - 0xf) of sysfs "attention" write values are written directly to the Attention Indicator Control and Power Indicator Control fields. [bhelgaas: changelog, rename flag and accessors to s/attention/indicator/] Signed-off-by: Keith Busch Signed-off-by: Bjorn Helgaas --- drivers/pci/hotplug/pciehp.h | 3 +++ drivers/pci/hotplug/pciehp_core.c | 3 +++ drivers/pci/hotplug/pciehp_hpc.c | 27 +++++++++++++++++++++++++++ include/linux/pci.h | 3 +++ 4 files changed, 36 insertions(+) diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h index e764918641ae2..37d70b5ad22f9 100644 --- a/drivers/pci/hotplug/pciehp.h +++ b/drivers/pci/hotplug/pciehp.h @@ -152,6 +152,9 @@ bool pciehp_check_link_active(struct controller *ctrl); void pciehp_release_ctrl(struct controller *ctrl); int pciehp_reset_slot(struct slot *slot, int probe); +int pciehp_set_raw_indicator_status(struct hotplug_slot *h_slot, u8 status); +int pciehp_get_raw_indicator_status(struct hotplug_slot *h_slot, u8 *status); + static inline const char *slot_name(struct slot *slot) { return hotplug_slot_name(slot->hotplug_slot); diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c index ac531e674a051..276e39c64d06f 100644 --- a/drivers/pci/hotplug/pciehp_core.c +++ b/drivers/pci/hotplug/pciehp_core.c @@ -114,6 +114,9 @@ static int init_slot(struct controller *ctrl) if (ATTN_LED(ctrl)) { ops->get_attention_status = get_attention_status; ops->set_attention_status = set_attention_status; + } else if (ctrl->pcie->port->hotplug_user_indicators) { + ops->get_attention_status = pciehp_get_raw_indicator_status; + ops->set_attention_status = pciehp_set_raw_indicator_status; } /* register this slot with the hotplug pci core */ diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c index 4582fdf2d8b52..b57fc6d6e28a6 100644 --- a/drivers/pci/hotplug/pciehp_hpc.c +++ b/drivers/pci/hotplug/pciehp_hpc.c @@ -355,6 +355,18 @@ static int pciehp_link_enable(struct controller *ctrl) return __pciehp_link_set(ctrl, true); } +int pciehp_get_raw_indicator_status(struct hotplug_slot *hotplug_slot, + u8 *status) +{ + struct slot *slot = hotplug_slot->private; + struct pci_dev *pdev = ctrl_dev(slot->ctrl); + u16 slot_ctrl; + + pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl); + *status = (slot_ctrl & (PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC)) >> 6; + return 0; +} + void pciehp_get_attention_status(struct slot *slot, u8 *status) { struct controller *ctrl = slot->ctrl; @@ -431,6 +443,17 @@ int pciehp_query_power_fault(struct slot *slot) return !!(slot_status & PCI_EXP_SLTSTA_PFD); } +int pciehp_set_raw_indicator_status(struct hotplug_slot *hotplug_slot, + u8 status) +{ + struct slot *slot = hotplug_slot->private; + struct controller *ctrl = slot->ctrl; + + pcie_write_cmd_nowait(ctrl, status << 6, + PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC); + return 0; +} + void pciehp_set_attention_status(struct slot *slot, u8 value) { struct controller *ctrl = slot->ctrl; @@ -814,6 +837,10 @@ struct controller *pcie_init(struct pcie_device *dev) } ctrl->pcie = dev; pcie_capability_read_dword(pdev, PCI_EXP_SLTCAP, &slot_cap); + + if (pdev->hotplug_user_indicators) + slot_cap &= ~(PCI_EXP_SLTCAP_AIP | PCI_EXP_SLTCAP_PIP); + ctrl->slot_cap = slot_cap; mutex_init(&ctrl->ctrl_lock); init_waitqueue_head(&ctrl->queue); diff --git a/include/linux/pci.h b/include/linux/pci.h index 2599a980340f4..c81fbf7d5e9e1 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -308,6 +308,9 @@ struct pci_dev { powered on/off by the corresponding bridge */ unsigned int ignore_hotplug:1; /* Ignore hotplug events */ + unsigned int hotplug_user_indicators:1; /* SlotCtl indicators + controlled exclusively by + user sysfs */ unsigned int d3_delay; /* D3->D0 transition time in ms */ unsigned int d3cold_delay; /* D3cold->D0 transition time in ms */ From 3161832d58c7f3bf8b190a2887086be0932d8dd3 Mon Sep 17 00:00:00 2001 From: Keith Busch Date: Tue, 13 Sep 2016 09:05:40 -0600 Subject: [PATCH 11/11] x86/PCI: VMD: Request userspace control of PCIe hotplug indicators Add set_dev_domain_options() to set PCI domain-specific options as devices are added. The first usage is to request exclusive userspace control of PCIe hotplug indicators in VMD domains. Devices in a VMD domain use PCIe hotplug Attention and Power Indicators in a non-standard way; tell pciehp to ignore the indicators so userspace can control them via the sysfs "attention" file. To determine whether a bus is within a VMD domain, add a bool to the pci_sysdata structure that the VMD driver sets during initialization. [bhelgaas: changelog] Requested-by: Kapil Karkra Tested-by: Artur Paszkiewicz Signed-off-by: Keith Busch Signed-off-by: Bjorn Helgaas --- arch/x86/include/asm/pci.h | 14 ++++++++++++++ arch/x86/pci/common.c | 7 +++++++ arch/x86/pci/vmd.c | 1 + 3 files changed, 22 insertions(+) diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h index 9ab7507ca1c2c..1411dbed5e5e7 100644 --- a/arch/x86/include/asm/pci.h +++ b/arch/x86/include/asm/pci.h @@ -23,6 +23,9 @@ struct pci_sysdata { #ifdef CONFIG_PCI_MSI_IRQ_DOMAIN void *fwnode; /* IRQ domain for MSI assignment */ #endif +#if IS_ENABLED(CONFIG_VMD) + bool vmd_domain; /* True if in Intel VMD domain */ +#endif }; extern int pci_routeirq; @@ -56,6 +59,17 @@ static inline void *_pci_root_bus_fwnode(struct pci_bus *bus) #define pci_root_bus_fwnode _pci_root_bus_fwnode #endif +static inline bool is_vmd(struct pci_bus *bus) +{ +#if IS_ENABLED(CONFIG_VMD) + struct pci_sysdata *sd = bus->sysdata; + + return sd->vmd_domain; +#else + return false; +#endif +} + /* Can be used to override the logic in pci_scan_bus for skipping already-configured bus numbers - to be used for buggy BIOSes or architectures with incomplete PCI setup by the loader */ diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c index 7b6a9d14c8c0a..a4fdfa7dcc1bc 100644 --- a/arch/x86/pci/common.c +++ b/arch/x86/pci/common.c @@ -677,6 +677,12 @@ static void set_dma_domain_ops(struct pci_dev *pdev) static void set_dma_domain_ops(struct pci_dev *pdev) {} #endif +static void set_dev_domain_options(struct pci_dev *pdev) +{ + if (is_vmd(pdev->bus)) + pdev->hotplug_user_indicators = 1; +} + int pcibios_add_device(struct pci_dev *dev) { struct setup_data *data; @@ -707,6 +713,7 @@ int pcibios_add_device(struct pci_dev *dev) iounmap(data); } set_dma_domain_ops(dev); + set_dev_domain_options(dev); return 0; } diff --git a/arch/x86/pci/vmd.c b/arch/x86/pci/vmd.c index b814ca675131e..a021b7b0eb69a 100644 --- a/arch/x86/pci/vmd.c +++ b/arch/x86/pci/vmd.c @@ -596,6 +596,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd) .parent = res, }; + sd->vmd_domain = true; sd->domain = vmd_find_free_domain(); if (sd->domain < 0) return sd->domain;