Re: [PATCH] pciehp: Fix infinite interupt handler loop

[Bjorn Helgaas]

On Tue, Aug 01, 2017 at 03:11:52AM -0400, Keith Busch wrote:
> We've encountered a particular platform that under some circumstances
> always has the power fault detected status raised. The pciehp irq handler
> would loop forever because it thinks it is handling new events when in
> fact the power fault is not new. This patch fixes that by masking off
> the power fault status from new events if the driver hasn't seen the
> power fault clear from the previous handling attempt.

Can you say which platform this is?  If this is a hardware defect,
it'd be interesting to know where it happens.

But I'm not sure we handle PCI_EXP_SLTSTA correctly.  We basically
have this:

  pciehp_isr()
  {
    pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &status);
    events = status & (<events we care about>);
    pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, events);
    <queue event handling>
  }

The write to PCI_EXP_SLTSTA clears PCI_EXP_SLTSTA_PFD because it's
RW1C.  But we haven't done anything that would actually change the
situation that caused a power fault, so I don't think it would be
surprising if the hardware immediately reasserted it.

So maybe this continual assertion of power fault is really a software
bug, not a hardware problem?

> Fixes: fad214b0aa72 ("PCI: pciehp: Process all hotplug events before looking for new ones")
> 
> Cc: <stable@xxxxxxxxxxxxxxx> # 4.9+
> Cc: Mayurkumar Patel <mayurkumar.patel@xxxxxxxxx>
> Signed-off-by: Keith Busch <keith.busch@xxxxxxxxx>
> ---
> Resending due to send-email setup error; this patch may appear twice
> for some.
> 
>  drivers/pci/hotplug/pciehp_hpc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 026830a..8ecbc13 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -583,7 +583,9 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
>  	 * Slot Status contains plain status bits as well as event
>  	 * notification bits; right now we only want the event bits.
>  	 */
> -	events = status & (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
> +	events = status & (PCI_EXP_SLTSTA_ABP |
> +			  (ctrl->power_fault_detected ?
> +				0 : PCI_EXP_SLTSTA_PFD) |
>  			   PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_CC |
>  			   PCI_EXP_SLTSTA_DLLSC);
>  	if (!events)
> -- 
> 2.5.5
>