[Qemu-devel] [PATCH] pcie: fix device unplug timeout

[Qemu-devel] [PATCH] pcie: fix device unplug timeout

[Zhangbo (Oscar)]

If the linux kernel only receives an ABP event during pcie unplug, it will sleep 5s 
to expect a PDC event, which will cause device unplug timeout.

In the meanwhile, if the kernel only receives a PDC event during the unplug, it
will wait for at least 1 second before checking card present as data link layer
state changed (link down) event reported prior to presence detect changed
(card is not present).

Therefore we can send both ABP and PDC events to the kernel in unplug process
to avoid unplug timeout.

Signed-off-by: limingwang@huawei.com
Signed-off-by: fangying1@huawei.com
Signed-off-by: oscar.zhangbo@huawei.com
---
 hw/pci/pcie.c         | 8 ++------
 include/hw/pci/pcie.h | 1 -
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 174f392..a800f23 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -485,7 +485,8 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
                                      PCI_EXP_LNKSTA_DLLLA);
     }
 
-    pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev));
+    pcie_cap_slot_event(PCI_DEVICE(hotplug_dev),
+                        PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
 }
 
 /* pci express slot for pci express root/downstream port
@@ -701,11 +702,6 @@ int pcie_cap_slot_post_load(void *opaque, int version_id)
     return 0;
 }
 
-void pcie_cap_slot_push_attention_button(PCIDevice *dev)
-{
-    pcie_cap_slot_event(dev, PCI_EXP_HP_EV_ABP);
-}
-
 /* root control/capabilities/status. PME isn't emulated for now */
 void pcie_cap_root_init(PCIDevice *dev)
 {
diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index 8cf3361..0975a54 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -112,7 +112,6 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
                                 uint16_t old_slt_ctl, uint16_t old_slt_sta,
                                 uint32_t addr, uint32_t val, int len);
 int pcie_cap_slot_post_load(void *opaque, int version_id);
-void pcie_cap_slot_push_attention_button(PCIDevice *dev);
 
 void pcie_cap_root_init(PCIDevice *dev);
 void pcie_cap_root_reset(PCIDevice *dev);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] pcie: fix device unplug timeout

[Michael S. Tsirkin]

On Tue, Jul 23, 2019 at 07:48:00AM +0000, Zhangbo (Oscar) wrote:
> If the linux kernel only receives an ABP event during pcie unplug, it will sleep 5s 
> to expect a PDC event, which will cause device unplug timeout.

My understanding is that there's no timeout. Spec says:
	If present, the Power Indicator provides visual feedback to the human operator (if the system
	software accepts the request initiated by the Attention Button) by blinking. Once the Power
	Indicator begins blinking, a 5-second abort interval exists during which a second depression of the
	Attention Button cancels the operation.

this is exactly what linux implements.
Thus after an ABP event, linux starts a 5 second timer:
	schedule_delayed_work(&ctrl->button_work, 5 * HZ);


> In the meanwhile, if the kernel only receives a PDC event during the unplug, it
> will wait for at least 1 second before checking card present as data link layer
> state changed (link down) event reported prior to presence detect changed
> (card is not present).

I don't understand what you are saying exactly.
But I don't see any other delayed work in
drivers/pci/hotplug/pciehp_ctrl.c


> Therefore we can send both ABP and PDC events to the kernel in unplug process
> to avoid unplug timeout.
> 
> Signed-off-by: limingwang@huawei.com
> Signed-off-by: fangying1@huawei.com
> Signed-off-by: oscar.zhangbo@huawei.com

I see this in linux:

/**
 * pciehp_check_presence() - synthesize event if presence has changed
 *              
 * On probe and resume, an explicit presence check is necessary to bring up an
 * occupied slot or bring down an unoccupied slot.  This can't be triggered by
 * events in the Slot Status register, they may be stale and are therefore
 * cleared.  Secondly, sending an interrupt for "events that occur while
 * interrupt generation is disabled [when] interrupt generation is subsequently
 * enabled" is optional per PCIe r4.0, sec 6.7.3.4.
 */


My suggestion therefore is to try to clear Presence Detect State,
set presence detect changed and do not set attention button
pressed.


> ---
>  hw/pci/pcie.c         | 8 ++------
>  include/hw/pci/pcie.h | 1 -
>  2 files changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 174f392..a800f23 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -485,7 +485,8 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
>                                       PCI_EXP_LNKSTA_DLLLA);
>      }
>  
> -    pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev));
> +    pcie_cap_slot_event(PCI_DEVICE(hotplug_dev),
> +                        PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
>  }
>  
>  /* pci express slot for pci express root/downstream port
> @@ -701,11 +702,6 @@ int pcie_cap_slot_post_load(void *opaque, int version_id)
>      return 0;
>  }
>  
> -void pcie_cap_slot_push_attention_button(PCIDevice *dev)
> -{
> -    pcie_cap_slot_event(dev, PCI_EXP_HP_EV_ABP);
> -}
> -
>  /* root control/capabilities/status. PME isn't emulated for now */
>  void pcie_cap_root_init(PCIDevice *dev)
>  {
> diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> index 8cf3361..0975a54 100644
> --- a/include/hw/pci/pcie.h
> +++ b/include/hw/pci/pcie.h
> @@ -112,7 +112,6 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
>                                  uint16_t old_slt_ctl, uint16_t old_slt_sta,
>                                  uint32_t addr, uint32_t val, int len);
>  int pcie_cap_slot_post_load(void *opaque, int version_id);
> -void pcie_cap_slot_push_attention_button(PCIDevice *dev);
>  
>  void pcie_cap_root_init(PCIDevice *dev);
>  void pcie_cap_root_reset(PCIDevice *dev);
> -- 
> 1.8.3.1