[RFC] PCI: hotplug: Fix surprise removal report card present and link failed

Message ID 1547649064-19019-1-git-send-email-liudongdong3@huawei.com
State New
Delegated to: Bjorn Helgaas
Headers show
Series
  • [RFC] PCI: hotplug: Fix surprise removal report card present and link failed
Related show

Commit Message

Dongdong Liu Jan. 16, 2019, 2:31 p.m.
The lspci -tv topology is as below.
 +-[0000:80]-+-00.0-[81]----00.0  Huawei Technologies Co., Ltd. Device 3714
 |           +-02.0-[82]----00.0  Huawei Technologies Co., Ltd. Device 3714
 |           +-04.0-[83]----00.0  Huawei Technologies Co., Ltd. Device 3714
 |           +-06.0-[84]----00.0  Huawei Technologies Co., Ltd. Device 3714
 |           +-10.0-[87]----00.0  Huawei Technologies Co., Ltd. Device 3714

Then surprise removal 87:00.0 NVME SSD card. The message is as below.

pciehp 0000:80:10.0:pcie004: Slot(36): Link Down
iommu: Removing device 0000:87:00.0 from group 12
pciehp 0000:80:10.0:pcie004: Slot(36): Card present
pcieport 0000:80:10.0: Data Link Layer Link Active not set in 1000 msec
pciehp 0000:80:10.0:pcie004: Failed to check link status

Then lspci -s 80:10.0 -vvv|grep -i SltSta
SltSta:Status: AttnBtn- PowerFlt- MRL- CmdCplt- PresDet- Interlock
We can see the card is not present (PresDet-).

pciehp 0000:80:10.0:pcie004: Slot #36 AttnBtn- PwrCtrl- MRL- AttnInd+
PwrInd+ HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+

The NVME SSD card is permited to surprise removal with slot power on
on the D06 board. The hotplug port's POWER_CTRL(ctrl) is false.

Data link layer state changed (link down) event reported prior to presence
detect changed (card is not present) when surprise removal.

Current code pciehp_handle_presence_or_link_change() handle link down event,
then check the card present, but at this time presence detect state have
not updated, so have such issue. If surprise removal and power controller
present is not implemented, wait for at least 1 second before checking
card present to fix the issue.

Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
---
 drivers/pci/hotplug/pciehp_ctrl.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Lukas Wunner Jan. 16, 2019, 2:22 p.m. | #1
[cc += Mika]

On Wed, Jan 16, 2019 at 10:31:04PM +0800, Dongdong Liu wrote:
> The lspci -tv topology is as below.
>  +-[0000:80]-+-00.0-[81]----00.0  Huawei Technologies Co., Ltd. Device 3714
>  |           +-02.0-[82]----00.0  Huawei Technologies Co., Ltd. Device 3714
>  |           +-04.0-[83]----00.0  Huawei Technologies Co., Ltd. Device 3714
>  |           +-06.0-[84]----00.0  Huawei Technologies Co., Ltd. Device 3714
>  |           +-10.0-[87]----00.0  Huawei Technologies Co., Ltd. Device 3714
> 
> Then surprise removal 87:00.0 NVME SSD card. The message is as below.
> 
> pciehp 0000:80:10.0:pcie004: Slot(36): Link Down
> iommu: Removing device 0000:87:00.0 from group 12
> pciehp 0000:80:10.0:pcie004: Slot(36): Card present
> pcieport 0000:80:10.0: Data Link Layer Link Active not set in 1000 msec
> pciehp 0000:80:10.0:pcie004: Failed to check link status

What is the problem that you're trying to fix?  That these messages
are logged?  Or is there a bigger issue?  If the only problem are the
messages, then I feel that the current behavior is a feature, not a bug.
We could probably tone down the "Failed to check link status" message's
severity.  (Currently it's KERN_ERR, all the other messages are KERN_INFO.)


> The NVME SSD card is permited to surprise removal with slot power on
> on the D06 board. The hotplug port's POWER_CTRL(ctrl) is false.

I don't quite follow:  Does the hotplug port have a Power Controller but
misrepresents that fact in the Slot Capabilities register?


> Data link layer state changed (link down) event reported prior to presence
> detect changed (card is not present) when surprise removal.

It's normal that PDS and DLLSC events are received in varying order and with
significant delays.  (I've seen 244 msec once with Thunderbolt between
PDS- and DLLSC-.)


> Current code pciehp_handle_presence_or_link_change() handle link down event,
> then check the card present, but at this time presence detect state have
> not updated, so have such issue. If surprise removal and power controller
> present is not implemented, wait for at least 1 second before checking
> card present to fix the issue.

Thunderbolt doesn't have a Power Controller either and this change would
imply that if Thunderbolt devices are quickly swapped, bringing up the
replaced device would take longer than it takes now.  So I'm not really
happy about this change.

Thanks,

Lukas

> 
> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
> ---
>  drivers/pci/hotplug/pciehp_ctrl.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index 3f3df4c..ef8952d 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -216,6 +216,7 @@ void pciehp_handle_disable_request(struct controller *ctrl)
>  void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
>  {
>  	bool present, link_active;
> +	bool safe_removal = SAFE_REMOVAL;
>  
>  	/*
>  	 * If the slot is on and presence or link has changed, turn it off.
> @@ -236,6 +237,7 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
>  			ctrl_info(ctrl, "Slot(%s): Card not present\n",
>  				  slot_name(ctrl));
>  		pciehp_disable_slot(ctrl, SURPRISE_REMOVAL);
> +		safe_removal = SURPRISE_REMOVAL;
>  		break;
>  	default:
>  		mutex_unlock(&ctrl->state_lock);
> @@ -244,6 +246,15 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
>  
>  	/* Turn the slot on if it's occupied or link is up */
>  	mutex_lock(&ctrl->state_lock);
> +	/*
> +	 * if surprise removal and power controller present is not implemented,
> +	 * 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).
> +	 */
> +	if (!safe_removal && !POWER_CTRL(ctrl))
> +		msleep(1000);
> +
>  	present = pciehp_card_present(ctrl);
>  	link_active = pciehp_check_link_active(ctrl);
>  	if (!present && !link_active) {
> -- 
> 1.9.1
Dongdong Liu Jan. 17, 2019, 12:07 p.m. | #2
Hi Lukas

Many thanks for your reply.

在 2019/1/16 22:22, Lukas Wunner 写道:
> [cc += Mika]
>
> On Wed, Jan 16, 2019 at 10:31:04PM +0800, Dongdong Liu wrote:
>> The lspci -tv topology is as below.
>>  +-[0000:80]-+-00.0-[81]----00.0  Huawei Technologies Co., Ltd. Device 3714
>>  |           +-02.0-[82]----00.0  Huawei Technologies Co., Ltd. Device 3714
>>  |           +-04.0-[83]----00.0  Huawei Technologies Co., Ltd. Device 3714
>>  |           +-06.0-[84]----00.0  Huawei Technologies Co., Ltd. Device 3714
>>  |           +-10.0-[87]----00.0  Huawei Technologies Co., Ltd. Device 3714
>>
>> Then surprise removal 87:00.0 NVME SSD card. The message is as below.
>>
>> pciehp 0000:80:10.0:pcie004: Slot(36): Link Down
>> iommu: Removing device 0000:87:00.0 from group 12
>> pciehp 0000:80:10.0:pcie004: Slot(36): Card present
>> pcieport 0000:80:10.0: Data Link Layer Link Active not set in 1000 msec
>> pciehp 0000:80:10.0:pcie004: Failed to check link status
>
> What is the problem that you're trying to fix?  That these messages
> are logged?  Or is there a bigger issue?  If the only problem are the
> messages, then I feel that the current behavior is a feature, not a bug.
> We could probably tone down the "Failed to check link status" message's
> severity.  (Currently it's KERN_ERR, all the other messages are KERN_INFO.)
>

Yes, the only problem is the messages, looks not good,
as the card have been removed from board, the message still show
card present and failed to check link status.
Only tone down the "Failed to check link status" message's severity seems not
good enough.

>
>> The NVME SSD card is permited to surprise removal with slot power on
>> on the D06 board. The hotplug port's POWER_CTRL(ctrl) is false.
>
> I don't quite follow:  Does the hotplug port have a Power Controller but
> misrepresents that fact in the Slot Capabilities register?
>
Yes, SlotCap:  PwrCtrl- HotPlug+ Surprise+
The port support Hot-Plug Surprise without Power Controller Present.

>
>> Data link layer state changed (link down) event reported prior to presence
>> detect changed (card is not present) when surprise removal.
>
> It's normal that PDS and DLLSC events are received in varying order and with
> significant delays.  (I've seen 244 msec once with Thunderbolt between
> PDS- and DLLSC-.)
>
Thanks for clarifying this.
>
>> Current code pciehp_handle_presence_or_link_change() handle link down event,
>> then check the card present, but at this time presence detect state have
>> not updated, so have such issue. If surprise removal and power controller
>> present is not implemented, wait for at least 1 second before checking
>> card present to fix the issue.
>
> Thunderbolt doesn't have a Power Controller either and this change would
> imply that if Thunderbolt devices are quickly swapped, bringing up the
> replaced device would take longer than it takes now.  So I'm not really
> happy about this change.

I got your point, any suggestion to resolve the message problem ?

Thanks,
Dongdong
>
> Thanks,
>
> Lukas
>
>>
>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>> ---
>>  drivers/pci/hotplug/pciehp_ctrl.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
>> index 3f3df4c..ef8952d 100644
>> --- a/drivers/pci/hotplug/pciehp_ctrl.c
>> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
>> @@ -216,6 +216,7 @@ void pciehp_handle_disable_request(struct controller *ctrl)
>>  void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
>>  {
>>  	bool present, link_active;
>> +	bool safe_removal = SAFE_REMOVAL;
>>
>>  	/*
>>  	 * If the slot is on and presence or link has changed, turn it off.
>> @@ -236,6 +237,7 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
>>  			ctrl_info(ctrl, "Slot(%s): Card not present\n",
>>  				  slot_name(ctrl));
>>  		pciehp_disable_slot(ctrl, SURPRISE_REMOVAL);
>> +		safe_removal = SURPRISE_REMOVAL;
>>  		break;
>>  	default:
>>  		mutex_unlock(&ctrl->state_lock);
>> @@ -244,6 +246,15 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
>>
>>  	/* Turn the slot on if it's occupied or link is up */
>>  	mutex_lock(&ctrl->state_lock);
>> +	/*
>> +	 * if surprise removal and power controller present is not implemented,
>> +	 * 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).
>> +	 */
>> +	if (!safe_removal && !POWER_CTRL(ctrl))
>> +		msleep(1000);
>> +
>>  	present = pciehp_card_present(ctrl);
>>  	link_active = pciehp_check_link_active(ctrl);
>>  	if (!present && !link_active) {
>> --
>> 1.9.1
>
> .
>
Lukas Wunner Jan. 17, 2019, 6:30 p.m. | #3
On Thu, Jan 17, 2019 at 08:07:13PM +0800, Dongdong Liu wrote:
> ??? 2019/1/16 22:22, Lukas Wunner ??????:
> > On Wed, Jan 16, 2019 at 10:31:04PM +0800, Dongdong Liu wrote:
> > > The lspci -tv topology is as below.
> > >  +-[0000:80]-+-00.0-[81]----00.0  Huawei Technologies Co., Ltd. Device 3714
> > >  |           +-02.0-[82]----00.0  Huawei Technologies Co., Ltd. Device 3714
> > >  |           +-04.0-[83]----00.0  Huawei Technologies Co., Ltd. Device 3714
> > >  |           +-06.0-[84]----00.0  Huawei Technologies Co., Ltd. Device 3714
> > >  |           +-10.0-[87]----00.0  Huawei Technologies Co., Ltd. Device 3714
> > > 
> > > Then surprise removal 87:00.0 NVME SSD card. The message is as below.
> > > 
> > > pciehp 0000:80:10.0:pcie004: Slot(36): Link Down
> > > iommu: Removing device 0000:87:00.0 from group 12
> > > pciehp 0000:80:10.0:pcie004: Slot(36): Card present
> > > pcieport 0000:80:10.0: Data Link Layer Link Active not set in 1000 msec
> > > pciehp 0000:80:10.0:pcie004: Failed to check link status
> > 
> > What is the problem that you're trying to fix?  That these messages
> > are logged?  Or is there a bigger issue?  If the only problem are the
> > messages, then I feel that the current behavior is a feature, not a bug.
> > We could probably tone down the "Failed to check link status" message's
> > severity.  (Currently it's KERN_ERR, all the other messages are KERN_INFO.)
> 
> Yes, the only problem is the messages, looks not good,
> as the card have been removed from board, the message still show
> card present and failed to check link status.
> Only tone down the "Failed to check link status" message's severity
> seems not good enough.

Well, getting messages like this is par for the course with PCIe hotplug.

E.g. some older Thunderbolt controllers do not support MSI on their
hotplug ports, but only INTx.  If multiple such devices are daisy-
chained, they'll share an interrupt, so whenever a device is hot-removed,
a "pciehp_isr: no response from device" message is logged with
KERN_INFO severity because the hot-removed device was inaccessible
for its interrupt handler.  The interrupt didn't come from the
hot-removed device of course but from another one further upstream
in the daisy-chain where the plug event occurred.  We can't do much
better with such broken hardware.

The reason you're seeing messages is because it takes an unusually
long time for the controller to clear the Presence Detect State bit
after a Data Link Layer State Changed event upon hot-removal.
That's arguably a quirk of the hardware you're dealing with.

pciehp cannot tell whether the Presence Detect State bit is set
because a new card is already present in the slot or if it's trailing
hot-removal and will be cleared shortly.  The protocol doesn't allow
for a clear disambiguation, so pciehp copes by optimistically trying
to bring up the slot, and giving up after a certain delay.

There is other quirky hardware out there which flaps the Presence
Detect State and Data Link Layer Link Active bits a couple of times
before they become stable, which is why pciehp needs to try for a
certain period to bring up the slot.

Again, we could probably tone down or remove some of the messages,
but that might make it harder to diagnose when something really
doesn't work.  It's Bjorn's call anyway.

Thanks,

Lukas

Patch

diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 3f3df4c..ef8952d 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -216,6 +216,7 @@  void pciehp_handle_disable_request(struct controller *ctrl)
 void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
 {
 	bool present, link_active;
+	bool safe_removal = SAFE_REMOVAL;
 
 	/*
 	 * If the slot is on and presence or link has changed, turn it off.
@@ -236,6 +237,7 @@  void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
 			ctrl_info(ctrl, "Slot(%s): Card not present\n",
 				  slot_name(ctrl));
 		pciehp_disable_slot(ctrl, SURPRISE_REMOVAL);
+		safe_removal = SURPRISE_REMOVAL;
 		break;
 	default:
 		mutex_unlock(&ctrl->state_lock);
@@ -244,6 +246,15 @@  void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
 
 	/* Turn the slot on if it's occupied or link is up */
 	mutex_lock(&ctrl->state_lock);
+	/*
+	 * if surprise removal and power controller present is not implemented,
+	 * 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).
+	 */
+	if (!safe_removal && !POWER_CTRL(ctrl))
+		msleep(1000);
+
 	present = pciehp_card_present(ctrl);
 	link_active = pciehp_check_link_active(ctrl);
 	if (!present && !link_active) {