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@xxxxxxxxxx>
> > > >
> > > > 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
