diff mbox series

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

Message ID 20201102053750.2281818-1-rvkagan@yandex-team.ru
State Accepted
Commit 5d593bdf10aace376c83161aa6d6c828cd6fd428
Headers show
Series pci/shpc: don't push attention button when ejecting powered-off device | expand

Commit Message

Roman Kagan Nov. 2, 2020, 5:37 a.m. UTC
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(-)

Comments

Roman Kagan Nov. 23, 2020, 7:47 a.m. UTC | #1
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?
Roman Kagan Dec. 14, 2020, 10:40 a.m. UTC | #2
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?
Roman Kagan Jan. 13, 2021, 9:47 a.m. UTC | #3
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?
diff mbox series

Patch

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);