Re: [PATCH] pci/shpc: don't push attention button when ejecting powered-off device

[Roman Kagan]

On Mon, Dec 14, 2020 at 01:40:45PM +0300, Roman Kagan wrote:
> On Mon, Nov 23, 2020 at 10:47:32AM +0300, Roman Kagan wrote:
> > On Mon, Nov 02, 2020 at 08:37:50AM +0300, Roman Kagan wrote:
> > > When the slot is in steady powered-off state and the device is being
> > > removed, there's no need to press the attention button.  Nor is it
> > > mandated by the Standard Hot-Plug Controller Specification, Rev. 1.0.
> > > 
> > > Moreover it confuses the guest, Linux in particular, as it assumes that
> > > the attention button pressed in this state indicates that the device has
> > > been inserted and will need to be powered on.  Therefore it transitions
> > > the slot into BLINKING_ON state for 5 seconds, and discovers at the end
> > > that no device is actually inserted:
> > > 
> > > ... unplug request
> > > [12685.451329] shpchp 0000:01:00.0: Button pressed on Slot(2)
> > > [12685.455478] shpchp 0000:01:00.0: PCI slot #2 - powering off due to 
> > > button press
> > > ... in 5 seconds OS powers off the slot, QEMU ejects the device
> > > [12690.632282] shpchp 0000:01:00.0: Latch open on Slot(2)
> > > ... excessive button press in steady powered-off state
> > > [12690.634267] shpchp 0000:01:00.0: Button pressed on Slot(2)
> > > [12690.636256] shpchp 0000:01:00.0: Card not present on Slot(2)
> > > ... the last button press spawns powering on the slot
> > > [12690.638909] shpchp 0000:01:00.0: PCI slot #2 - powering on due to 
> > > button press
> > > ... in 5 more seconds attempt to power on discovers empty slot
> > > [12695.735986] shpchp 0000:01:00.0: No adapter on slot(2)
> > > 
> > > Worse, if the real device insertion happens within 5 seconds from the
> > > apparent completion of the previous device removal (signaled via
> > > DEVICE_DELETED event), the new button press will be interpreted as the
> > > cancellation of that misguided powering on:
> > > 
> > > [13448.965295] shpchp 0000:01:00.0: Button pressed on Slot(2)
> > > [13448.969430] shpchp 0000:01:00.0: PCI slot #2 - powering off due to 
> > > button press
> > > [13454.025107] shpchp 0000:01:00.0: Latch open on Slot(2)
> > > [13454.027101] shpchp 0000:01:00.0: Button pressed on Slot(2)
> > > [13454.029165] shpchp 0000:01:00.0: Card not present on Slot(2)
> > > ... the excessive button press spawns powering on the slot
> > > ... device has already been ejected by QEMU
> > > [13454.031949] shpchp 0000:01:00.0: PCI slot #2 - powering on due to 
> > > button press
> > > ... new device is inserted in the slot
> > > [13456.861545] shpchp 0000:01:00.0: Latch close on Slot(2)
> > > ... valid button press arrives before 5 s since the wrong one
> > > [13456.864894] shpchp 0000:01:00.0: Button pressed on Slot(2)
> > > [13456.869211] shpchp 0000:01:00.0: Card present on Slot(2)
> > > ... the valid button press is counted as cancellation of the wrong one
> > > [13456.873173] shpchp 0000:01:00.0: Button cancel on Slot(2)
> > > [13456.877101] shpchp 0000:01:00.0: PCI slot #2 - action canceled due to 
> > > button press
> > > 
> > > As a result, the newly inserted device isn't brought up by the guest.
> > > 
> > > Avoid this situation by not pushing the attention button when the device
> > > in the slot is in powered-off state and is being ejected.
> > > 
> > > FWIW pcie implementation doesn't suffer from this problem.
> > > 
> > > Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> > > ---
> > >  hw/pci/shpc.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
> > > index b00dce629c..837159c5bd 100644
> > > --- a/hw/pci/shpc.c
> > > +++ b/hw/pci/shpc.c
> > > @@ -300,7 +300,6 @@ static void shpc_slot_command(SHPCDevice *shpc, 
> > > uint8_t target,
> > >              shpc_set_status(shpc, slot, SHPC_SLOT_STATUS_PRSNT_EMPTY,
> > >                              SHPC_SLOT_STATUS_PRSNT_MASK);
> > >              shpc->config[SHPC_SLOT_EVENT_LATCH(slot)] |=
> > > -                SHPC_SLOT_EVENT_BUTTON |
> > >                  SHPC_SLOT_EVENT_MRL |
> > >                  SHPC_SLOT_EVENT_PRESENCE;
> > >          }
> > > @@ -566,7 +565,6 @@ void shpc_device_unplug_request_cb(HotplugHandler 
> > > *hotplug_dev,
> > >          return;
> > >      }
> > >  
> > > -    shpc->config[SHPC_SLOT_EVENT_LATCH(slot)] |= SHPC_SLOT_EVENT_BUTTON;
> > >      state = shpc_get_status(shpc, slot, SHPC_SLOT_STATE_MASK);
> > >      led = shpc_get_status(shpc, slot, SHPC_SLOT_PWR_LED_MASK);
> > >      if (state == SHPC_STATE_DISABLED && led == SHPC_LED_OFF) {
> > > @@ -577,6 +575,8 @@ void shpc_device_unplug_request_cb(HotplugHandler 
> > > *hotplug_dev,
> > >          shpc->config[SHPC_SLOT_EVENT_LATCH(slot)] |=
> > >              SHPC_SLOT_EVENT_MRL |
> > >              SHPC_SLOT_EVENT_PRESENCE;
> > > +    } else {
> > > +        shpc->config[SHPC_SLOT_EVENT_LATCH(slot)] |= 
> > > SHPC_SLOT_EVENT_BUTTON;
> > >      }
> > >      shpc_set_status(shpc, slot, 0, SHPC_SLOT_STATUS_66);
> > >      shpc_interrupt_update(pci_hotplug_dev);
> > > -- 
> > > 2.28.0
> > > 
> > 
> > Ping?
> 
> Ping?

Ping?