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")?
BLINKINGOFF_STATE and ON_STATE are the only cases where
pciehp_handle_presence_or_link_change() calls pciehp_disable_slot() to
turn off slot power.
> > The message print like this:
> > pcieport 0000:00:01.5: pciehp: Slot(0-5): Attention button pressed
> > pcieport 0000:00:01.5: pciehp: Slot(0-5) Powering on due to button press
> > pcieport 0000:00:01.5: pciehp: Slot(0-5): Attention button pressed
> > pcieport 0000:00:01.5: pciehp: Slot(0-5): Button cancel
> > pcieport 0000:00:01.5: pciehp: Slot(0-5): Action canceled due to button press
I think the above messages are from *two* button presses, which
complicates the description unnecessarily, since IIUC we only need one
press to see the problem.
I propose the following commit log:
If a PCIe hotplug slot has an Attention Button, the normal hot-add
flow is:
- Slot is empty and slot power is off
- User inserts card in slot and presses Attention Button
- OS blinks Power Indicator for 5 seconds
- After 5 seconds, OS turns on Power Indicator, turns on slot
power, and enumerates the device
Previously, if a user pressed the Attention Button on an *empty*
slot, pciehp logged the following messages and blinked the Power
Indicator indefinitely:
[0.000] pcieport 0000:00:01.5: pciehp: Slot(0-5): Attention button pressed
[0.001] pcieport 0000:00:01.5: pciehp: Slot(0-5): Powering on due to button press
An empty slot is in OFF_STATE. When the Attention Button is pressed,
pciehp_handle_button_press() puts the slot in BLINKINGON_STATE, sets
the Power Indicator blinking, and schedules pciehp_queue_pushbutton_work()
to run 5 seconds later.
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 Power Indicator will blink for 5 seconds before stopping, and
messages like this will be logged:
[0.000] pcieport 0000:00:01.5: pciehp: Slot(0-5): Attention button pressed
[0.001] pcieport 0000:00:01.5: pciehp: Slot(0-5): Powering on due to button press
[5.001] pcieport 0000:00:01.5: pciehp: Slot(0-5): Card not present
Let me know if I got anything wrong above.
Bjorn
> > Fixes: d331710ea78f ("PCI: pciehp: Become resilient to missed events")
> > Link: https://lore.kernel.org/linux-pci/20230403054619.19163-1-clementwei90@xxxxxxx/
> > Link: https://lore.kernel.org/linux-pci/20230421025641.655991-1-clementwei90@xxxxxxx/
> > Suggested-by: Lukas Wunner <lukas@xxxxxxxxx>
> > Signed-off-by: Rongguang Wei <weirongguang@xxxxxxxxxx>
> > ---
> > drivers/pci/hotplug/pciehp_ctrl.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> > index 529c34808440..32baba1b7f13 100644
> > --- a/drivers/pci/hotplug/pciehp_ctrl.c
> > +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> > @@ -256,6 +256,14 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
> > present = pciehp_card_present(ctrl);
> > link_active = pciehp_check_link_active(ctrl);
> > if (present <= 0 && link_active <= 0) {
> > + if (ctrl->state == BLINKINGON_STATE) {
> > + ctrl->state = OFF_STATE;
> > + cancel_delayed_work(&ctrl->button_work);
> > + pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_OFF,
> > + INDICATOR_NOOP);
> > + ctrl_info(ctrl, "Slot(%s): Card not present\n",
> > + slot_name(ctrl));
> > + }
> > mutex_unlock(&ctrl->state_lock);
> > return;
> > }
