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

[Bjorn Helgaas <helgaas@kernel.org>]

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")?

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@163.com/
> > Link: https://lore.kernel.org/linux-pci/20230421025641.655991-1-clementwei90@163.com/
> > Suggested-by: Lukas Wunner <lukas@wunner.de>
> > Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>
> > ---
> >  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;
> >  	}