linux-kernel - Re: [PATCH] PCI: Remove not needed check in disable aspm link

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]

Date:	Fri, 14 Jun 2013 11:26:49 -0700
From:	Yinghai Lu <yinghai@...nel.org>
To:	Bjorn Helgaas <bhelgaas@...gle.com>,
	Matthew Garrett <matthew.garrett@...ula.com>
Cc:	Jiang Liu <jiang.liu@...wei.com>,
	Roman Yepishev <roman.yepishev@...il.com>,
	"Rafael J. Wysocki" <rjw@...k.pl>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH] PCI: Remove not needed check in disable aspm link

On Fri, Jun 14, 2013 at 10:44 AM, Bjorn Helgaas <bhelgaas@...gle.com> wrote:
> On Fri, Jun 14, 2013 at 10:57 AM, Yinghai Lu <yinghai@...nel.org> wrote:
>> On Fri, Jun 14, 2013 at 9:33 AM, Bjorn Helgaas <bhelgaas@...gle.com> wrote:
>>> On Fri, Jun 14, 2013 at 10:17 AM, Yinghai Lu <yinghai@...nel.org> wrote:
>>>
>>> Can you please refer to specific function names?  I can't read your mind.
>>>
>>> You might be referring to quirk_disable_aspm_l0s().  This is a
>>> pci_fixup_final quirk that calls pci_disable_link_state().  In the
>>> current tree, we enumerate devices before requesting _OSC control.
>>> However, pci_fixup_final quirks are not run until the
>>> pci_apply_final_quirks() fs_initcall, which is after we request _OSC
>>> control.
>>>
>>> As far as I can tell, we never call pci_disable_link_state() before
>>> calling pcie_no_aspm().
>>
>> ok, you are right, that is not pci_disable_link_state.
>>
>> It is pcie_aspm_init_link_state ==> pcie_aspm_sanity_check in booting path
>> that disable aspm.  It has  "if (aspm_disabled)" in it, and it cause
>> the difference.
>
> Yes, I agree, the pcie_aspm_init_link_state() path uses aspm_disabled
> before we set it:
>
>     acpi_pci_root_add
>       pci_acpi_scan_root
>         pci_scan_child_bus
>           pci_scan_slot
>             pcie_aspm_init_link_state
>               pcie_aspm_sanity_check
>                 if (aspm_disabled)              # used before set
>                   ...
>       acpi_pci_osc_control_set
>       pcie_no_aspm
>         aspm_disabled = 1                       # set
>
> That might mean we do some ASPM configuration during enumeration (in
> pci_scan_slot()) even though the BIOS hasn't given us permission.  It
> looks like we did that even in v3.7, since we did the enumeration
> before the _OSC there as well.  That looks like a bug to me.

agreed. that means commits from Matthew Garrett

commit 4949be16822e92a18ea0cc1616319926628092ee
Author: Matthew Garrett <mjg@...hat.com>
Date:   Tue Mar 6 13:41:49 2012 -0500

    PCI: ignore pre-1.1 ASPM quirking when ASPM is disabled

commit c9651e70ad0aa499814817cbf3cc1d0b806ed3a1
Author: Matthew Garrett <mjg@...hat.com>
Date:   Tue Mar 27 10:17:41 2012 -0400

    ASPM: Fix pcie devices with non-pcie children

will only works when the user specify "aspm=off" in boot command line.

(Roman's should have problem when he boot current linus tree with
"aspm=off", as no one will disable aspm for the offending pci devices)

To close the hole that Matthew' commits miss, that we should move _OSC
support/control set ahead.

For Roman's system it will have to fail, as BIOS enable prep-1.1 pcie devices
aspm, and do not handle over control to OS, so os can not disable aspm link
state for it.

To workaround the problem in Roman's system, we can add pcie_aspm=force_off

so we will have
pcie_aspm=off
pcie_aspm=force
pcie_aspm=force_off

What a mess!

>
> I don't think the fact that we have been doing ASPM config during
> enumeration before _OSC is an argument for dropping the check in
> pci_disable_link_state().

Agreed.

Yinghai
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/