[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] pcie: fix device unplug timeout
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH] pcie: fix device unplug timeout |
Date: |
Tue, 23 Jul 2019 06:09:01 -0400 |
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: address@hidden
> Signed-off-by: address@hidden
> Signed-off-by: address@hidden
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