[Qemu-devel] CC wangxiongfeng. RE: [PATCH] pcie: fix device unplug timeout

[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: 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.
>
@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

Re: [Qemu-devel] CC wangxiongfeng. RE: [PATCH] pcie: fix device unplug timeout

[Xiongfeng Wang]

On 2019/7/25 17:23, 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);

Sorry for the confusion. It's not timeout.
It's just that we hope the hotplug process to be faster.

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

It's because kernel will wait one second for link to become active if
Link Active Reporting Capable is not available.
Refer to 'pciehp_check_link_status()' in drivers/pci/hotplug/pciehp_hpc.c.

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

We have tried that but found out the PDC event can't be sent because PDCE is not set in SlotCtrl Register.
It's because kernel didn't set PDCE if there exists the attention button.
Refer to 'pcie_enable_notification()' in drivers/pci/hotplug/pciehp_hpc.c
It seems to avoid power interrupt storm on some machine.
Also I think we may clear Presence Detect State after the device is remove, otherwise
kernel will print 'Card not present'. But it has no bad effect, the device is still removed.
Kernel will power off the slot after remove the device. If qemu can catch this event, we can clear
Presence Detect State there.

Thanks,
Xiongfeng

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