Re: [PATCH v4] PCI: pciehp: Fix the slot in BLINKINGON_STATE when Presence Detect Changed event occurred

[Bjorn Helgaas <helgaas@kernel.org>]

On Sat, May 20, 2023 at 10:31:18AM +0200, Lukas Wunner wrote:
> On Fri, May 19, 2023 at 03:55:35PM -0500, Bjorn Helgaas wrote:
> > On Wed, May 17, 2023 at 04:02:01PM -0500, Bjorn Helgaas wrote:
> > > On Fri, May 12, 2023 at 10:15:18AM +0800, Rongguang Wei wrote:
> > > > From: Rongguang Wei <weirongguang@kylinos.cn>
> > > > 
> > > > pciehp's behavior is incorrect if the Attention Button is pressed
> > > > on an unoccupied slot.
> > > > 
> > > > When a Presence Detect Changed event has occurred, the slot status
> > > > in either BLINKINGOFF_STATE or OFF_STATE, turn it off unconditionally.
> > 
> > Was this supposed to say "BLINKINGOFF_STATE or ON_STATE"
> > (not "OFF_STATE")?
> 
> Yes I think you're right.
> 
> > I propose the following commit log:
> [...]
> >   pciehp_queue_pushbutton_work() synthesizes a Presence Detect Changed
> >   event, and pciehp_handle_presence_or_link_change() exits when it
> >   finds the slot empty, leaving the slot in BLINKINGON_STATE with the
> >   Power Indicator blinking.
> > 
> >   To fix the indefinitely blinking Power Indicator, change
> >   pciehp_handle_presence_or_link_change() to put the empty slot back
> >   in OFF_STATE and turn off the Power Indicator before exiting.
> 
> The indefinitely blinking Power Indicator is only one half of the problem.
> The other half is that the next button press doesn't result in slot
> bringup, even if the slot is occupied and the 5 second timeout has
> elapsed.  

Thanks for your patience, I think I understand that.  Here's another
try:

  Previously, if a user pressed the Attention Button on an *empty* slot,
  pciehp logged the following messages and blinked the Power Indicator
  until a second button press:

    [0.000] pciehp: Attention button pressed
    [0.001] pciehp: Powering on due to button press
    [0.002] # Power Indicator starts blinking
    [5.002] # 5 second timeout should abort power-on sequence, but doesn't

    [8.000] # Power Indicator should be off, but is still blinking
    [9.000] # possible card insertion
    [9.000] pciehp: Attention button pressed
    [9.001] pciehp: Button cancel
    [9.002] pciehp: Action canceled due to button press

  The first button press incorrectly left the slot in BLINKINGON_STATE,
  so the second was interpreted as a "button cancel" event regardless of
  whether a card was present.

  If the slot is empty, turn off the Power Indicator and return from
  BLINKINGON_STATE to OFF_STATE after 5 seconds.  Putting the slot in
  OFF_STATE also means the second button press will correctly start a
  bringup attempt if the slot is occupied.

Maybe the above is enough for a commit log.  The notes below are my
attempt to work through in more detail:

IIUC, if the button is pressed twice on an empty slot, we end up back
in the "Empty slot, OFF" state (although the indicator blinks until
the second press, when it should stop after 5 seconds), and inserting
a card and pressing the button works as expected.

The problem is when the card is inserted between first and second
button presses, where the second press cancels the BLINKINGON when it
should *start* BLINKINGON.  A third press would power on the slot,
when it should go to BLINKINGOFF to power it off:

                    Slot        v6.4               Expected
                    --------    -----------        -----------
  Slot empty        Empty       OFF                OFF
  Button press 1    Empty       BLINKINGON         BLINKINGON
                                "Powering on"      "Powering on"
                                sched-work         sched-work
    +5s synth PDC   Empty       BLINKINGON         OFF
                                (a)                "Card not present"
  Insert card       Occupied    BLINKINGON         OFF
  Button press 2    Occupied    OFF                BLINKINGON
                                "Button cancel"    "Powering on"
                                                   sched-work
    +5s synth PDC   Occupied    (b, N/A)           POWERON
    Power control   Occupied    (b, N/A)           ON
  Button press 3    Occupied    BLINKINGON         BLINKINGOFF
                                "Powering on"      "Powering off"
                                sched-work         sched-work
    +5s synth PDC   Occupied    POWERON            POWEROFF
    Power control   Occupied    ON                 OFF

At (a), v6.4-rc1 will blink until another button press.  At (b), the
button press generates a "Button cancel" message and does not schedule
button_work.

And (b) is the situation you refer to where the second button press
doesn't bring the slot up when it should.  Right?

> Suggested wording, feel free to rephrase as you see fit:
> 
>   Because the slot was previously left in BLINKINGON_STATE, the next
>   button press was interpreted as a "button cancel" event, even if the
>   slot was occupied upon that next button press:  pciehp stopped blinking
>   and did not perform another slot bringup attempt.
> 
>   By putting the slot in OFF_STATE, such user-unfriendly behavior is
>   avoided:  Instead, the next button press will result in the slot
>   starting to blink again and another bringup attempt after 5 seconds.
> 
> Thanks,
> 
> Lukas