Re: [RFC PATCH] PCI/portdrv: No need to call pci_disable_device() during shutdown

From: Tiezhu Yang
Date: Thu Sep 10 2020 - 23:20:07 EST

On 09/11/2020 10:35 AM, Oliver O'Halloran wrote:
On Fri, Sep 11, 2020 at 11:55 AM Tiezhu Yang <yangtiezhu@xxxxxxxxxxx> wrote:
On 09/11/2020 04:21 AM, Bjorn Helgaas wrote:
[+cc Huacai]

On Thu, Sep 10, 2020 at 02:41:39PM -0500, Bjorn Helgaas wrote:
On Sat, Sep 05, 2020 at 04:33:26PM +0800, Tiezhu Yang wrote:
After commit 745be2e700cd ("PCIe: portdrv: call pci_disable_device
during remove") and commit cc27b735ad3a ("PCI/portdrv: Turn off PCIe
services during shutdown"), it also calls pci_disable_device() during
shutdown, this seems unnecessary, so just remove it.
I would like to get rid of the portdrv completely by folding its
functionality into the PCI core itself, so there would be no portdrv
probe or remove.

Does this solve a problem?
Yes, sometimes it can not shutdown or reboot normally with
Do you have any more details about what goes wrong here?

This issue is related with the operation "pci_command &= ~PCI_COMMAND_MASTER;"
in the following function:

static void do_pci_disable_device(struct pci_dev *dev)
u16 pci_command;

pci_read_config_word(dev, PCI_COMMAND, &pci_command);
if (pci_command & PCI_COMMAND_MASTER) {
pci_command &= ~PCI_COMMAND_MASTER;
pci_write_config_word(dev, PCI_COMMAND, pci_command);


When remove "pci_command &= ~PCI_COMMAND_MASTER;", it can work well.

devices enabled when actually shutting down probably doesn't matter.

Yes, I think so too.

However, .shutdown() is also used when kexec()ing into a new kernel
and we probably should be disabling devices before handing over to the
new kernel.

Is the real issue that we're closing the bridge windows before the
endpoint drivers have had a chance to clean up?

I notice that check kexec_in_progress first before call pci_disable_devie()
in pci_device_shutdown(), can we do the similar thing in pcie?

static void pci_device_shutdown(struct device *dev)
struct pci_dev *pci_dev = to_pci_dev(dev);
struct pci_driver *drv = pci_dev->driver;


if (drv && drv->shutdown)

* If this is a kexec reboot, turn off Bus Master bit on the
* device to tell it to not continue to do DMA. Don't touch
* devices in D3cold or unknown states.
* If it is not a kexec reboot, firmware will hit the PCI
* devices with big hammer and stop their DMA any way.
if (kexec_in_progress && (pci_dev->current_state <= PCI_D3hot))

Something like this:

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 50a9522..3de1dab 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -491,7 +491,8 @@ void pcie_port_device_remove(struct pci_dev *dev)
device_for_each_child(&dev->dev, NULL, remove_iter);
- pci_disable_device(dev);
+ if (kexec_in_progress && (pci_dev->current_state <= PCI_D3hot))
+ pci_disable_device(dev);