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 988C8CDB465 for ; Mon, 16 Oct 2023 14:28:12 +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:Content-ID:Content-Type: MIME-Version:References:Message-ID:In-Reply-To:Subject:cc:To:From:Date: Reply-To:Content-Transfer-Encoding:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=LeiqOTFtlDLMNgDT96YsY97alNYXvFKcJMBZ/x3C1Bc=; b=d2j+UbhpGK9L+i7lXlG0yjczpy 8IWJsC5mMqF6hJzJVmk3iOFwS34Hss9I8k6EA9YAPYrH1c/8i9u1NeLnaLJydwaMlyBcFExsGqi4S xYwreIODTC48D22pztNFy3cEy2QThY+97nUnNIehWBX8ikm43W29WfY/HjXXAXTJarMLmZ4DMZghx Gz0jxzr/nSczWD7Sn1anXPC/X581gIYi8yNYqP79G+jKxwypVnJmrv4oQo5D1JNpaBfPvF6O48lPV FGC6hlx6/yi4eX1d1spmx17UUBXFwKXCNB4HG7iwEjQBARCHahtkpJ4rOU7FBdSz7gOtpmDA+nRyl NsEGLC9Q==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qsOZX-009riU-1k; Mon, 16 Oct 2023 14:27:55 +0000 Received: from mgamail.intel.com ([134.134.136.24]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qsOZU-009rgf-2A; Mon, 16 Oct 2023 14:27:54 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1697466472; x=1729002472; h=date:from:to:cc:subject:in-reply-to:message-id: references:mime-version:content-id; bh=WAHtEU+QRIIV0hjjctV4dFUOMA87lFSZZckv/e6FmbI=; b=IkTX6lhlY1FuqijdSWvkIzdDkZDGD46bsHERHQbhLODCsVzoxC9LqPd4 u9JsyuuJ8gq3jYan1Pttp8LidJ/z81B7hCN51dWkZ+AQp3vfQSBFAC2tr FLPggcUPSVWGNweI7nHriuXEBBWT95mwuqp0NdiZYc84CiZW8aAky7/+t ko3E9uGNo27Mst/GHEGrnZjNoUUHX3uQ3X8WclFs7xrduH6gAF8hKAsHW MHwl/xY/q4Y8wJjvv1NQ9GDtVZNGDhozt0mvMHk5N/VxuN5+HpgOll/lZ G1ADAo++W6ePndVIW34cJm5HFYjaDjTOwzHIUdM7W2WCRaBc7MhvALhIn Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10863"; a="388396816" X-IronPort-AV: E=Sophos;i="6.03,229,1694761200"; d="scan'208";a="388396816" Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Oct 2023 07:27:48 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10863"; a="759416190" X-IronPort-AV: E=Sophos;i="6.03,229,1694761200"; d="scan'208";a="759416190" Received: from rhaeussl-mobl.ger.corp.intel.com ([10.252.59.103]) by fmsmga007-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Oct 2023 07:27:40 -0700 Date: Mon, 16 Oct 2023 17:27:37 +0300 (EEST) From: =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= To: Bjorn Helgaas , "Rafael J . Wysocki" cc: linux-pci@vger.kernel.org, Lorenzo Pieralisi , Rob Herring , =?ISO-8859-2?Q?Krzysztof_Wilczy=F1ski?= , 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 In-Reply-To: <20231013164228.GA1117889@bhelgaas> Message-ID: References: <20231013164228.GA1117889@bhelgaas> MIME-Version: 1.0 Content-Type: multipart/mixed; BOUNDARY="8323329-158890400-1697462044=:1986" Content-ID: <58c8d854-b57c-582-1ba0-efeb857febe@linux.intel.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231016_072752_769021_2149A43A X-CRM114-Status: GOOD ( 38.01 ) X-BeenThere: ath10k@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "ath10k" Errors-To: ath10k-bounces+ath10k=archiver.kernel.org@lists.infradead.org This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323329-158890400-1697462044=:1986 Content-Type: text/plain; CHARSET=ISO-8859-15 Content-Transfer-Encoding: 8BIT Content-ID: On Fri, 13 Oct 2023, Bjorn Helgaas wrote: > 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. Hi Bjorn, Thanks a lot for all your feedback so far, it has been very helpful. I think there's still one important thing to discuss and none of the comments have covered that area so far. The drivers that have workaround are not going to turn more dangerous than they're already without this change, so we're mostly within charted waters there even with what you propose. However, I think the bigger catch and potential source of problems, with both this v2 and your alternative, are the drivers that do not have the workarounds around CONFIG_PCIEASPM=n and/or _OSC permissions. Those code paths just call pci_disable_link_state() and do nothing else. Do you think it's okay to alter the behavior for those drivers too (disable ASPM where it previously was a no-op)? I'm okay with going the direction you indicated but I just wanted to ask this in advance before reworking the behavior so I can take that detail also into account. -- i. --8323329-158890400-1697462044=:1986 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k --8323329-158890400-1697462044=:1986-- 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 EA38ECDB482 for ; Mon, 16 Oct 2023 14:27:57 +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:Content-ID:Content-Type: MIME-Version:References:Message-ID:In-Reply-To:Subject:cc:To:From:Date: Reply-To:Content-Transfer-Encoding:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=V3yNZGJ+rJpHomMcatn1T1MDK4EjOWQCx9RTT7lCqpA=; b=r3io0CFLbNprWxO/Iw9FU28eh5 Vcya7L+Oy3gOe4NB95cAWIZA/lMN9QBEP6v0crE4PjpO+JYLv4NPn4b5emmttUin22pTBJZRxcO3T vvHyQvCUvmsIVhcLLR3DjL5zM79NVlTJ4l4ovcNKXIxqVel5XLLUPXUx4Uv0ymsLN++TJgWE6UZGi Yc/1kFAvb3YVIYmJ+fs+xIAPy6ldzYhkJT6cFl7rKC5O98Bs0T1xmDk9kakhE8Sjkh2Rdv29l93Y6 t+18P4TNJRPQtfOk9F6VMSXwbSYY5ZHT1d2EooGviPYu/4DSloryr3bpX4EmjRfL/t4SUJybNBJJe ESVfeD3A==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qsOZZ-009rj6-0l; Mon, 16 Oct 2023 14:27:57 +0000 Received: from mgamail.intel.com ([134.134.136.24]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qsOZU-009rgf-2A; Mon, 16 Oct 2023 14:27:54 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1697466472; x=1729002472; h=date:from:to:cc:subject:in-reply-to:message-id: references:mime-version:content-id; bh=WAHtEU+QRIIV0hjjctV4dFUOMA87lFSZZckv/e6FmbI=; b=IkTX6lhlY1FuqijdSWvkIzdDkZDGD46bsHERHQbhLODCsVzoxC9LqPd4 u9JsyuuJ8gq3jYan1Pttp8LidJ/z81B7hCN51dWkZ+AQp3vfQSBFAC2tr FLPggcUPSVWGNweI7nHriuXEBBWT95mwuqp0NdiZYc84CiZW8aAky7/+t ko3E9uGNo27Mst/GHEGrnZjNoUUHX3uQ3X8WclFs7xrduH6gAF8hKAsHW MHwl/xY/q4Y8wJjvv1NQ9GDtVZNGDhozt0mvMHk5N/VxuN5+HpgOll/lZ G1ADAo++W6ePndVIW34cJm5HFYjaDjTOwzHIUdM7W2WCRaBc7MhvALhIn Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10863"; a="388396816" X-IronPort-AV: E=Sophos;i="6.03,229,1694761200"; d="scan'208";a="388396816" Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Oct 2023 07:27:48 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10863"; a="759416190" X-IronPort-AV: E=Sophos;i="6.03,229,1694761200"; d="scan'208";a="759416190" Received: from rhaeussl-mobl.ger.corp.intel.com ([10.252.59.103]) by fmsmga007-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Oct 2023 07:27:40 -0700 Date: Mon, 16 Oct 2023 17:27:37 +0300 (EEST) From: =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= To: Bjorn Helgaas , "Rafael J . Wysocki" cc: linux-pci@vger.kernel.org, Lorenzo Pieralisi , Rob Herring , =?ISO-8859-2?Q?Krzysztof_Wilczy=F1ski?= , 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 In-Reply-To: <20231013164228.GA1117889@bhelgaas> Message-ID: References: <20231013164228.GA1117889@bhelgaas> MIME-Version: 1.0 Content-Type: multipart/mixed; BOUNDARY="8323329-158890400-1697462044=:1986" Content-ID: <58c8d854-b57c-582-1ba0-efeb857febe@linux.intel.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231016_072752_769021_2149A43A X-CRM114-Status: GOOD ( 38.01 ) X-BeenThere: ath11k@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "ath11k" Errors-To: ath11k-bounces+ath11k=archiver.kernel.org@lists.infradead.org This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323329-158890400-1697462044=:1986 Content-Type: text/plain; CHARSET=ISO-8859-15 Content-Transfer-Encoding: 8BIT Content-ID: On Fri, 13 Oct 2023, Bjorn Helgaas wrote: > 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. Hi Bjorn, Thanks a lot for all your feedback so far, it has been very helpful. I think there's still one important thing to discuss and none of the comments have covered that area so far. The drivers that have workaround are not going to turn more dangerous than they're already without this change, so we're mostly within charted waters there even with what you propose. However, I think the bigger catch and potential source of problems, with both this v2 and your alternative, are the drivers that do not have the workarounds around CONFIG_PCIEASPM=n and/or _OSC permissions. Those code paths just call pci_disable_link_state() and do nothing else. Do you think it's okay to alter the behavior for those drivers too (disable ASPM where it previously was a no-op)? I'm okay with going the direction you indicated but I just wanted to ask this in advance before reworking the behavior so I can take that detail also into account. -- i. --8323329-158890400-1697462044=:1986 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline -- ath11k mailing list ath11k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath11k --8323329-158890400-1697462044=:1986-- 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 77351CDB465 for ; Mon, 16 Oct 2023 14:27:56 +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:Content-ID:Content-Type: MIME-Version:References:Message-ID:In-Reply-To:Subject:cc:To:From:Date: Reply-To:Content-Transfer-Encoding:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=pX/mr5Usv4RBqE1MvSbeq/UTuQexpTk4eGCA4N+T9aQ=; b=v9ZAp5/ttJtQTcTUj4TGMsjqqi BEJiz43Ls0qK/H03MgHB0WTioTbu8xiwqUOtG/znSbjv4d4fTZUidbVUOeILuWMmSt3vsCvAchJ4w v6QEOwtmjBUVYYjTF8tXBPJtTatt15dzgJpNErqcwzZLuDgRbId9G1WoRDtywi7UWLCTXn893RJxr zSCHBu2aZHxvOiOzgDoSlwZHTkO/QjNcGrGdaw2/T9y0ch4ZobgEf3Ir9kVs/qk6Jd06mMDv0xRAP 4f9m7sQjxpbgCshW5dcbu+KsZ7woiPD6vF+comexlgbb3huJROyqNm1g8+qSZxAIZ0ZsFduxGM75P ib94t2QQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qsOZY-009ric-0F for ath12k@archiver.kernel.org; Mon, 16 Oct 2023 14:27:56 +0000 Received: from mgamail.intel.com ([134.134.136.24]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qsOZU-009rgf-2A; Mon, 16 Oct 2023 14:27:54 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1697466472; x=1729002472; h=date:from:to:cc:subject:in-reply-to:message-id: references:mime-version:content-id; bh=WAHtEU+QRIIV0hjjctV4dFUOMA87lFSZZckv/e6FmbI=; b=IkTX6lhlY1FuqijdSWvkIzdDkZDGD46bsHERHQbhLODCsVzoxC9LqPd4 u9JsyuuJ8gq3jYan1Pttp8LidJ/z81B7hCN51dWkZ+AQp3vfQSBFAC2tr FLPggcUPSVWGNweI7nHriuXEBBWT95mwuqp0NdiZYc84CiZW8aAky7/+t ko3E9uGNo27Mst/GHEGrnZjNoUUHX3uQ3X8WclFs7xrduH6gAF8hKAsHW MHwl/xY/q4Y8wJjvv1NQ9GDtVZNGDhozt0mvMHk5N/VxuN5+HpgOll/lZ G1ADAo++W6ePndVIW34cJm5HFYjaDjTOwzHIUdM7W2WCRaBc7MhvALhIn Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10863"; a="388396816" X-IronPort-AV: E=Sophos;i="6.03,229,1694761200"; d="scan'208";a="388396816" Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Oct 2023 07:27:48 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10863"; a="759416190" X-IronPort-AV: E=Sophos;i="6.03,229,1694761200"; d="scan'208";a="759416190" Received: from rhaeussl-mobl.ger.corp.intel.com ([10.252.59.103]) by fmsmga007-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Oct 2023 07:27:40 -0700 Date: Mon, 16 Oct 2023 17:27:37 +0300 (EEST) From: =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= To: Bjorn Helgaas , "Rafael J . Wysocki" cc: linux-pci@vger.kernel.org, Lorenzo Pieralisi , Rob Herring , =?ISO-8859-2?Q?Krzysztof_Wilczy=F1ski?= , 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 In-Reply-To: <20231013164228.GA1117889@bhelgaas> Message-ID: References: <20231013164228.GA1117889@bhelgaas> MIME-Version: 1.0 Content-Type: multipart/mixed; BOUNDARY="8323329-158890400-1697462044=:1986" Content-ID: <58c8d854-b57c-582-1ba0-efeb857febe@linux.intel.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231016_072752_769021_2149A43A X-CRM114-Status: GOOD ( 38.01 ) X-BeenThere: ath12k@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "ath12k" Errors-To: ath12k-bounces+ath12k=archiver.kernel.org@lists.infradead.org This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323329-158890400-1697462044=:1986 Content-Type: text/plain; CHARSET=ISO-8859-15 Content-Transfer-Encoding: 8BIT Content-ID: On Fri, 13 Oct 2023, Bjorn Helgaas wrote: > 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. Hi Bjorn, Thanks a lot for all your feedback so far, it has been very helpful. I think there's still one important thing to discuss and none of the comments have covered that area so far. The drivers that have workaround are not going to turn more dangerous than they're already without this change, so we're mostly within charted waters there even with what you propose. However, I think the bigger catch and potential source of problems, with both this v2 and your alternative, are the drivers that do not have the workarounds around CONFIG_PCIEASPM=n and/or _OSC permissions. Those code paths just call pci_disable_link_state() and do nothing else. Do you think it's okay to alter the behavior for those drivers too (disable ASPM where it previously was a no-op)? I'm okay with going the direction you indicated but I just wanted to ask this in advance before reworking the behavior so I can take that detail also into account. -- i. --8323329-158890400-1697462044=:1986 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline -- ath12k mailing list ath12k@lists.infradead.org https://lists.infradead.org/mailman/listinfo/ath12k --8323329-158890400-1697462044=:1986-- 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 smtp1.osuosl.org (smtp1.osuosl.org [140.211.166.138]) (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 B9AA5CDB483 for ; Tue, 17 Oct 2023 17:26:16 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id 4BC9882173; Tue, 17 Oct 2023 17:26:16 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org 4BC9882173 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=osuosl.org; s=default; t=1697563576; bh=pWnRJ00lGIuaYvaHO2mTB2IfXaaHtW6sKS5+2hC/YAU=; h=Date:From:To:In-Reply-To:References:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: Cc:From; b=y2TQxEmezl68hPaNONKDH3SOpc1dCMxCVBFkOjAAv6XECBf5lPKZ4vMZII3X0eiFW kUGAJfRk9bWmljzLKUt1hcWtxGrsWMtb/5RbVh3w4R0JRn8TvlcUh4vKspFG1sNsIY 9zQWSh6pH14h+nGVL35hwogOmI0lcUtIQNcAkrnR29b2uC7C4KIkU6sYxCTgRSZ9n3 bp0OUzL0uJNuP5zeoY1cPRtuma5a5S5KsLS7Yzctrux1Pleo3ZqZr45yKWxngrENUF jHcWd99b6MYZlmxUnxmEOaPj2wK8xLo2J0YAd338wLjRcrUTLzG+qBdjvotnvDRZi+ BGymFYWUqawYA== X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp1.osuosl.org ([127.0.0.1]) by localhost (smtp1.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id vnEnZQXH9exw; Tue, 17 Oct 2023 17:26:13 +0000 (UTC) Received: from ash.osuosl.org (ash.osuosl.org [140.211.166.34]) by smtp1.osuosl.org (Postfix) with ESMTP id C192E82195; Tue, 17 Oct 2023 17:26:12 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org C192E82195 Received: from smtp1.osuosl.org (smtp1.osuosl.org [140.211.166.138]) by ash.osuosl.org (Postfix) with ESMTP id D61691BF28E for ; Mon, 16 Oct 2023 14:27:50 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id AEAA481F16 for ; Mon, 16 Oct 2023 14:27:50 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org AEAA481F16 X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp1.osuosl.org ([127.0.0.1]) by localhost (smtp1.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Om_dswjGKAYv for ; Mon, 16 Oct 2023 14:27:49 +0000 (UTC) Received: from mgamail.intel.com (mgamail.intel.com [134.134.136.24]) by smtp1.osuosl.org (Postfix) with ESMTPS id A6DFB81EE8 for ; Mon, 16 Oct 2023 14:27:49 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org A6DFB81EE8 X-IronPort-AV: E=McAfee;i="6600,9927,10863"; a="388396815" X-IronPort-AV: E=Sophos;i="6.03,229,1694761200"; d="scan'208";a="388396815" Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Oct 2023 07:27:48 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10863"; a="759416190" X-IronPort-AV: E=Sophos;i="6.03,229,1694761200"; d="scan'208";a="759416190" Received: from rhaeussl-mobl.ger.corp.intel.com ([10.252.59.103]) by fmsmga007-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Oct 2023 07:27:40 -0700 Date: Mon, 16 Oct 2023 17:27:37 +0300 (EEST) From: =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= To: Bjorn Helgaas , "Rafael J . Wysocki" In-Reply-To: <20231013164228.GA1117889@bhelgaas> Message-ID: References: <20231013164228.GA1117889@bhelgaas> MIME-Version: 1.0 Content-Type: multipart/mixed; BOUNDARY="8323329-158890400-1697462044=:1986" Content-ID: <58c8d854-b57c-582-1ba0-efeb857febe@linux.intel.com> X-Mailman-Approved-At: Tue, 17 Oct 2023 17:25:58 +0000 X-Mailman-Original-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1697466469; x=1729002469; h=date:from:to:cc:subject:in-reply-to:message-id: references:mime-version:content-id; bh=WAHtEU+QRIIV0hjjctV4dFUOMA87lFSZZckv/e6FmbI=; b=OufOnZrvvyE1EFjz2aoL107j51mw+MlBz2TFVCRqpCfgUt85f4ijc/pP j/9ecFLSccMKEYoA567fzz9qaGY00PhiO+HDPSvfjkBHiVjyIm14KTizG Fb44jlk17oT5pusE1CkTJFjOnuJeFumjO1IuU0UVxMPLbXrrKDR7t07UE 2Fd6e0+ZI+Rve8FfhwYDwBOoOnLDxdnFqIPab+8IptCjJvKNNWFu+grAN GqGkxvAIOQEead+a8eeKv91/reJGhd6IePsZa+D23bSBVI7SXV4P+20tS ShiT5vK+/Wh6PTZpZCxkas9Nty1A3Wui/wodkt5FhPfcowBf3i6Eh2xV2 g==; X-Mailman-Original-Authentication-Results: smtp1.osuosl.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.a=rsa-sha256 header.s=Intel header.b=OufOnZrv Subject: Re: [Intel-wired-lan] [PATCH v2 03/13] PCI/ASPM: Disable ASPM when driver requests it X-BeenThere: intel-wired-lan@osuosl.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel Wired Ethernet Linux Kernel Driver Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: =?ISO-8859-2?Q?Krzysztof_Wilczy=F1ski?= , linux-rdma@vger.kernel.org, Lorenzo Pieralisi , Rob Herring , ath12k@lists.infradead.org, linux-pci@vger.kernel.org, linux-wireless@vger.kernel.org, LKML , ath10k@lists.infradead.org, Emmanuel Grumbach , linux-bluetooth@vger.kernel.org, Lukas Wunner , intel-wired-lan@lists.osuosl.org, Netdev , Bjorn Helgaas , linux-mediatek@lists.infradead.org, ath11k@lists.infradead.org, linux-arm-kernel@lists.infradead.org, Heiner Kallweit Errors-To: intel-wired-lan-bounces@osuosl.org Sender: "Intel-wired-lan" This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323329-158890400-1697462044=:1986 Content-Type: text/plain; CHARSET=ISO-8859-15 Content-Transfer-Encoding: 8BIT Content-ID: On Fri, 13 Oct 2023, Bjorn Helgaas wrote: > 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. Hi Bjorn, Thanks a lot for all your feedback so far, it has been very helpful. I think there's still one important thing to discuss and none of the comments have covered that area so far. The drivers that have workaround are not going to turn more dangerous than they're already without this change, so we're mostly within charted waters there even with what you propose. However, I think the bigger catch and potential source of problems, with both this v2 and your alternative, are the drivers that do not have the workarounds around CONFIG_PCIEASPM=n and/or _OSC permissions. Those code paths just call pci_disable_link_state() and do nothing else. Do you think it's okay to alter the behavior for those drivers too (disable ASPM where it previously was a no-op)? I'm okay with going the direction you indicated but I just wanted to ask this in advance before reworking the behavior so I can take that detail also into account. -- i. --8323329-158890400-1697462044=:1986 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Intel-wired-lan mailing list Intel-wired-lan@osuosl.org https://lists.osuosl.org/mailman/listinfo/intel-wired-lan --8323329-158890400-1697462044=:1986-- 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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 661BCC41513 for ; Mon, 16 Oct 2023 14:27:52 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233736AbjJPO1w (ORCPT ); Mon, 16 Oct 2023 10:27:52 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42206 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232381AbjJPO1v (ORCPT ); Mon, 16 Oct 2023 10:27:51 -0400 Received: from mgamail.intel.com (mgamail.intel.com [134.134.136.24]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id CFA8683; Mon, 16 Oct 2023 07:27:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1697466468; x=1729002468; h=date:from:to:cc:subject:in-reply-to:message-id: references:mime-version:content-id; bh=WAHtEU+QRIIV0hjjctV4dFUOMA87lFSZZckv/e6FmbI=; b=MLdYaMUrX5YccmQcg2hEP63YIkH+x1b3KeRgjLxsRYMcuSx99wFnnN6R IzN66rHPFhvJKLQ5WOEcR5hT9yNZ5cybG6UUjUvmz6wtAPXVToW0V62OQ KWIsNyrR/rnjiWcpfFHWYHYRzurXPtROJnnmoOY/gJUYmC/SAREcjIa8G dr+7jGx5XGCIU974YKCV8goveF9t0DYWMxLia3zr31EIN2VMir1eVyQXc ghvlcbrHLr8zwmKLE1HtgSvqd+Mc+eQ/s0YnbLieqOoVgsSWB/76qp6TV AS10mjor5216OwX7Lpd9ZCG58YtCpoikRazXWSM0i+nYQqeeyZuCo2kwW g==; X-IronPort-AV: E=McAfee;i="6600,9927,10863"; a="388396812" X-IronPort-AV: E=Sophos;i="6.03,229,1694761200"; d="scan'208";a="388396812" Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Oct 2023 07:27:48 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10863"; a="759416190" X-IronPort-AV: E=Sophos;i="6.03,229,1694761200"; d="scan'208";a="759416190" Received: from rhaeussl-mobl.ger.corp.intel.com ([10.252.59.103]) by fmsmga007-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Oct 2023 07:27:40 -0700 Date: Mon, 16 Oct 2023 17:27:37 +0300 (EEST) From: =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= To: Bjorn Helgaas , "Rafael J . Wysocki" cc: linux-pci@vger.kernel.org, Lorenzo Pieralisi , Rob Herring , =?ISO-8859-2?Q?Krzysztof_Wilczy=F1ski?= , 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 In-Reply-To: <20231013164228.GA1117889@bhelgaas> Message-ID: References: <20231013164228.GA1117889@bhelgaas> MIME-Version: 1.0 Content-Type: multipart/mixed; BOUNDARY="8323329-158890400-1697462044=:1986" Content-ID: <58c8d854-b57c-582-1ba0-efeb857febe@linux.intel.com> Precedence: bulk List-ID: X-Mailing-List: linux-bluetooth@vger.kernel.org This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323329-158890400-1697462044=:1986 Content-Type: text/plain; CHARSET=ISO-8859-15 Content-Transfer-Encoding: 8BIT Content-ID: On Fri, 13 Oct 2023, Bjorn Helgaas wrote: > 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. Hi Bjorn, Thanks a lot for all your feedback so far, it has been very helpful. I think there's still one important thing to discuss and none of the comments have covered that area so far. The drivers that have workaround are not going to turn more dangerous than they're already without this change, so we're mostly within charted waters there even with what you propose. However, I think the bigger catch and potential source of problems, with both this v2 and your alternative, are the drivers that do not have the workarounds around CONFIG_PCIEASPM=n and/or _OSC permissions. Those code paths just call pci_disable_link_state() and do nothing else. Do you think it's okay to alter the behavior for those drivers too (disable ASPM where it previously was a no-op)? I'm okay with going the direction you indicated but I just wanted to ask this in advance before reworking the behavior so I can take that detail also into account. -- i. --8323329-158890400-1697462044=:1986-- 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 23379CDB465 for ; Mon, 16 Oct 2023 14:28:22 +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:Content-ID:Content-Type: MIME-Version:References:Message-ID:In-Reply-To:Subject:cc:To:From:Date: Reply-To:Content-Transfer-Encoding:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=oCZTFkHreEY3tXu/zRTu+TPLpCQTzTkhURAfJHTxQTI=; b=SG/+J98MO9xIprVROwtZ7ktQEC 3i7O+0BIVon8SQieqeMMZ8zCCWJlCchHNsj0TVdc8tdZjveIrQpK3E4feYPNrE5lRqdUm8i99AUmH Ds4Y9vwFCBGYZQ/XlFJP8/QA8tjk8VdURHCnM4kTEUnLAwTGo4CNcc+3sD+oTSl+0KYigZ6DO6yDl KThT4c6KZxRlgBkMaAVaI8OfVpe8Inov0gn/wQay1sxVkIveoZfGrl4ptJ9wk7V+nGMH0puJomNE2 AmkpzlXt69bR+YNGth+sDv1HAemuj/6I3AL7JDD3immvFv+XIpW5b+jDBTLebSVkMnG7m17dqlKvJ ld4WHDOA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qsOZY-009rio-1P; Mon, 16 Oct 2023 14:27:56 +0000 Received: from mgamail.intel.com ([134.134.136.24]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qsOZU-009rgf-2A; Mon, 16 Oct 2023 14:27:54 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1697466472; x=1729002472; h=date:from:to:cc:subject:in-reply-to:message-id: references:mime-version:content-id; bh=WAHtEU+QRIIV0hjjctV4dFUOMA87lFSZZckv/e6FmbI=; b=IkTX6lhlY1FuqijdSWvkIzdDkZDGD46bsHERHQbhLODCsVzoxC9LqPd4 u9JsyuuJ8gq3jYan1Pttp8LidJ/z81B7hCN51dWkZ+AQp3vfQSBFAC2tr FLPggcUPSVWGNweI7nHriuXEBBWT95mwuqp0NdiZYc84CiZW8aAky7/+t ko3E9uGNo27Mst/GHEGrnZjNoUUHX3uQ3X8WclFs7xrduH6gAF8hKAsHW MHwl/xY/q4Y8wJjvv1NQ9GDtVZNGDhozt0mvMHk5N/VxuN5+HpgOll/lZ G1ADAo++W6ePndVIW34cJm5HFYjaDjTOwzHIUdM7W2WCRaBc7MhvALhIn Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10863"; a="388396816" X-IronPort-AV: E=Sophos;i="6.03,229,1694761200"; d="scan'208";a="388396816" Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Oct 2023 07:27:48 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10863"; a="759416190" X-IronPort-AV: E=Sophos;i="6.03,229,1694761200"; d="scan'208";a="759416190" Received: from rhaeussl-mobl.ger.corp.intel.com ([10.252.59.103]) by fmsmga007-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Oct 2023 07:27:40 -0700 Date: Mon, 16 Oct 2023 17:27:37 +0300 (EEST) From: =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= To: Bjorn Helgaas , "Rafael J . Wysocki" cc: linux-pci@vger.kernel.org, Lorenzo Pieralisi , Rob Herring , =?ISO-8859-2?Q?Krzysztof_Wilczy=F1ski?= , 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 In-Reply-To: <20231013164228.GA1117889@bhelgaas> Message-ID: References: <20231013164228.GA1117889@bhelgaas> MIME-Version: 1.0 Content-Type: multipart/mixed; BOUNDARY="8323329-158890400-1697462044=:1986" Content-ID: <58c8d854-b57c-582-1ba0-efeb857febe@linux.intel.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231016_072752_769021_2149A43A X-CRM114-Status: GOOD ( 38.01 ) X-BeenThere: linux-arm-kernel@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-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323329-158890400-1697462044=:1986 Content-Type: text/plain; CHARSET=ISO-8859-15 Content-Transfer-Encoding: 8BIT Content-ID: On Fri, 13 Oct 2023, Bjorn Helgaas wrote: > 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. Hi Bjorn, Thanks a lot for all your feedback so far, it has been very helpful. I think there's still one important thing to discuss and none of the comments have covered that area so far. The drivers that have workaround are not going to turn more dangerous than they're already without this change, so we're mostly within charted waters there even with what you propose. However, I think the bigger catch and potential source of problems, with both this v2 and your alternative, are the drivers that do not have the workarounds around CONFIG_PCIEASPM=n and/or _OSC permissions. Those code paths just call pci_disable_link_state() and do nothing else. Do you think it's okay to alter the behavior for those drivers too (disable ASPM where it previously was a no-op)? I'm okay with going the direction you indicated but I just wanted to ask this in advance before reworking the behavior so I can take that detail also into account. -- i. --8323329-158890400-1697462044=:1986 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel --8323329-158890400-1697462044=:1986--