Regression (sort of): PCI/portdrv: Turn off PCIe services during shutdown

Hi Sinan,

I updated to 4.15 this week and noticed a regression (if one may call it that, it's more like a very noticeable annoyance) affecting machines with Thunderbolt controllers wherein a reboot or shutdown is delayed by 8 seconds, caused by

commit cc27b735ad3a75574a6ab1a66ed6b09385e77e5e
Author: Sinan Kaya <[email protected]>
Date: Wed Oct 25 15:01:02 2017 -0400

PCI/portdrv: Turn off PCIe services during shutdown

Some of the PCIe services such as AER are being left enabled during shutdown. This might cause spurious AER errors while SOC is being powered down.

Clean up the PCIe services gracefully during shutdown to clear these false positives.

The reason is that Thunderbolt controllers claim to support Command Complete interrupts (value of 0b in the No Command Completed Support field of the Slot Capabilities register), but in reality never bother to send the interrupt. Your commit causes pcie_shutdown_notification() to be called on ->shutdown, which calls pcie_write_cmd(), which calls pcie_wait_cmd() twice, each invocation causing a delay of 1 second. Because Thunderbolt controllers typically have 4 hotplug bridges, this adds up to 8 seconds.

The 1 second delay comes from PCIe base spec r2.1 sec. 6.7.3.2:

If command completed events are supported, then software must wait for a command to complete before issuing the next command. However, if the status field is not set after the 1 second limit on command execution, software is permitted to repeat the command or to issue the next command.

I'm not sure which Thunderbolt controllers fake the ability to send CC interrupts but my old Light Ridge is definitely affected and I recall seeing logs of newer Cactus Ridge and Falcon Ridge (Thunderbolt 2) controllers which also had this issue. I'm cc'ing Intel folks in the hope that they may know the details. I believe Mika has a MacBookPro13,3 with Alpine Ridge (Thunderbolt 3) and could verify if that also has this issue. These MacBook Pros have two Thunderbolt controllers, so if affected, the commit would delay reboot by 16 seconds.

On the one hand, your commit message sounds as though the change merely addresses a *potential* issue, so one could argue that if it causes *real* issues such as delayed reboots, it's probably not a good idea. On the other hand I do see your point since a user might surprise-remove a device from a hotplug slot during shutdown, and we clearly wouldn't want to act on that.

The Thunderbolt hotplug slots aren't "real" slots that you insert cards into, but rather "virtual" hotplug slots implemented in silicon. Devices appear beyond those slots once a PCI tunnel is established on top of the Thunderbolt switching infrastructure. So I'm wondering if we have to wait for command completion at all in this case. We could add a bit to struct pci_dev_flags indicating that command completion need not be awaited, and bail out of pcie_wait_cmd() if the bit is set. The bit could be set in set_pcie_hotplug_bridge() or in a PCI quirk.

However I'm not sure if it would be safe. Maybe one of the Intel devs can comment on whether any delay must be observed after issuing a command to the Slot Control Register on Thunderbolt controllers? Surely 1 second seems excessive.

Thanks,

Lukas

I managed to dig up lspci output for a MacBookPro13,3 and Alpine Ridge has 1b in the No Command Completed Support field of the Slot Capabilities register, so no delay is observed at all and these newer machines are not affected.

Question is, do the older controllers likewise not need a delay?

Lukas

Hi Lukas,

Sorry for that.

I wonder if we can separate remove from shutdown and just disable the IRQs in shutdown case rather than turning off the slot power etc.

Let me see if I can come up with a quick patch.

We are seeing the problem on our platform (QDF2400). It is just sporadic. That's why, the commit message was loosely worded.

Sinan

Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

But don't we risk "IRQ xx: nobody cared" splats if we do that?

Not sure if just disabling the IRQ without telling the hotplug port not to send interrupts is safe.

The issue that Command Complete events are falsely claimed to be supported needs to be adressed regardless because whenever a Thunderbolt device is unplugged, the same 2 second delay occurs. So if you unplug a daisy chain of, say 6 devices, it takes 12 seconds until they're all removed from the system.

By now I've found out that Falcon Ridge (Thunderbolt 2) is not affected, I'm not yet sure if Redwood Ridge is, but it seems the issue is contrained to Thunderbolt 1 controllers only, probably an erratum.

Thanks,

Lukas

I assumed code was turning off the slot power etc. aggressively.

After looking at the code some more time, it seems to be doing the right thing and telling pcie controller not to generate interrupts for hotplug.

I think this is what is failing for you probably because by the time you are shutting down there is nobody to issue the command completion. This would repeat for each hotplug capable pcie slot.

static void pcie_disable_notification(struct controller *ctrl)

{
	u16 mask;

	mask = (PCI_EXP_SLTCTL_PDCE | PCI_EXP_SLTCTL_ABPE |
		PCI_EXP_SLTCTL_MRLSCE | PCI_EXP_SLTCTL_PFDE |
		PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_CCIE |
		PCI_EXP_SLTCTL_DLLSCE);
	pcie_write_cmd(ctrl, 0, mask);
	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, 0);
} 

Can you confirm this?

Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

You mean we disable Command Completed interrupts and thus the port can't notify that the command was completed? It seems the code accommodates to that by polling the PCI_EXP_SLTSTA_CC bit:

	if (ctrl->slot_ctrl & PCI_EXP_SLTCTL_HPIE &&
	    ctrl->slot_ctrl & PCI_EXP_SLTCTL_CCIE)
		rc = wait_event_timeout(ctrl->queue, !ctrl->cmd_busy, timeout);
	else
		rc = pcie_poll_cmd(ctrl, jiffies_to_msecs(timeout));

The problem is that these Thunderbolt controllers never seem to set the PCI_EXP_SLTSTA_CC bit, resulting in "Timeout on hotplug command" messages.

The pciehp code is okay, we just need a workaround for the broken Thunderbolt 1 chips. This has been a pain point all along, but your patch made the brokenness visible enough that investigating and fixing it became unavoidable. So don't worry about your patch, it's all fine. ;-)

Thanks,

Lukas

I waa thinking of using nowait variant of write function in notification disable function in order to not introduce new behavior for existing silicon.

After writing to the Slot Control register to turn off IRQ notification, we free the IRQ with pciehp_free_irq(). If we don't wait for the command to complete, we risk getting an interrupt after the IRQ was freed.

Kind regards,

Lukas

Yeah, that is true.

Checked the MacBook with 2 x AR and it does have NoCompl+ for the hotplug downstream ports. Also tried reboot the system and it does not take 16s, more like 1-2s.

Yes, as said Alpine Ridge isn't affected, only older Thunderbolt 1 chips. You mentioned last year that you also have a mac mini with Light Ridge and a MacBook with Cactus Ridge:
http://lkml.iu.edu/hypermail/linux/kernel/1705.3/00443.html

Install a 4.15 kernel on one of those and reboot to see the 8 second delay.

I've just sent out a patch which fixes the issue for me, an ack would be appreciated, barring any objections of course. Do you have access to specs for older Thunderbolt 1 chips and if so, could you verify if they require a delay to be observed after writing to the Slot Control register? Is there an errata sheet confirming that the advertised NoCompl- is bogus?

There are certain Xeons from the same era as the affected Thunderbolt 1 chips which advertise NoCompl- but erroneously only acknowledge Command Completed for a few specific commands:
- Power Controller Control
- Power Indicator Control
- Attention Indicator Control
- Electromechanical Interlock Control

See erratum CF118 in:
https://www.intel.com/content/www/us/en/processors/xeon/xeon-e7-v2-spec-update.html

I figured that maybe Intel recycled the faulty VHDL files of these Xeon hotplug ports in Thunderbolt 1 controllers, so I set all of the above bits on every access to the Slot Control register. Alas it didn't help, the behavior stayed the same, no Command Completed event and no Command Completed indication in the Slot Status register. (Perhaps unsurprising since Light Ridge doesn't advertise support for any of the above four features.)

Thanks,

Lukas