From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id B30B7CDB486 for ; Fri, 13 Oct 2023 16:42:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To: Content-Transfer-Encoding:Content-Type:MIME-Version:Message-ID:Subject:Cc:To: From:Date:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:References:List-Owner; bh=I82J6tKVpwxNq9aEGQO/8yoS1Ouunq9K13m48AlSuB8=; b=oX1qfSg1B74AZBLarPu/w8/fG5 1HtlElCr1pPFQBz2S3ELPBbBQCChurB0aRLcgZUcCiB5d4/sdyQO7/ZDHqERo+A4s1LObQFiaf/Sj v5J6BrQ17b6l1yjGhQHtUXsd5W3d43tWNQucJySkJGHzcj4LoxdTOgf5JqiIdED4Ef4NXSX3wnv5K BnM+Yn4gZUmJu7Ikto+sVwD0ZhDYoHPNKH1XYSyvgslXZTGN76aarZ2EFW4/2MAQNKRikR/mU5fDI ouwk5ZaGHlI2FxrZaexHLyn8FKiOvqasI2XvVlTqhXWlb8zhL0jZhO/VX2LdC8DWhTgob78jba3h6 xrGYxPaw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qrLFF-003tru-1H; Fri, 13 Oct 2023 16:42:37 +0000 Received: from sin.source.kernel.org ([145.40.73.55]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qrLFA-003toi-2d; Fri, 13 Oct 2023 16:42:34 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id 2D17ACE315D; Fri, 13 Oct 2023 16:42:31 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 1C518C433C8; Fri, 13 Oct 2023 16:42:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1697215350; bh=MqC3yrRRj3UMGOPti8ERknVNqGEUu0oFiV03E0Vidv0=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=b86OTm7/vWPz1lXdWBiZn7RcGveX1aFsvL5jmXTq2YbcyNGIbGhWA6q1/AnMGbd1N IFYYegR+vc8lMskeUk8lajThoc+bo1PlPWPhNqD2fwOG3AfKYpg90Eza/kNH0uSNPW Jky7Vj9783vlIkTQS3O7jJucPYVzjXAq/DJZUjUVY+A/BEROWzFLMN0GpWgbN/I8Vu ImQsRWZuesvRBHjxd0H3/8ZdxSWJjJnhbYb4F5CAQ1P15D5R2lTSA+hkWwJq0LpHAe Bn/6fD9G+fr9mHm/5sCM1EEe3TdFQCGEcv6U3ysqKGhpLI6CBcvRuzJ9zOUE+g+0U1 YdquVEXDnza8g== Date: Fri, 13 Oct 2023 11:42:28 -0500 From: Bjorn Helgaas To: Ilpo =?utf-8?B?SsOkcnZpbmVu?= Cc: "Rafael J . Wysocki" , linux-pci@vger.kernel.org, Lorenzo Pieralisi , Rob Herring , Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= , Lukas Wunner , Heiner Kallweit , Emmanuel Grumbach , LKML , Bjorn Helgaas , ath10k@lists.infradead.org, ath11k@lists.infradead.org, ath12k@lists.infradead.org, intel-wired-lan@lists.osuosl.org, linux-arm-kernel@lists.infradead.org, linux-bluetooth@vger.kernel.org, linux-mediatek@lists.infradead.org, linux-rdma@vger.kernel.org, linux-wireless@vger.kernel.org, Netdev Subject: Re: [PATCH v2 03/13] PCI/ASPM: Disable ASPM when driver requests it Message-ID: <20231013164228.GA1117889@bhelgaas> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231013_094233_197241_07A7B50C X-CRM114-Status: GOOD ( 29.33 ) X-BeenThere: linux-mediatek@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org On Thu, Oct 12, 2023 at 01:56:16PM +0300, Ilpo Järvinen wrote: > On Wed, 11 Oct 2023, Bjorn Helgaas wrote: > > On Mon, Sep 18, 2023 at 04:10:53PM +0300, Ilpo Järvinen wrote: > > > PCI core/ASPM service driver allows controlling ASPM state through > > > pci_disable_link_state() and pci_enable_link_state() API. It was > > > decided earlier (see the Link below), to not allow ASPM changes when OS > > > does not have control over it but only log a warning about the problem > > > (commit 2add0ec14c25 ("PCI/ASPM: Warn when driver asks to disable ASPM, > > > but we can't do it")). Similarly, if ASPM is not enabled through > > > config, ASPM cannot be disabled. > ... > > This disables *all* ASPM states, unlike the version when > > CONFIG_PCIEASPM is enabled. I suppose there's a reason, and maybe a > > comment could elaborate on it? > > > > When CONFIG_PCIEASPM is not enabled, I don't think we actively > > *disable* ASPM in the hardware; we just leave it as-is, so firmware > > might have left it enabled. > > This whole trickery is intended for drivers that do not want to have ASPM > because the devices are broken with it. So leaving it as-is is not really > an option (as demonstrated by the custom workarounds). Right. > > Conceptually it seems like the LNKCTL updates here should be the same > > whether CONFIG_PCIEASPM is enabled or not (subject to the question > > above). > > > > When CONFIG_PCIEASPM is enabled, we might need to do more stuff, but > > it seems like the core should be the same. > > So you think it's safer to partially disable ASPM (as per driver's > request) rather than disable it completely? I got the impression that the > latter might be safer from what Rafael said earlier but I suppose I might > have misinterpreted him since he didn't exactly say that it might be safer > to _completely_ disable it. My question is whether the state of the device should depend on CONFIG_PCIEASPM. If the driver does this: pci_disable_link_state(PCIE_LINK_STATE_L0S) do we want to leave L1 enabled when CONFIG_PCIEASPM=y but disable L1 when CONFIG_PCIEASPM is unset? I can see arguments both ways. My thought was that it would be nice to end up with a single implementation of pci_disable_link_state() with an #ifdef around the CONFIG_PCIEASPM-enabled stuff because it makes the code easier to read. Bjorn