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: 2580691 Return-Path: X-Original-To: patchwork-linux-wireless@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork1.kernel.org (Postfix) with ESMTP id 7FA9C40079 for ; Thu, 16 May 2013 22:55:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754976Ab3EPWzm (ORCPT ); Thu, 16 May 2013 18:55:42 -0400 Received: from mail-ob0-f171.google.com ([209.85.214.171]:65278 "EHLO mail-ob0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754468Ab3EPWzk (ORCPT ); Thu, 16 May 2013 18:55:40 -0400 Received: by mail-ob0-f171.google.com with SMTP id ef5so4056694obb.30 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=pP8gb6q5DTN131hVpLL/Er0sPmN7/3rDrKiEmS5EnfQvGouipwWMKdJU14KumQsWwf L0Lc5v/TwSYku/iInmaFaTmjkgymNe2BZkJMU4NQJZomVFPlO03jTrZ+dXqGsCWcUjvp 0OZwk/TDM6m8FUNxZ5gRLWigqjdXlq4kJUX1HhHq39ui1s2PwR+FyYIE5Rzl4oVybSht +/OfucU4gb/qG+xllNF39GPaRE4Vbxhxxw1QTv39xc1j1GHZGOb/GNxwI8OSS0SfMpxN a8Q2VMW4utqCf7DkwBDXcAmLtVZptnaCWiaVl4igJVh5SbxeJ2fP8b7o6OM9urnDYeUx W4Bw== 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: ALoCoQnLbppM4VWudDE4SOO3dGOd/QNccYXeTP1Fb/sJ82w9iJ/Me6k7d7/qf82FiwBwIRyv7Dz61X5o0b/B8GJSn7oq8gi+WSGB48SCl/Q7TuGdE7q584AIFg9YUXGaCIwNfb9khlZKBBYV6/89NJlqhO4Tkw/QcT5zc8ey6tzD6qG7j7S7M0n+CnVpa8oRmuMfzghqxWfC56eSCMvohOHQe0DRcblpOA== Sender: linux-wireless-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-wireless@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-wireless" 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);