linux-kernel - Re: [PATCH v5 2/5] PCI: Check PCI_PM_CTRL instead of PCI_COMMAND in pci_dev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <dabbd8fa-f5ec-46fe-994b-695058195d47@amd.com>
Date: Fri, 13 Sep 2024 15:56:46 -0500
From: Mario Limonciello <mario.limonciello@....com>
To: Mika Westerberg <mika.westerberg@...ux.intel.com>,
 Bjorn Helgaas <helgaas@...nel.org>
Cc: Gary Li <Gary.Li@....com>, Mario Limonciello <superm1@...nel.org>,
 Bjorn Helgaas <bhelgaas@...gle.com>, Mathias Nyman
 <mathias.nyman@...el.com>,
 "open list : PCI SUBSYSTEM" <linux-pci@...r.kernel.org>,
 open list <linux-kernel@...r.kernel.org>,
 "open list : USB XHCI DRIVER" <linux-usb@...r.kernel.org>,
 Daniel Drake <drake@...lessos.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Subject: Re: [PATCH v5 2/5] PCI: Check PCI_PM_CTRL instead of PCI_COMMAND in
 pci_dev_wait()

On 9/13/2024 02:23, Mika Westerberg wrote:
> Hi again,
> 
> On Fri, Sep 13, 2024 at 07:58:07AM +0300, Mika Westerberg wrote:
>> Yeah, I agree now. It does not look like the methods are messing each
>> other here. We don't see the GPE handler being run but I don't think it
>> matters here. For some reason the device just is not yet ready when it
>> is supposed to be in D0.
>>
>> Sorry for wasting your time with these suspects.

Don't worry about it.  It validates that we're coming to the right 
conclusions about where the problem is.

> 
> One more suggestion though ;-) I realized that my hack patch to disable
> I/O and MMIO did not actually do that because it looks like we don't
> clear those bits ever. I wonder if you could still check if the below
> changes anything? At least it should now "disable" the device to follow
> the spec.

This actually causes the system to fail to boot.  I guess some deadlock 
from other callers to pci_disable_device().

We also double checked putting the PCI_COMMAND writes just into the 
runtime suspend call backs instead (to narrow down if that is part of 
the issue here).  Putting it there fixed the boot hang, but no change to 
the actual issue behavior.

Bjorn,

Considering this finding, are you more amenable to the polling approach?

> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index f412ef73a6e4..79a566376301 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1332,6 +1332,7 @@ static int pci_pm_runtime_suspend(struct device *dev)
>   
>   	if (!pci_dev->state_saved) {
>   		pci_save_state(pci_dev);
> +		pci_pm_default_suspend(pci_dev);
>   		pci_finish_runtime_suspend(pci_dev);
>   	}
>   
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index ffaaca0978cb..91f4e7a03c94 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2218,6 +2218,13 @@ static void do_pci_disable_device(struct pci_dev *dev)
>   		pci_command &= ~PCI_COMMAND_MASTER;
>   		pci_write_config_word(dev, PCI_COMMAND, pci_command);
>   	}
> +	/*
> +	 * PCI PM 1.2 sec 8.2.2 says that when a function is put into D3
> +	 * the OS needs to disable I/O and MMIO space in addition to bus
> +	 * mastering so do that here.
> +	 */
> +	pci_command &= ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY);
> +	pci_write_config_word(dev, PCI_COMMAND, pci_command);
>   
>   	pcibios_disable_device(dev);
>   }