From patchwork Thu Aug 29 17:47:13 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bjorn Helgaas X-Patchwork-Id: 2851525 Return-Path: X-Original-To: patchwork-linux-acpi@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 637DCC0AD3 for ; Thu, 29 Aug 2013 17:47:27 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id D6FB1204B4 for ; Thu, 29 Aug 2013 17:47:22 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 542782049C for ; Thu, 29 Aug 2013 17:47:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754517Ab3H2RrU (ORCPT ); Thu, 29 Aug 2013 13:47:20 -0400 Received: from mail-yh0-f48.google.com ([209.85.213.48]:40473 "EHLO mail-yh0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753756Ab3H2RrT (ORCPT ); Thu, 29 Aug 2013 13:47:19 -0400 Received: by mail-yh0-f48.google.com with SMTP id i57so191727yha.35 for ; Thu, 29 Aug 2013 10:47:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=mBweoRpDz/nxv2PNMuXkDd+0LWP6KqnoG5KwGl4o7hc=; b=HztrB3pa7q2lybJ56gi+fqZHfrT2aZDNyQ/7vAfRN3Qsct9593nHKUqcfgAlZxMvi3 5wxfZbPQDlKa479KSkLI4FXKPaEH05E9hZ22x26ebkBeCf4T3avrE0M4MRg7SinfaGVL OocvjKSvpQgcZ6m8L0jRZiLmNYDOrNt63ESEVFaD8FwM0uwDnCVmf4YTG+qb0zmL+65e W2DNkT6/MP87JCjoYCHLFhL7bbE+KgESdSqYuUg8wZ6rUd+eVm2OMOA2UYfy5ppAdZyo mz37rqvzmhtR9drHtBF8pifCE6prvhPLx2oZzjv2fO0ocfMa9UWon+Kzj47YJVhRLVow GPGw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=mBweoRpDz/nxv2PNMuXkDd+0LWP6KqnoG5KwGl4o7hc=; b=WFXocS64a5rNXi/ws2kyB2z8V4BB5w2jelsyqJtcy7PQqzy5MJATrSoQOQ8e35KRKJ bsFrIWFMm6LuIHrI98d4PpG+0aDwupTvok+VE6kn6okAMhr7lQDKCJvcCS/3ZVsDxtKn 0qFo1+QKpznNpkCkK9Z6re53D52DrfU/4OH8cY0Pb496YDoGtIt0VDUwOpeGPpLxkrQa ntbH+pYY/MHoNtda1bjtmR1xp/gYNPFxSgtTOIcPh60vVy7kAddTKnsUU3sBJYE+JiWZ Ol7rFKFNHjWtLen3FSAk3RWCTx6vqwRpsuSagNihmvCbvCQdNr2G2s6Bfz9C5P2MP3v8 8WHw== X-Gm-Message-State: ALoCoQmFHlEkQXZ0Wjd3J2M19Kj4092BUfhkgGVi0+Ai1Nu90KioaZ/rkuCkgaHCCmrsvru9ta2Tt1cf2bh04CJ0y6SGOPkowyjE7YziGudrQnHRglbQdzLA2DPc6FSy11wgr1QN2pwRk1lrib52aDYA9CzJ3dk40VvKXuO44SJxpZ654JK9yas6XQZXr6WtmOX5eVj7L12b9rnmg62Y2autyXuOE7Qjqw== X-Received: by 10.236.81.50 with SMTP id l38mr4091987yhe.29.1377798437445; Thu, 29 Aug 2013 10:47:17 -0700 (PDT) Received: from google.com ([172.16.52.97]) by mx.google.com with ESMTPSA id o25sm39819254yhb.25.1969.12.31.16.00.00 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Thu, 29 Aug 2013 10:47:16 -0700 (PDT) Date: Thu, 29 Aug 2013 11:47:13 -0600 From: Bjorn Helgaas To: Neil Horman Cc: linux-kernel@vger.kernel.org, Len Brown , "Rafael J. Wysocki" , linux-acpi@vger.kernel.org, linux-pci@vger.kernel.org Subject: Re: [PATCH v3] ACPI: Fix osc flag setup ordering to allow pcie hotplug use when available Message-ID: <20130829174713.GA6489@google.com> References: <1377278379-9054-1-git-send-email-nhorman@tuxdriver.com> <1377531553-17716-1-git-send-email-nhorman@tuxdriver.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1377531553-17716-1-git-send-email-nhorman@tuxdriver.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-acpi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org X-Spam-Status: No, score=-9.3 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, T_DKIM_INVALID, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Mon, Aug 26, 2013 at 11:39:13AM -0400, Neil Horman wrote: > Somewhere between 3.9 and 3.10 it seems the order in which pcie and acpi probed > slots for hotplug capabilites got reversed. While this isn't a big deal, it did > uncover a bug in the ACPI bus setup path. Specifically, acpi_pci_root_add calls > pci_acpi_scan_root before setting the osc flags for the device handle. > pci_acpi_scan_root, among other things uses device_is_managed_by_native_pciehp() > to determine if a given slot has pcie hotplug capabilties, whcih checks the > devices OSC_PCI_EXPRESS_NATIVE_HP_CONTROL flag. Since that flag is not set > until after pci_acpi_scan_root_completes, the acpi code never sees that pcie > slots are hotplug capable and configures them all to use acpi instead. > > Fix is pretty simple, just defer the scan until after the osc flags have been > set on the device. Tested by myself and it seems to work well. > > Signed-off-by: Neil Horman > CC: Len Brown > CC: "Rafael J. Wysocki" > CC: Bjorn Helgaas > CC: linux-acpi@vger.kernel.org > CC: linux-pci@vger.kernel.org > > --- > Change notes: > v2) eferred the disabling of aspm until after the acpi scan of the pci bus is > complete. This was done to allow proper handling of pcie 1.1 devices, as per: > > commit b8178f130e25c1bdac1c33e0996f1ff6e20ec08e > Author: Bjorn Helgaas > Date: Mon Apr 1 15:47:39 2013 -0600 > > Revert "PCI/ACPI: Request _OSC control before scanning PCI root bus" > > As discussed previously in the thread the disable logic for aspm needs to be > untangled and refactored, which is not something I'm sufficently versed in teh > hotplug code to do right now, but this fixes the problem above, and prevents the > problem that necessitated the revert without adding any visible complexity to > the user, so I think its ok. > > v3) Fixup stupid authorship error > --- > drivers/acpi/pci_root.c | 54 +++++++++++++++++++++++++++++-------------------- > 1 file changed, 32 insertions(+), 22 deletions(-) > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index 5917839..1e80a90 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -378,6 +378,7 @@ static int acpi_pci_root_add(struct acpi_device *device, > struct acpi_pci_root *root; > u32 flags, base_flags; > acpi_handle handle = device->handle; > + bool no_aspm = false; > > root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL); > if (!root) > @@ -437,27 +438,6 @@ static int acpi_pci_root_add(struct acpi_device *device, > flags = base_flags = OSC_PCI_SEGMENT_GROUPS_SUPPORT; > acpi_pci_osc_support(root, flags); > > - /* > - * TBD: Need PCI interface for enumeration/configuration of roots. > - */ > - > - /* > - * Scan the Root Bridge > - * -------------------- > - * Must do this prior to any attempt to bind the root device, as the > - * PCI namespace does not get created until this call is made (and > - * thus the root bridge's pci_dev does not exist). > - */ > - root->bus = pci_acpi_scan_root(root); > - if (!root->bus) { > - dev_err(&device->dev, > - "Bus %04x:%02x not present in PCI namespace\n", > - root->segment, (unsigned int)root->secondary.start); > - result = -ENODEV; > - goto end; > - } > - > - /* Indicate support for various _OSC capabilities. */ > if (pci_ext_cfg_avail()) > flags |= OSC_EXT_PCI_CONFIG_SUPPORT; > if (pcie_aspm_support_enabled()) { > @@ -512,7 +492,14 @@ static int acpi_pci_root_add(struct acpi_device *device, > acpi_format_exception(status), flags); > dev_info(&device->dev, > "ACPI _OSC control for PCIe not granted, disabling ASPM\n"); > - pcie_no_aspm(); > + /* > + * We want to disable aspm here, but aspm_disabled > + * needs to remain in its state from boot so that we > + * properly handle pcie 1.1 devices. So we set this > + * flag here, to defer the action until after the acpi > + * root scan > + */ > + no_aspm = true; > } > } else { > dev_info(&device->dev, > @@ -520,6 +507,29 @@ static int acpi_pci_root_add(struct acpi_device *device, > "(_OSC support mask: 0x%02x)\n", flags); > } > > + /* > + * TBD: Need PCI interface for enumeration/configuration of roots. > + */ > + > + /* > + * Scan the Root Bridge > + * -------------------- > + * Must do this prior to any attempt to bind the root device, as the > + * PCI namespace does not get created until this call is made (and > + * thus the root bridge's pci_dev does not exist). > + */ > + root->bus = pci_acpi_scan_root(root); > + if (!root->bus) { > + dev_err(&device->dev, > + "Bus %04x:%02x not present in PCI namespace\n", > + root->segment, (unsigned int)root->secondary.start); > + result = -ENODEV; > + goto end; > + } > + > + if (no_aspm) > + pcie_no_aspm(); > + > pci_acpi_add_bus_pm_notifier(device, root->bus); > if (device->wakeup.flags.run_wake) > device_set_run_wake(root->bus->bridge, true); I think there are two problems with this patch: 1) There's another call of pcie_no_aspm() at line 454 (in the error path when acpi_pci_osc_support() fails), which happens before scanning the bus. I think this needs to be converted to "no_aspm = true" as you did for the one in the error path when acpi_pci_osc_control_set() fails. 2) You effectively moved the call of "pcie_clear_aspm(root->bus)" so it now happens before scanning the bus, which will cause a NULL pointer dereference if we take that path. I think we need something like the patch below on top of your v3 patch. Can you take a look and see if this makes sense, and if so, update your patch to include these or similar fixes? Here are my notes about where the ASPM and root->osc_control_set setting and testing happen. Before your patch: acpi_pci_root_add root = kzalloc # root->osc_control_set = 0 acpi_pci_osc_support # indicate OS support for segments root->bus = pci_acpi_scan_root # SCAN BUS pci_create_root_bus pcibios_add_bus acpi_pci_add_bus acpiphp_enumerate_slots acpi_walk_namespace(..., register_slot, ...) register_slot device_is_managed_by_native_pciehp osc_control_set> # root->osc_control_set == 0 return if OS has PCIe hotplug control (we never do) acpiphp_register_hotplug_slot (so we always do this) acpi_pci_osc_support # indicate OS support for MSI, ASPM, etc if (_osc_support() failed) pci_no_aspm acpi_pci_osc_control_set # request OS control of hotplug, PME, AER, etc root->osc_control_set = XX if (_osc_control_set() succeeded) { if (FADT NO_ASPM bit) pcie_clear_aspm list_for_each_entry(..., &bus->devices, ...) } else pcie_no_aspm # _osc_control_set() failed After your patch: acpi_pci_root_add root = kzalloc # root->osc_control_set = 0 acpi_pci_osc_support # indicate OS support for segments if (_osc_support() failed) pci_no_aspm # ** (1) before bus scan acpi_pci_osc_support # indicate OS support for MSI, ASPM, etc acpi_pci_osc_control_set # request OS control of hotplug, PME, AER, etc root->osc_control_set = XX if (_osc_control_set() succeeded) { if (FADT NO_ASPM bit) pcie_clear_aspm(root->bus) # ** (2) root->bus == NULL list_for_each_entry(..., &bus->devices, ...) } else no_aspm = true # _osc_control_set() failed root->bus = pci_acpi_scan_root # SCAN BUS pci_create_root_bus pcibios_add_bus acpi_pci_add_bus acpiphp_enumerate_slots acpi_walk_namespace(..., register_slot, ...) register_slot device_is_managed_by_native_pciehp osc_control_set> return if OS has PCIe hotplug control acpiphp_register_hotplug_slot if (no_aspm) pcie_no_aspm Bjorn --- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index a37a372..a67853e 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -378,7 +378,7 @@ static int acpi_pci_root_add(struct acpi_device *device, struct acpi_pci_root *root; u32 flags, base_flags; acpi_handle handle = device->handle; - bool no_aspm = false; + bool no_aspm = false, clear_aspm = false; root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL); if (!root) @@ -451,7 +451,7 @@ static int acpi_pci_root_add(struct acpi_device *device, if (ACPI_FAILURE(status)) { dev_info(&device->dev, "ACPI _OSC support " "notification failed, disabling PCIe ASPM\n"); - pcie_no_aspm(); + no_aspm = true; flags = base_flags; } } @@ -483,7 +483,7 @@ static int acpi_pci_root_add(struct acpi_device *device, * We have ASPM control, but the FADT indicates * that it's unsupported. Clear it. */ - pcie_clear_aspm(root->bus); + clear_aspm = true; } } else { dev_info(&device->dev, @@ -527,6 +527,10 @@ static int acpi_pci_root_add(struct acpi_device *device, goto end; } + if (clear_aspm) { + dev_info(&device->dev, "Disabling ASPM (FADT indicates it is unsupported)\n"); + pcie_clear_aspm(root->bus); + } if (no_aspm) pcie_no_aspm();