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

[Lukas Wunner <lukas@wunner.de>]

On Thu, May 18, 2023 at 06:09:41AM -0500, Bjorn Helgaas wrote:
> On Thu, May 18, 2023 at 08:25:57AM +0200, Lukas Wunner wrote:
> > On Wed, May 17, 2023 at 04:02:01PM -0500, Bjorn Helgaas wrote:
> > > I'm curious why we want the 5 seconds of blinking power indicator at
> > > all.  We can't really do anything in response to an Attention Button
> > > on an empty slot, so could we just ignore it completely in
> > > pciehp_handle_button_press()?
> > 
> > That wouldn't cover the case where the slot is occupied when the
> > button is pressed, but the card is yanked out during the 5 second
> > blinking interval.
> 
> Obviously we can't ignore a button press when the slot is occupied,
> because that's part of the "insert card, press button to power it up"
> and "press button to power down card, remove card" flows.
> 
> I'm asking about ignoring it when the slot is empty, which would mean
> adding a check for card presence in pciehp_handle_button_press().  But
> maybe there's a reason why we can't do that there?

It would of course be possible to copy/paste the pciehp_card_present() +
pciehp_check_link_active() check from pciehp_handle_presence_or_link_change().

The only downside is that the symmetry between the ON_STATE / OFF_STATE
cases in pciehp_handle_button_press() could no longer be preserved.
(Because the additional check only applies to OFF_STATE.)  So it could
be argued that readability becomes a little worse and the logic of the
state machine slightly more difficult to follow.

Ultimately any engineering discipline boils down to balancing various
competing traits (such as simplicity of code versus usability) and
personally I would decide to continue allowing the "press button first,
insert card afterwards" usage model because it keeps the code lean.

Unfortunately back in the day the PCISIG decided to saddle PCIe hotplug
with numerous optional features which now complicate implementations.
Form factors implementing the Attention Button seem pretty rare,
as evidenced by the fact this glitch was discovered by Rongguang Wei
almost 5 years after the issue was introduced.  I think most modern
form factors just use surprise-removal instead (NVMe drives in data
centers specifically, and Thunderbolt).

The present commit is necessary anyway to deal with the "card is in slot
when button is pressed, but yanked within 5 seconds" case.  The additional
check in pciehp_handle_button_press() you're contemplating to avoid bringup
attempts if the card is not in the slot upon button press is optional.

Your call. :)

Thanks,

Lukas