linux-kernel - Re: [PATCH] pci: check PCI_EXP_FLAGS_SLOT before setting hotplug bridge

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: <CAErSpo45HV27k-ULgj4JFzEWqDC+o9dH11qVwO21YfYBFGP9Mw@mail.gmail.com>
Date:	Tue, 19 Nov 2013 10:40:09 -0700
From:	Bjorn Helgaas <bhelgaas@...gle.com>
To:	Adam Lee <adam.lee@...onical.com>
Cc:	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	open list <linux-kernel@...r.kernel.org>,
	Amos Kong <kongjianjun@...il.com>,
	Myron Stowe <myron.stowe@...hat.com>,
	Thomas Renninger <trenn@...e.de>,
	Ben Hutchings <ben@...adent.org.uk>
Subject: Re: [PATCH] pci: check PCI_EXP_FLAGS_SLOT before setting hotplug bridge

On Mon, Nov 18, 2013 at 10:57 PM, Adam Lee <adam.lee@...onical.com> wrote:
> On Mon, Nov 18, 2013 at 10:38:17AM -0700, Bjorn Helgaas wrote:
>> [+cc Myron, Amos, Thomas, Ben]
>>
>> On Mon, Nov 18, 2013 at 2:40 AM, Adam Lee <adam.lee@...onical.com> wrote:
>> > This patch adds the PCI_EXP_FLAGS_SLOT check back before setting
>> > hotplug bridge, which is omitted by an API switching commit,
>> > 59875ae489609b2267548dc85160c5f0f0c6f9d4 "PCI/core: Use PCI Express
>> > Capability accessors".
>> >
>> > Some Lenovo laptops hang in booting without this fix.
>>
>> What kernel version hangs?  I suspect you might be missing 6d3a1741f1
>> ("PCI: Support PCIe Capability Slot registers only for ports with
>> slots"), because it *looks* like the current kernel should work
>> correctly even without your patch.
>
> No, patching 6d3a1741f1 and d3694d4fa3 doesn't fix the hang.
>
> It hangs in acpi_evaluate_integer() from
> 59875ae489609b2267548dc85160c5f0f0c6f9d4 "PCI/core: Use PCI Express
> Capability accessors" and before
> ac212b6980d8d5eda705864fc5a8ecddc6d6eacc "ACPI / processor: Use common
> hotplug infrastructure", 3.4~3.11. (double confirmed)

I don't understand what you're saying here.  We're talking about this path:

    set_pcie_hotplug_bridge
      pcie_capability_read_dword(dev, PCI_EXP_SLTCAP, ...)
        pcie_capability_reg_implemented(dev, PCI_EXP_SLTCAP)
          pcie_cap_has_sltctl(dev)

This path doesn't invoke acpi_evaluate_integer().  How does a hang
there relate to the patch you posted?  Are you saying you bisected the
hang to one of the commits you mentioned?

Your patch appears to be functionally equivalent to 6d3a1741f1 -- they
both check the PCI_EXP_FLAGS_SLOT bit in the PCI_EXP_FLAGS word.  But
6d3a1741f1 uses a cached copy of PCI_EXP_FLAGS, so it could be that
you found a path where the cached copy hasn't been updated yet.

I don't want to apply a patch that is logically unnecessary, because
then it's likely that somebody in the future will think "there's no
need to check PCI_EXP_FLAGS_SLOT twice, so I'll remove the second
one," which will break things again.  Your patch appears unnecessary,
so obviously I'm missing something.  I'm just trying to understand
what I'm missing.

Bjorn
--
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/