Re: [PATCH 12/16] PCI/pciehp: Fix powerfault detection order

[Keith Busch <keith.busch@intel.com>]

On Sat, Sep 01, 2018 at 05:18:36PM +0200, Lukas Wunner wrote:
> * In pcie_enable_notification() there's a code comment:
>   "TBD: Power fault detected software notification support"
>   Is there really anything left to be done or can this code comment be
>   deleted?  AFAICS, a power fault *is* detected and acted upon, then
>   has to be cleared by the user, either through enablement via sysfs
>   or by physically replacing the card.
> 
> * In pciehp_ist(), the if-condition to check for a power fault also
>   checks for "&& !ctrl->power_fault_detected".  The check seems
>   superfluous to me because pciehp_isr() clears the PCI_EXP_SLTSTA_PFD
>   bit in the pending events if ctrl->power_fault_detected is true.
>   Can the additional check be removed from the if-condition in
>   pciehp_ist()?

The slot is supposed to have PFD set while the controller detects a
power fault. The host acknowledging the event doesn't actually make the
power fault stop exisiting, so we have to make the observation sticky
until it goes away.
 
> * In pciehp_ist(), we call pciehp_green_led_off() if a power fault is
>   detected.  However my (limited) understanding is that a power fault
>   always results in a Link Down, and when bringing down the slot in
>   response to that, we already call pciehp_green_led_off() in
>   remove_board().  Can the call to pciehp_green_led_off() be removed
>   from pciehp_ist()?  Or does the link not necessarily go down on a
>   power fault and we should rather bring the slot down when a power
>   fault is detected?  Is that the "TBD" mentioned in the code comment?
>   Or is this just an intentional cosmetic behavior that we turn off
>   the green LED as early as possible once we detect the slot is off?