[Qemu-devel] CC wangxiongfeng. RE: [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.
>
>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.
>
@wangxiongfeng

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