From patchwork Thu May 16 22:55:35 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bjorn Helgaas X-Patchwork-Id: 2580671 X-Patchwork-Delegate: bhelgaas@google.com Return-Path: X-Original-To: patchwork-linux-pci@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id DEA36DFE75 for ; Thu, 16 May 2013 22:55:42 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754621Ab3EPWzl (ORCPT ); Thu, 16 May 2013 18:55:41 -0400 Received: from mail-ob0-f174.google.com ([209.85.214.174]:44252 "EHLO mail-ob0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751185Ab3EPWzj (ORCPT ); Thu, 16 May 2013 18:55:39 -0400 Received: by mail-ob0-f174.google.com with SMTP id un3so4043186obb.5 for ; Thu, 16 May 2013 15:55:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-received:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=mefG9WK6hy50tQanNANWY2Xm7X+yB7H50g0bDLfxD6Q=; b=aj8b7R3A4GhBni65Nk/8RELQdnc8jTQWPyR/zHDR9iyqPokCNhkFqxV9CH5+WZIkWs fYAXg7uuEdMEtJhBv2P472VnRzRY3s8DTVkXQuHwKWLRviJf7+DNX97UR+qeowuHBeb/ Nv/x0MTBZw+DXEYcJ3GFYZvLyt1anJgCYEZ9eh5CgvZ/oo5N8l58k384H+DH8dwyyp3u wIHuKcIHPwYzFBPyVjSZBy3Is6NTEBxvg3snu0KamZoti37qfOZBiQ1lCtaWq9JEZJj/ meTr9u7cwuCK1MbHlEfTNxl9vTK0gmmXoqN0iVmnHY5S5vG4ThIOWbIjIzWNL7Z171M+ AuJg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-received:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-disposition:in-reply-to :user-agent:x-gm-message-state; bh=mefG9WK6hy50tQanNANWY2Xm7X+yB7H50g0bDLfxD6Q=; b=jDOnIWFcYKQyxcwNv7TUXig2c5DDBaJiI9r3h+5L8uAZ0U1hTQK2Yq93gQLQhNlOq9 EXqWdcTfMxPRgk2zj+7S7FwA32lSKJy1jl6GtP9//GLvng+S8F4z64JotQXGIZmO7aeK Cmjq6rbBauw6UZYYTYVVEJmqwlOx4nX/0t3kAEEUiOqFKuxU3+K+I1Nz4NLOZrrzBINa JRNdhO7nbe/nNlz7LU4ZpK7WNOSLDVWoEKX/nN5pGpMcEpF0c409714oJ87OtNw+hyI3 BXRtIc9wuLynE3HFDNBgpcfx494HUJkbCxwlUWMvtDyjKPH/Twm8M6fvndcpDWWikit5 5HxQ== X-Received: by 10.60.47.1 with SMTP id z1mr23385732oem.134.1368744939119; Thu, 16 May 2013 15:55:39 -0700 (PDT) Received: from google.com ([172.16.49.227]) by mx.google.com with ESMTPSA id x5sm9360487oep.1.2013.05.16.15.55.37 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Thu, 16 May 2013 15:55:38 -0700 (PDT) Date: Thu, 16 May 2013 16:55:35 -0600 From: Bjorn Helgaas To: Matthew Garrett Cc: "Rafael J. Wysocki" , Emmanuel Grumbach , Stanislaw Gruszka , "linux-pci@vger.kernel.org" , linux-wireless , John Linville , Roman Yepishev , "Guy, Wey-Yi" , Mike Miller , "iss_storagedev@hp.com" , Guo-Fu Tseng , "netdev@vger.kernel.org" , Francois Romieu , "nic_swsd@realtek.com" , "aacraid@adaptec.com" , "linux-kernel@vger.kernel.org" Subject: Re: is L1 really disabled in iwlwifi Message-ID: <20130516225535.GA27962@google.com> References: <20130510225257.GA10847@google.com> <1725435.3DlCxYF2FV@vostro.rjw.lan> <1368303730.2425.47.camel@x230> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1368303730.2425.47.camel@x230> User-Agent: Mutt/1.5.21 (2010-09-15) X-Gm-Message-State: ALoCoQmQtLEatGdt7m0pxc++dLimsEmzys4Ghy8jJ4yxXqweTUIfVaoHgAM0l7wWnRg5l+q9Q7is+LDN/Pm3qpfhKx8yzWBIQMJ4JWibOZR/WdB1me9iohOAjgZgTHHkka+X0SL9+7Yd2WJXC+c1HngbcgdgWaeRCR7O7Y/LRbn6nawF9H4yCvXisKWgDKiLxWhzdcR9UhfD+i13EtFxxji2DcGYYbL7HA== Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org On Sat, May 11, 2013 at 08:22:11PM +0000, Matthew Garrett wrote: > On Sat, 2013-05-11 at 22:26 +0200, Rafael J. Wysocki wrote: > > On Friday, May 10, 2013 04:52:57 PM Bjorn Helgaas wrote: > > > I propose the following patch. Any comments? > > > > In my opinion this is dangerous, because it opens us to bugs that right now > > are prevented from happening due to the way the code works. > > Right, I'm also not entirely comfortable with this. The current > behaviour may be confusing, but we could reduce that by renaming the > functions. I'm still not clear on whether anyone's actually seeing > problems caused by the existing behaviour. I couldn't imagine that silently ignoring the request to disable ASPM would be the right thing, but I spent a long time experimenting with Windows on qemu, and I think you're right. Windows 7 also seems to ignore the "PciASPMOptOut" directive when we don't have permission to manage ASPM. All the gory details are at https://bugzilla.kernel.org/show_bug.cgi?id=57331 The current behavior is definitely confusing. I hate to rename or change pci_disable_link_state() because it's exported and we'd have to maintain the old interface for a while anyway. And I don't really want to return failure to drivers, because I think that would encourage people to fiddle with the Link Control register directly in the driver, which doesn't seem like a good idea. And you're also right that (as far as I know) there's not an actual problem with the current behavior other than the confusion it causes. So, how about something like the following patch, which just prints a warning when we can't do what the driver requested? I suppose this may also be a nuisance, because users will be worried, but they can't actually *do* anything about it. Maybe it should be dev_info() instead. commit f1956960fa0759c53b28e3a2810bd7e1b6e8925f Author: Bjorn Helgaas Date: Wed May 15 17:02:37 2013 -0600 PCI/ASPM: Warn when driver asks to disable ASPM, but we can't do it Some devices have hardware problems related to using ASPM. Drivers for these devices use pci_disable_link_state() to prevent their device from entering L0s or L1. But on platforms where the OS doesn't have permission to manage ASPM, pci_disable_link_state() doesn't actually disable ASPM. Windows has a similar mechanism ("PciASPMOptOut"), and when the OS doesn't have control of ASPM, it doesn't actually disable ASPM either. This patch just adds a warning in dmesg about the fact that pci_disable_link_state() is doing nothing. Reported-by: Emmanuel Grumbach Reference: https://lkml.kernel.org/r/CANUX_P3F5YhbZX3WGU-j1AGpbXb_T9Bis2ErhvKkFMtDvzatVQ@mail.gmail.com Reference: https://bugzilla.kernel.org/show_bug.cgi?id=57331 Signed-off-by: Bjorn Helgaas --- To unsubscribe from this list: send the line "unsubscribe linux-pci" 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/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index d320df6..faa83b6 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -724,9 +724,6 @@ static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem, struct pci_dev *parent = pdev->bus->self; struct pcie_link_state *link; - if (aspm_disabled && !force) - return; - if (!pci_is_pcie(pdev)) return; @@ -736,6 +733,19 @@ static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem, if (!parent || !parent->link_state) return; + /* + * A driver requested that ASPM be disabled on this device, but + * if we don't have permission to manage ASPM (e.g., on ACPI + * systems we have to observe the FADT ACPI_FADT_NO_ASPM bit and + * the _OSC method), we can't honor that request. Windows has + * a similar mechanism using "PciASPMOptOut", which is also + * ignored in this situation. + */ + if (aspm_disabled && !force) { + dev_warn(&pdev->dev, "can't disable ASPM; OS doesn't have ASPM control\n"); + return; + } + if (sem) down_read(&pci_bus_sem); mutex_lock(&aspm_lock);