From patchwork Fri Jul 11 22:44:21 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bjorn Helgaas X-Patchwork-Id: 4538101 X-Patchwork-Delegate: bhelgaas@google.com Return-Path: X-Original-To: patchwork-linux-pci@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 218DBC0514 for ; Fri, 11 Jul 2014 22:44:31 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 47B1E202A1 for ; Fri, 11 Jul 2014 22:44:30 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 35B5720260 for ; Fri, 11 Jul 2014 22:44:29 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753541AbaGKWo1 (ORCPT ); Fri, 11 Jul 2014 18:44:27 -0400 Received: from mail-ig0-f178.google.com ([209.85.213.178]:56937 "EHLO mail-ig0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750719AbaGKWo0 (ORCPT ); Fri, 11 Jul 2014 18:44:26 -0400 Received: by mail-ig0-f178.google.com with SMTP id uq10so315488igb.11 for ; Fri, 11 Jul 2014 15:44:25 -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=WoBS1vW4EcZLGSXuh0l8WvXrtEhd6tFsOboSjYzKmnA=; b=nrAzJ35+9kBfU8YADfiaZCCUtGihhRhtya1cnZpqfBcMyxnU2RrXfhhPaAIFEhCHkA gA+6M3f48L4hmxxeAbNN67pgkWe9e9DxvuDmS/JIkZmzqEXuO/Zreq4SCl0vcTrceIRs Wh8nO3zNTX8sn5TjnVM5scX6BWx4Ki5VDxByx2LzZShsRB8b73AEPIM3lGo2zAZhbOZt 4p13jGUks/eweKtkiHw+UuMns7QYUhvjbbgJldI1mixWZW3rBjBm5ix5F2dou7q/VvlJ 50qJ+3lXC6+wrauyLXVe9yWXh9PiR680BWYuDr49qdUS/ukqsgTKFzKz6olMg1APyANz 9f4Q== 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=WoBS1vW4EcZLGSXuh0l8WvXrtEhd6tFsOboSjYzKmnA=; b=I6D3+yzayoI/00MTlngxtd2JK1IOtueTZuIxgUPMbKho7Y+rUW1MUCLb8Cc4Ys2MvA vvfXTzKEhSG8NTgC+TH4rq40IE2Hea8Gas/FkrjE/tAFZpcvv0kcq1LCggjTHkCZm0mv w1SPJ+ooDkP/oTwkXYgvg90hyaI6S08q8rYAtgHkZbIoIi51V/fzH0eouql4GMaRwi7O Tv7uYVj0mtp9TR9SZGokwmXtw0BfsmYNeteInVwHsWa1KPAZkSrVY3c9WrFlSpMP+Vl5 hU/wBDRJnZJjTN9cC9pUwXxRXBRguAONzClSSbMrlkGA5tjQ4WAsmRCXbKSOOzOaZ8eH YQkQ== X-Gm-Message-State: ALoCoQlnjEV/3UID/tJfw0sHdbOXz3Gbg5CRnrBjZ1v1K9Wg0EV3LCmTjHpgU1ErYYU/NjoBx8EJ X-Received: by 10.50.25.130 with SMTP id c2mr8310210igg.7.1405118665536; Fri, 11 Jul 2014 15:44:25 -0700 (PDT) Received: from google.com ([172.16.48.57]) by mx.google.com with ESMTPSA id ro10sm9886344igb.18.2014.07.11.15.44.23 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Fri, 11 Jul 2014 15:44:24 -0700 (PDT) Date: Fri, 11 Jul 2014 16:44:21 -0600 From: Bjorn Helgaas To: vidya sagar Cc: Vidya Sagar , "thierry.reding@gmail.com" , Stephen Warren , Krishna Thota , "linux-pci@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Matthew Garrett , "Rafael J. Wysocki" , "tianyu.lan@intel.com" , Nagananda Chumbalkar Subject: Re: [PATCH v1] PCI: enable ASPM configuration in PCIE POWERSAVE mode Message-ID: <20140711224421.GA5417@google.com> References: <1404198978-26593-1-git-send-email-vidyas@nvidia.com> <20140705185736.GD28871@google.com> <20140705190203.GE28871@google.com> <113ACA888B71994BB56E5CF3704953486D8E9DFE3C@BGMAIL02.nvidia.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org X-Spam-Status: No, score=-7.4 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 [updated Naga's email address] On Wed, Jul 09, 2014 at 11:50:01PM +0530, vidya sagar wrote: > On Tue, Jul 8, 2014 at 2:42 AM, Bjorn Helgaas wrote: > > On Mon, Jul 7, 2014 at 12:00 PM, Vidya Sagar wrote: > >>> -----Original Message----- > >>> From: Bjorn Helgaas [mailto:bhelgaas@google.com] > >>> Sent: Sunday, July 06, 2014 12:32 AM > >>> To: Vidya Sagar > >>> Cc: nagananda.chumbalkar@hp.com; thierry.reding@gmail.com; Stephen > >>> Warren; Krishna Thota; linux-pci@vger.kernel.org; linux- > >>> kernel@vger.kernel.org; Matthew Garrett; Rafael J. Wysocki > >>> Subject: Re: [PATCH v1] PCI: enable ASPM configuration in PCIE POWERSAVE > >>> mode > >>> > >>> [+cc Rafael] > >>> > >>> On Sat, Jul 05, 2014 at 12:57:36PM -0600, Bjorn Helgaas wrote: > >>> > [+cc Matthew] > >>> > > >>> > On Tue, Jul 01, 2014 at 12:46:18PM +0530, Vidya Sagar wrote: > >>> > > commit 1a680b7c moved pcie_aspm_powersave_config_link() out of > >>> > > pci_raw_set_power_state() to pci_set_power_state() which would > >>> > > enable ASPM. But, with commit db288c9c, which re-introduced the > >>> > > following check > >>> > > ./drivers/pci/pci.c: pci_set_power_state() > >>> > > + /* Check if we're already there */ > >>> > > + if (dev->current_state == state) > >>> > > + return 0; > >>> > > in pci_set_power_state(), call to pcie_aspm_powersave_config_link() > >>> > > is never made leaving ASPM broken. > >>> > > Fix it by not returning from when the above condition is true, > >>> > > rather, jump to ASPM configuration code and exit from there eventually. > >>> > > >>> > Rafael, Matthew, any comments? We have vacillated on this before and > >>> > the web is already pretty tangled. > >>> > > >> I've raised a bugzilla bug https://bugzilla.kernel.org/show_bug.cgi?id=79621 for the same. > >> Scenario here is, when the driver of a particular PCIe device is loaded and when CONFIG_PCIEASPM_POWERSAVE=y, ASPM is expected to get configured by the sub system which is not happening as of today. > >> The reason for this behavior is the 'if' condition that checks for the device's power state and returns if it is already in D0. Ideally, it shouldn't return from there, rather proceed to configure ASPM. > > > Please find the attached files (before_aspm_patch.txt & > after_aspm_patch.txt) with 'dmesg' and 'lspci -vvv' info. > BTW, my understanding is that this issue (enabling ASPM while booting) > exists for all the devices which are in D0 state. > Please correct my understanding if it's wrong. > Though, enabling aspm through writing 'powersave' to > '/sys/module/pcie_aspm/parameters' is working fine always. I think your observation is correct: it looks like if we boot with all devices in D0 (which I would expect to be a very common case), we don't touch ASPM configuration at all. That would mean we would just leave the BIOS settings alone. It seems pretty surprising that it could have been this way since v3.6, when db288c9c5f9d was merged, and we haven't noticed until now, but if most BIOSes configure ASPM themselves, maybe it's possible. But I wonder if we can untangle ASPM from pci_set_power_state(). I don't think they're really related. There are a zillion .suspend() and .resume() methods that call pci_set_power_state(). I doubt that we need to do ASPM configuration during suspend, and I'm dubious about resume, too. What about a patch like the following? --- 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/pci.c b/drivers/pci/pci.c index 63a54a340863..75fabd1f72bc 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -839,12 +839,6 @@ int pci_set_power_state(struct pci_dev *dev, pci_power_t state) if (!__pci_complete_power_transition(dev, state)) error = 0; - /* - * When aspm_policy is "powersave" this call ensures - * that ASPM is configured. - */ - if (!error && dev->bus->self) - pcie_aspm_powersave_config_link(dev->bus->self); return error; } @@ -1195,12 +1189,18 @@ int __weak pcibios_enable_device(struct pci_dev *dev, int bars) static int do_pci_enable_device(struct pci_dev *dev, int bars) { int err; + struct pci_dev *bridge; u16 cmd; u8 pin; err = pci_set_power_state(dev, PCI_D0); if (err < 0 && err != -EIO) return err; + + bridge = pci_upstream_bridge(dev); + if (bridge) + pcie_aspm_powersave_config_link(bridge); + err = pcibios_enable_device(dev, bars); if (err < 0) return err;