On Thu, Sep 06, 2018 at 02:36:57PM -0500, Bjorn Helgaas wrote: > On Wed, Sep 05, 2018 at 02:35:41PM -0600, Keith Busch wrote: > > A device add in a power controller controlled slot will power on and > > clear power fault slot events, but this was happening before the interrupt > > handler attempted to set the sticky status and attention indicators. The > > wrong status will be set if a hot-add and power fault are handled in > > one interrupt. This patch fixes that by checking for power faults before > > checking for new devices. > > Can you clarify the part about "the interrupt handler attempting to set the > sticky status and attention indicators"? My first impression is that > you're talking about bits in the Slot Status register, but that's > obviously wrong because those bits are set by hardware (not the interrupt > handler) and they're RW1C so software clears them by writing 1 to them. The sticky status being the pciehp driver's "power_fault_detected" field. We set it on the first observation of a slot's PFD and do not clear it until we have a successful board_added event. > Lukas suggests that this patch should be in v4.19. Do you agree, and if > so, can you help me justify it by describing the user-visible effect of > this? I'm not sure what "setting the wrong status" means to a user, e.g., > does this result in a non-functional device, an incorrect status LED on the > slot, something else? Does it fix a regression or something we merged for > v4.19? >From a user point of view, it is possible the attention LED light could be on after a successful hot add. The only reason this was successful before was how everything was chained through work queues, the work order being: INT_PRESENCE_ON -> INT_POWER_FAULT -> ENABLE_REQ The ENABLE_REQ cleared the power fault at the end, but now everything is handled inline with the interrupt thread (which was a great change, IMO), such that the work ENABLE_REQ was doing happens before power fault handling now. The commit that changed that order: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=0e94916e6091f48391b65110e71c87c583021640 > > Signed-off-by: Keith Busch <keith.busch@xxxxxxxxx> > > Reviewed-by: Lukas Wunner <lukas@xxxxxxxxx> > > --- > > drivers/pci/hotplug/pciehp_hpc.c | 16 ++++++++-------- > > 1 file changed, 8 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c > > index 9eb28a06cac6..52a18a7ec2a2 100644 > > --- a/drivers/pci/hotplug/pciehp_hpc.c > > +++ b/drivers/pci/hotplug/pciehp_hpc.c > > @@ -630,6 +630,14 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id) > > pciehp_handle_button_press(slot); > > } > > > > + /* Check Power Fault Detected */ > > + if ((events & PCI_EXP_SLTSTA_PFD) && !ctrl->power_fault_detected) { > > + ctrl->power_fault_detected = 1; > > + ctrl_err(ctrl, "Slot(%s): Power fault\n", slot_name(slot)); > > + pciehp_set_attention_status(slot, 1); > > + pciehp_green_led_off(slot); > > + } > > + > > /* > > * Disable requests have higher priority than Presence Detect Changed > > * or Data Link Layer State Changed events. > > @@ -641,14 +649,6 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id) > > pciehp_handle_presence_or_link_change(slot, events); > > up_read(&ctrl->reset_lock); > > > > - /* Check Power Fault Detected */ > > - if ((events & PCI_EXP_SLTSTA_PFD) && !ctrl->power_fault_detected) { > > - ctrl->power_fault_detected = 1; > > - ctrl_err(ctrl, "Slot(%s): Power fault\n", slot_name(slot)); > > - pciehp_set_attention_status(slot, 1); > > - pciehp_green_led_off(slot); > > - } > > - > > pci_config_pm_runtime_put(pdev); > > wake_up(&ctrl->requester); > > return IRQ_HANDLED; > > -- > > 2.14.4 > >
