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 D50A1CDB47E for ; Thu, 12 Oct 2023 12:53:55 +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-Type:MIME-Version: References:Message-ID:In-Reply-To:Subject:cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=rf8TuMArbL5hK3AxfSp0ebkbiI4LjN+6XF/ZWhYUbqA=; b=fvVjkphxH88rYJTFO/nYGqcm/V nCjKpY+TzwAcv2C72uXpKKCDNUZeilPpQdCeQbV83UbDMRhIpSvTRUAe9hXMdsTX0k4RWj7XFYUh+ H1nffIvnTYZ94ZsyMT2QkpbZ6yMVQ9eQ85mx6sefXSLU6KAFFbPTa9qfOzFJpAfI1NmsI7ZMu7s+8 oDR9gsgkdRMFBZLLVBFOFGXaZDsBXuwVRcj60Li8BQ7tftkwDYDyb3AIsMM22r1ZeyVUPtpFDK4LC z5luAPUJ0dfJjr0+je/u5FqoZ+E/XvtBaxdM5zptJCo7v+qd95XJ2ufMcpZVVJDjV2iIkVVM+ymPD 3LMIVd2w==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qqvCJ-0010jN-2n; Thu, 12 Oct 2023 12:53:51 +0000 Received: from mgamail.intel.com ([192.55.52.43]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qqvCG-0010i1-1s; Thu, 12 Oct 2023 12:53:50 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1697115228; x=1728651228; h=date:from:to:cc:subject:in-reply-to:message-id: references:mime-version; bh=gNLNaEec64/5yPf43OhzXjxJ+9f0AwPObPN0gw90rTk=; b=l+KzQ3z0GQLCht0HmiqacGHhpEMtgMdO5q2rCnYomL1KYTbxShJs0zUa h+5o6jAsOs+jcq4yjmFn8OSsA32I1f3sJkuU1HRuXCYN5UuydAY9XG+/q g0o+icxIaCOEvmOZZMxknHpiWRX0Q5REJadAkofO8zObSG4VZiH62DSXy m2bSH2+oxjYLfCv+pw6oaYnN2ZEjXRiZ2/JgSD8lKbQotF9S5OxMN0ZUW 00I3A5HGJIyt6s6j/8RrfxItDzCe3TktTjcLmigG8xsXsaEL/fPaMvgHT c+TViZ8cVXQV9X+EnUTGszKKj7QFRmxEULFp+thRjtCO3VzY0SwpSBlBF Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10861"; a="471169653" X-IronPort-AV: E=Sophos;i="6.03,219,1694761200"; d="scan'208";a="471169653" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Oct 2023 05:53:46 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10861"; a="783688889" X-IronPort-AV: E=Sophos;i="6.03,219,1694761200"; d="scan'208";a="783688889" Received: from asroczyn-mobl.ger.corp.intel.com ([10.249.36.107]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Oct 2023 05:53:41 -0700 Date: Thu, 12 Oct 2023 15:53:39 +0300 (EEST) From: =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= To: Bjorn Helgaas cc: linux-pci@vger.kernel.org, Lorenzo Pieralisi , Rob Herring , =?ISO-8859-2?Q?Krzysztof_Wilczy=F1ski?= , Lukas Wunner , "Rafael J . Wysocki" , 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 05/13] PCI/ASPM: Add pci_enable_link_state() In-Reply-To: <20231011215327.GA1043654@bhelgaas> Message-ID: References: <20231011215327.GA1043654@bhelgaas> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="8323329-1586418908-1697115226=:1692" X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231012_055348_678866_24D31BB7 X-CRM114-Status: GOOD ( 29.48 ) 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-1586418908-1697115226=:1692 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT On Wed, 11 Oct 2023, Bjorn Helgaas wrote: > On Mon, Sep 18, 2023 at 04:10:55PM +0300, Ilpo Järvinen wrote: > > pci_disable_link_state() lacks a symmetric pair. Some drivers want to > > disable ASPM during certain phases of their operation but then > > re-enable it later on. If pci_disable_link_state() is made for the > > device, there is currently no way to re-enable the states that were > > disabled. > > pci_disable_link_state() gives drivers a way to disable specified ASPM > states using a bitmask (PCIE_LINK_STATE_L0S, PCIE_LINK_STATE_L1, > PCIE_LINK_STATE_L1_1, etc), but IIUC the driver can't tell exactly > what changed and can't directly restore the original state, e.g., > > - PCIE_LINK_STATE_L1 enabled initially > - driver calls pci_disable_link_state(PCIE_LINK_STATE_L0S) > - driver calls pci_enable_link_state(PCIE_LINK_STATE_L0S) > - PCIE_LINK_STATE_L0S and PCIE_LINK_STATE_L1 are enabled now > > Now PCIE_LINK_STATE_L0S is enabled even though it was not initially > enabled. Maybe that's what we want; I dunno. > > pci_disable_link_state() currently returns success/failure, but only > r8169 and mt76 even check, and only rtl_init_one() (r8169) has a > non-trivial reason, so it's conceivable that it could return a bitmask > instead. It's great that you suggested this since it's actually what also I've been started to think should be done instead of this straightforward approach I used in V2. That is, don't have the drivers to get anything directly from LNKCTL but they should get everything through the API provided by the disable/enable calls which makes it easy for the driver to pass the same value back into the enable call. > > Add pci_enable_link_state() to remove ASPM states from the state > > disable mask. > > > > Signed-off-by: Ilpo Järvinen > > --- > > drivers/pci/pcie/aspm.c | 42 +++++++++++++++++++++++++++++++++++++++++ > > include/linux/pci.h | 2 ++ > > 2 files changed, 44 insertions(+) > > > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > > index 91dc95aca90f..f45d18d47c20 100644 > > --- a/drivers/pci/pcie/aspm.c > > +++ b/drivers/pci/pcie/aspm.c > > @@ -1117,6 +1117,48 @@ int pci_disable_link_state(struct pci_dev *pdev, int state) > > } > > EXPORT_SYMBOL(pci_disable_link_state); > > > > +/** > > + * pci_enable_link_state - Re-enable device's link state > > + * @pdev: PCI device > > + * @state: ASPM link states to re-enable > > + * > > + * Enable device's link state that were previously disable so the link is > > "state[s] that were previously disable[d]" alludes to the use case you > have in mind, but I don't think it describes how this function > actually works. This function just makes it possible to enable the > specified states. The @state parameter may have nothing to do with > any previously disabled states. Yes, it's what I've been thinking between the lines. But I see your point that this API didn't make it easy/obvious as is. Would you want me to enforce it too besides altering the API such that the states are actually returned from disable call? (I don't personally find that necessary as long as the API pair itself makes it obvious what the driver is expect to pass there.) -- i. --8323329-1586418908-1697115226=:1692 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-1586418908-1697115226=:1692-- 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 D5DEACDB47E for ; Thu, 12 Oct 2023 12:53:58 +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-Type:MIME-Version: References:Message-ID:In-Reply-To:Subject:cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=/N9thMRcGXuNZqHbd/pDn98p+xauXiNGkrKplttdics=; b=eEg26spnZR9VosOONXCv/LBB0x 5J9k2mUaSKmH4fA8dhMi53wvyCEJPGrTZ0oY9iIIeKQAN/YYxhRblFZPL+tMwbhUE5u8fOGtKYXvk mk5HNfl8oktHEDncjNrCXiF7TBEWuSNOZuisAVbo3qlM0jtjrW3OAi5bfcuyHh7hF0MtF1YPM14oB a1ci2A8dgZd6bYVDZMJ5OQIo0tCPFYp7z7uuc3Jo5lf53bSi0hJilkcaQLA9C/gYuUiajdVcSJcpc l73E/Kgbvs3iro7seHVRBJZR4D3LYYz6Xgd/TAuUtJHUEVys/gAJFHb5CDUKF9GGYD2Bx6qGgxMUS 8impQgcQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qqvCM-0010kV-11; Thu, 12 Oct 2023 12:53:54 +0000 Received: from mgamail.intel.com ([192.55.52.43]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qqvCG-0010i1-1s; Thu, 12 Oct 2023 12:53:50 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1697115228; x=1728651228; h=date:from:to:cc:subject:in-reply-to:message-id: references:mime-version; bh=gNLNaEec64/5yPf43OhzXjxJ+9f0AwPObPN0gw90rTk=; b=l+KzQ3z0GQLCht0HmiqacGHhpEMtgMdO5q2rCnYomL1KYTbxShJs0zUa h+5o6jAsOs+jcq4yjmFn8OSsA32I1f3sJkuU1HRuXCYN5UuydAY9XG+/q g0o+icxIaCOEvmOZZMxknHpiWRX0Q5REJadAkofO8zObSG4VZiH62DSXy m2bSH2+oxjYLfCv+pw6oaYnN2ZEjXRiZ2/JgSD8lKbQotF9S5OxMN0ZUW 00I3A5HGJIyt6s6j/8RrfxItDzCe3TktTjcLmigG8xsXsaEL/fPaMvgHT c+TViZ8cVXQV9X+EnUTGszKKj7QFRmxEULFp+thRjtCO3VzY0SwpSBlBF Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10861"; a="471169653" X-IronPort-AV: E=Sophos;i="6.03,219,1694761200"; d="scan'208";a="471169653" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Oct 2023 05:53:46 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10861"; a="783688889" X-IronPort-AV: E=Sophos;i="6.03,219,1694761200"; d="scan'208";a="783688889" Received: from asroczyn-mobl.ger.corp.intel.com ([10.249.36.107]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Oct 2023 05:53:41 -0700 Date: Thu, 12 Oct 2023 15:53:39 +0300 (EEST) From: =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= To: Bjorn Helgaas cc: linux-pci@vger.kernel.org, Lorenzo Pieralisi , Rob Herring , =?ISO-8859-2?Q?Krzysztof_Wilczy=F1ski?= , Lukas Wunner , "Rafael J . Wysocki" , 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 05/13] PCI/ASPM: Add pci_enable_link_state() In-Reply-To: <20231011215327.GA1043654@bhelgaas> Message-ID: References: <20231011215327.GA1043654@bhelgaas> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="8323329-1586418908-1697115226=:1692" X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231012_055348_678866_24D31BB7 X-CRM114-Status: GOOD ( 29.48 ) 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-1586418908-1697115226=:1692 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT On Wed, 11 Oct 2023, Bjorn Helgaas wrote: > On Mon, Sep 18, 2023 at 04:10:55PM +0300, Ilpo Järvinen wrote: > > pci_disable_link_state() lacks a symmetric pair. Some drivers want to > > disable ASPM during certain phases of their operation but then > > re-enable it later on. If pci_disable_link_state() is made for the > > device, there is currently no way to re-enable the states that were > > disabled. > > pci_disable_link_state() gives drivers a way to disable specified ASPM > states using a bitmask (PCIE_LINK_STATE_L0S, PCIE_LINK_STATE_L1, > PCIE_LINK_STATE_L1_1, etc), but IIUC the driver can't tell exactly > what changed and can't directly restore the original state, e.g., > > - PCIE_LINK_STATE_L1 enabled initially > - driver calls pci_disable_link_state(PCIE_LINK_STATE_L0S) > - driver calls pci_enable_link_state(PCIE_LINK_STATE_L0S) > - PCIE_LINK_STATE_L0S and PCIE_LINK_STATE_L1 are enabled now > > Now PCIE_LINK_STATE_L0S is enabled even though it was not initially > enabled. Maybe that's what we want; I dunno. > > pci_disable_link_state() currently returns success/failure, but only > r8169 and mt76 even check, and only rtl_init_one() (r8169) has a > non-trivial reason, so it's conceivable that it could return a bitmask > instead. It's great that you suggested this since it's actually what also I've been started to think should be done instead of this straightforward approach I used in V2. That is, don't have the drivers to get anything directly from LNKCTL but they should get everything through the API provided by the disable/enable calls which makes it easy for the driver to pass the same value back into the enable call. > > Add pci_enable_link_state() to remove ASPM states from the state > > disable mask. > > > > Signed-off-by: Ilpo Järvinen > > --- > > drivers/pci/pcie/aspm.c | 42 +++++++++++++++++++++++++++++++++++++++++ > > include/linux/pci.h | 2 ++ > > 2 files changed, 44 insertions(+) > > > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > > index 91dc95aca90f..f45d18d47c20 100644 > > --- a/drivers/pci/pcie/aspm.c > > +++ b/drivers/pci/pcie/aspm.c > > @@ -1117,6 +1117,48 @@ int pci_disable_link_state(struct pci_dev *pdev, int state) > > } > > EXPORT_SYMBOL(pci_disable_link_state); > > > > +/** > > + * pci_enable_link_state - Re-enable device's link state > > + * @pdev: PCI device > > + * @state: ASPM link states to re-enable > > + * > > + * Enable device's link state that were previously disable so the link is > > "state[s] that were previously disable[d]" alludes to the use case you > have in mind, but I don't think it describes how this function > actually works. This function just makes it possible to enable the > specified states. The @state parameter may have nothing to do with > any previously disabled states. Yes, it's what I've been thinking between the lines. But I see your point that this API didn't make it easy/obvious as is. Would you want me to enforce it too besides altering the API such that the states are actually returned from disable call? (I don't personally find that necessary as long as the API pair itself makes it obvious what the driver is expect to pass there.) -- i. --8323329-1586418908-1697115226=:1692 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-1586418908-1697115226=:1692-- 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 3025ACDB46E for ; Thu, 12 Oct 2023 12:53:53 +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-Type:MIME-Version: References:Message-ID:In-Reply-To:Subject:cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=sn47WvHrEyWpVv4FYQvSfam8ay+Bd9DBQVJRv1TEpGk=; b=AL6Ax7TUa6WAXFd1mkb0zMdBT5 K+Mh4YKMG8e98iNZ8OO2spSE4u2EfaeNTtwI0QdPJhLjK15ShJ9pJSrnFWdRXjq7tzTVnNEDLpgMh gV123xjkgDhd9d8mJaUk0wWFcW8SVtSGDySB8FrYIgNUhyUp3ZNHnzte4P4SLkD22Rf9DCntf2lMP shRqD2dLAOhgUCbk+Jw6ZtKVNevHNr8I9CS2OQeFfsTjUPj+vIR3KlayWhi7L9ksKqyL6+KEcorCz te3RC+ZMm2eyu2wvoTIQfUfftQagZdA3efXFTbVqlNxx0z+iJxvlGFoZqBdFKKc2aAu8Orzpjahoi iGQgAuXg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qqvCK-0010jl-2X for ath12k@archiver.kernel.org; Thu, 12 Oct 2023 12:53:52 +0000 Received: from mgamail.intel.com ([192.55.52.43]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qqvCG-0010i1-1s; Thu, 12 Oct 2023 12:53:50 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1697115228; x=1728651228; h=date:from:to:cc:subject:in-reply-to:message-id: references:mime-version; bh=gNLNaEec64/5yPf43OhzXjxJ+9f0AwPObPN0gw90rTk=; b=l+KzQ3z0GQLCht0HmiqacGHhpEMtgMdO5q2rCnYomL1KYTbxShJs0zUa h+5o6jAsOs+jcq4yjmFn8OSsA32I1f3sJkuU1HRuXCYN5UuydAY9XG+/q g0o+icxIaCOEvmOZZMxknHpiWRX0Q5REJadAkofO8zObSG4VZiH62DSXy m2bSH2+oxjYLfCv+pw6oaYnN2ZEjXRiZ2/JgSD8lKbQotF9S5OxMN0ZUW 00I3A5HGJIyt6s6j/8RrfxItDzCe3TktTjcLmigG8xsXsaEL/fPaMvgHT c+TViZ8cVXQV9X+EnUTGszKKj7QFRmxEULFp+thRjtCO3VzY0SwpSBlBF Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10861"; a="471169653" X-IronPort-AV: E=Sophos;i="6.03,219,1694761200"; d="scan'208";a="471169653" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Oct 2023 05:53:46 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10861"; a="783688889" X-IronPort-AV: E=Sophos;i="6.03,219,1694761200"; d="scan'208";a="783688889" Received: from asroczyn-mobl.ger.corp.intel.com ([10.249.36.107]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Oct 2023 05:53:41 -0700 Date: Thu, 12 Oct 2023 15:53:39 +0300 (EEST) From: =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= To: Bjorn Helgaas cc: linux-pci@vger.kernel.org, Lorenzo Pieralisi , Rob Herring , =?ISO-8859-2?Q?Krzysztof_Wilczy=F1ski?= , Lukas Wunner , "Rafael J . Wysocki" , 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 05/13] PCI/ASPM: Add pci_enable_link_state() In-Reply-To: <20231011215327.GA1043654@bhelgaas> Message-ID: References: <20231011215327.GA1043654@bhelgaas> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="8323329-1586418908-1697115226=:1692" X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231012_055348_678866_24D31BB7 X-CRM114-Status: GOOD ( 29.48 ) 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-1586418908-1697115226=:1692 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT On Wed, 11 Oct 2023, Bjorn Helgaas wrote: > On Mon, Sep 18, 2023 at 04:10:55PM +0300, Ilpo Järvinen wrote: > > pci_disable_link_state() lacks a symmetric pair. Some drivers want to > > disable ASPM during certain phases of their operation but then > > re-enable it later on. If pci_disable_link_state() is made for the > > device, there is currently no way to re-enable the states that were > > disabled. > > pci_disable_link_state() gives drivers a way to disable specified ASPM > states using a bitmask (PCIE_LINK_STATE_L0S, PCIE_LINK_STATE_L1, > PCIE_LINK_STATE_L1_1, etc), but IIUC the driver can't tell exactly > what changed and can't directly restore the original state, e.g., > > - PCIE_LINK_STATE_L1 enabled initially > - driver calls pci_disable_link_state(PCIE_LINK_STATE_L0S) > - driver calls pci_enable_link_state(PCIE_LINK_STATE_L0S) > - PCIE_LINK_STATE_L0S and PCIE_LINK_STATE_L1 are enabled now > > Now PCIE_LINK_STATE_L0S is enabled even though it was not initially > enabled. Maybe that's what we want; I dunno. > > pci_disable_link_state() currently returns success/failure, but only > r8169 and mt76 even check, and only rtl_init_one() (r8169) has a > non-trivial reason, so it's conceivable that it could return a bitmask > instead. It's great that you suggested this since it's actually what also I've been started to think should be done instead of this straightforward approach I used in V2. That is, don't have the drivers to get anything directly from LNKCTL but they should get everything through the API provided by the disable/enable calls which makes it easy for the driver to pass the same value back into the enable call. > > Add pci_enable_link_state() to remove ASPM states from the state > > disable mask. > > > > Signed-off-by: Ilpo Järvinen > > --- > > drivers/pci/pcie/aspm.c | 42 +++++++++++++++++++++++++++++++++++++++++ > > include/linux/pci.h | 2 ++ > > 2 files changed, 44 insertions(+) > > > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > > index 91dc95aca90f..f45d18d47c20 100644 > > --- a/drivers/pci/pcie/aspm.c > > +++ b/drivers/pci/pcie/aspm.c > > @@ -1117,6 +1117,48 @@ int pci_disable_link_state(struct pci_dev *pdev, int state) > > } > > EXPORT_SYMBOL(pci_disable_link_state); > > > > +/** > > + * pci_enable_link_state - Re-enable device's link state > > + * @pdev: PCI device > > + * @state: ASPM link states to re-enable > > + * > > + * Enable device's link state that were previously disable so the link is > > "state[s] that were previously disable[d]" alludes to the use case you > have in mind, but I don't think it describes how this function > actually works. This function just makes it possible to enable the > specified states. The @state parameter may have nothing to do with > any previously disabled states. Yes, it's what I've been thinking between the lines. But I see your point that this API didn't make it easy/obvious as is. Would you want me to enforce it too besides altering the API such that the states are actually returned from disable call? (I don't personally find that necessary as long as the API pair itself makes it obvious what the driver is expect to pass there.) -- i. --8323329-1586418908-1697115226=:1692 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-1586418908-1697115226=:1692-- 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 smtp3.osuosl.org (smtp3.osuosl.org [140.211.166.136]) (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 A4C95CDB46E for ; Thu, 12 Oct 2023 19:09:12 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp3.osuosl.org (Postfix) with ESMTP id 5CE15610C7; Thu, 12 Oct 2023 19:09:12 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org 5CE15610C7 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=osuosl.org; s=default; t=1697137752; bh=oqsyDOeIddkvDIn/Z4WiHmi/cIKpgS/ai7lTyOhassI=; h=Date:From:To:In-Reply-To:References:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: Cc:From; b=6svjMeoCNYgLR0UL6vDvYWxdGERH4UM6BAv7APDxkKVcFZz9haVH2s8oQ/DbfZOWb SKJAM26zZqlu3xZn6u3ObkR6iN4Cq+l4AiLs6BCXdfkmmeFZHBIqQEU/b1zEu2+p/y rjKopi9vYKv2Ch/MH4TY9rYskH/Mzdu1iShgR1AIRU5dl1sGUDoy7mRrnwhZRuOsbM WZA4NJ00sapOoEkPfvj4ajew6ytNPhOqoYAMJhPmRTzoDsMKOrnrIexWayZJXpVCKL qC5AWaYigcFTF4p+ge44SCUWzNZIdvOMx9gueRWhl6mfNGKF2u599vIoo0zuiOvKMa fylwvMHjTU5/A== X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp3.osuosl.org ([127.0.0.1]) by localhost (smtp3.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id x-o25dMgf5tP; Thu, 12 Oct 2023 19:09:09 +0000 (UTC) Received: from ash.osuosl.org (ash.osuosl.org [140.211.166.34]) by smtp3.osuosl.org (Postfix) with ESMTP id 1B4846134B; Thu, 12 Oct 2023 19:09:09 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org 1B4846134B Received: from smtp1.osuosl.org (smtp1.osuosl.org [140.211.166.138]) by ash.osuosl.org (Postfix) with ESMTP id AFBBB1BF308 for ; Thu, 12 Oct 2023 12:53:51 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id 8765882001 for ; Thu, 12 Oct 2023 12:53:51 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org 8765882001 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 X8J_o1zC-uJi for ; Thu, 12 Oct 2023 12:53:50 +0000 (UTC) Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.43]) by smtp1.osuosl.org (Postfix) with ESMTPS id 2C33282149 for ; Thu, 12 Oct 2023 12:53:49 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org 2C33282149 X-IronPort-AV: E=McAfee;i="6600,9927,10861"; a="471169651" X-IronPort-AV: E=Sophos;i="6.03,219,1694761200"; d="scan'208";a="471169651" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Oct 2023 05:53:46 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10861"; a="783688889" X-IronPort-AV: E=Sophos;i="6.03,219,1694761200"; d="scan'208";a="783688889" Received: from asroczyn-mobl.ger.corp.intel.com ([10.249.36.107]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Oct 2023 05:53:41 -0700 Date: Thu, 12 Oct 2023 15:53:39 +0300 (EEST) From: =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= To: Bjorn Helgaas In-Reply-To: <20231011215327.GA1043654@bhelgaas> Message-ID: References: <20231011215327.GA1043654@bhelgaas> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="8323329-1586418908-1697115226=:1692" X-Mailman-Approved-At: Thu, 12 Oct 2023 19:06:39 +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=1697115230; x=1728651230; h=date:from:to:cc:subject:in-reply-to:message-id: references:mime-version; bh=gNLNaEec64/5yPf43OhzXjxJ+9f0AwPObPN0gw90rTk=; b=P9azlUeX1mDd6T2hMq1h8Nzn4lcRhvLZ3X29PDZQUdMQH1vzAZjpKnaF VdqIi0tWwqWoDdbaYpyjJzfLVno08pwlZAVd04d+dOgDDC+BONWqlssqu f6SqLriq1oor2TSiREeVPMQsDuduuBRw81kK9+tbEEPy3W85Fe5vDMQpn hbap9xuM3wQhp1io64ZY2Taeu+Upf4Sc4aF8JC+YGQ89tJckgnpnfg186 9feFoUhDj81fBl3amwUjcUm71tTg356BPkCiki1w15lIwpuLXiF2Tnrvz U48GSaCjEAoaXd4fShlmgxBop70ru0TnqJ0wVYcIzJYIvLHA8ICrEWMh6 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=P9azlUeX Subject: Re: [Intel-wired-lan] [PATCH v2 05/13] PCI/ASPM: Add pci_enable_link_state() 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 , "Rafael J . Wysocki" , ath12k@lists.infradead.org, linux-pci@vger.kernel.org, linux-wireless@vger.kernel.org, LKML , ath10k@lists.infradead.org, ath11k@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, Rob Herring , 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-1586418908-1697115226=:1692 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT On Wed, 11 Oct 2023, Bjorn Helgaas wrote: > On Mon, Sep 18, 2023 at 04:10:55PM +0300, Ilpo Järvinen wrote: > > pci_disable_link_state() lacks a symmetric pair. Some drivers want to > > disable ASPM during certain phases of their operation but then > > re-enable it later on. If pci_disable_link_state() is made for the > > device, there is currently no way to re-enable the states that were > > disabled. > > pci_disable_link_state() gives drivers a way to disable specified ASPM > states using a bitmask (PCIE_LINK_STATE_L0S, PCIE_LINK_STATE_L1, > PCIE_LINK_STATE_L1_1, etc), but IIUC the driver can't tell exactly > what changed and can't directly restore the original state, e.g., > > - PCIE_LINK_STATE_L1 enabled initially > - driver calls pci_disable_link_state(PCIE_LINK_STATE_L0S) > - driver calls pci_enable_link_state(PCIE_LINK_STATE_L0S) > - PCIE_LINK_STATE_L0S and PCIE_LINK_STATE_L1 are enabled now > > Now PCIE_LINK_STATE_L0S is enabled even though it was not initially > enabled. Maybe that's what we want; I dunno. > > pci_disable_link_state() currently returns success/failure, but only > r8169 and mt76 even check, and only rtl_init_one() (r8169) has a > non-trivial reason, so it's conceivable that it could return a bitmask > instead. It's great that you suggested this since it's actually what also I've been started to think should be done instead of this straightforward approach I used in V2. That is, don't have the drivers to get anything directly from LNKCTL but they should get everything through the API provided by the disable/enable calls which makes it easy for the driver to pass the same value back into the enable call. > > Add pci_enable_link_state() to remove ASPM states from the state > > disable mask. > > > > Signed-off-by: Ilpo Järvinen > > --- > > drivers/pci/pcie/aspm.c | 42 +++++++++++++++++++++++++++++++++++++++++ > > include/linux/pci.h | 2 ++ > > 2 files changed, 44 insertions(+) > > > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > > index 91dc95aca90f..f45d18d47c20 100644 > > --- a/drivers/pci/pcie/aspm.c > > +++ b/drivers/pci/pcie/aspm.c > > @@ -1117,6 +1117,48 @@ int pci_disable_link_state(struct pci_dev *pdev, int state) > > } > > EXPORT_SYMBOL(pci_disable_link_state); > > > > +/** > > + * pci_enable_link_state - Re-enable device's link state > > + * @pdev: PCI device > > + * @state: ASPM link states to re-enable > > + * > > + * Enable device's link state that were previously disable so the link is > > "state[s] that were previously disable[d]" alludes to the use case you > have in mind, but I don't think it describes how this function > actually works. This function just makes it possible to enable the > specified states. The @state parameter may have nothing to do with > any previously disabled states. Yes, it's what I've been thinking between the lines. But I see your point that this API didn't make it easy/obvious as is. Would you want me to enforce it too besides altering the API such that the states are actually returned from disable call? (I don't personally find that necessary as long as the API pair itself makes it obvious what the driver is expect to pass there.) -- i. --8323329-1586418908-1697115226=:1692 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-1586418908-1697115226=:1692-- 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 2DB2ECDB47E for ; Thu, 12 Oct 2023 12:53:50 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1378440AbjJLMxt (ORCPT ); Thu, 12 Oct 2023 08:53:49 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37498 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1343867AbjJLMxs (ORCPT ); Thu, 12 Oct 2023 08:53:48 -0400 Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.43]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3BBA194; Thu, 12 Oct 2023 05:53:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1697115227; x=1728651227; h=date:from:to:cc:subject:in-reply-to:message-id: references:mime-version; bh=gNLNaEec64/5yPf43OhzXjxJ+9f0AwPObPN0gw90rTk=; b=XkJ9DZi5gy3NgX1d7rCxq+rNL5s/H7mbScg9qZtZuE0o2ovQ6o1aTPfT RSkn6+wb8jlENZvCR1cjCzBDMsPR4EVEUqfMche/YjYjxViTWu4FfobjZ CUIaoL0Vn88ucROtDhnXUx8cGrpkNZlqzvBTl+tEYjfJXXv5HwINStMMu 8zBmNOBuAp3ttOy2oERrV+cd3OPVAUXLpZtxRF48y1iThmGNEYQk/nnxp M5gc4C0aQJrhAithjm3hwCNljAEN+5WvwJjIgaNcaMlWVEUrwTo4vvUHc f6Aea66STp3AIMpiHaWybUqw4Q6mkcqqgbb1kzpYS7Qrk5evW4cL4CAl5 w==; X-IronPort-AV: E=McAfee;i="6600,9927,10861"; a="471169655" X-IronPort-AV: E=Sophos;i="6.03,219,1694761200"; d="scan'208";a="471169655" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Oct 2023 05:53:46 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10861"; a="783688889" X-IronPort-AV: E=Sophos;i="6.03,219,1694761200"; d="scan'208";a="783688889" Received: from asroczyn-mobl.ger.corp.intel.com ([10.249.36.107]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Oct 2023 05:53:41 -0700 Date: Thu, 12 Oct 2023 15:53:39 +0300 (EEST) From: =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= To: Bjorn Helgaas cc: linux-pci@vger.kernel.org, Lorenzo Pieralisi , Rob Herring , =?ISO-8859-2?Q?Krzysztof_Wilczy=F1ski?= , Lukas Wunner , "Rafael J . Wysocki" , 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 05/13] PCI/ASPM: Add pci_enable_link_state() In-Reply-To: <20231011215327.GA1043654@bhelgaas> Message-ID: References: <20231011215327.GA1043654@bhelgaas> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="8323329-1586418908-1697115226=:1692" 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-1586418908-1697115226=:1692 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT On Wed, 11 Oct 2023, Bjorn Helgaas wrote: > On Mon, Sep 18, 2023 at 04:10:55PM +0300, Ilpo Järvinen wrote: > > pci_disable_link_state() lacks a symmetric pair. Some drivers want to > > disable ASPM during certain phases of their operation but then > > re-enable it later on. If pci_disable_link_state() is made for the > > device, there is currently no way to re-enable the states that were > > disabled. > > pci_disable_link_state() gives drivers a way to disable specified ASPM > states using a bitmask (PCIE_LINK_STATE_L0S, PCIE_LINK_STATE_L1, > PCIE_LINK_STATE_L1_1, etc), but IIUC the driver can't tell exactly > what changed and can't directly restore the original state, e.g., > > - PCIE_LINK_STATE_L1 enabled initially > - driver calls pci_disable_link_state(PCIE_LINK_STATE_L0S) > - driver calls pci_enable_link_state(PCIE_LINK_STATE_L0S) > - PCIE_LINK_STATE_L0S and PCIE_LINK_STATE_L1 are enabled now > > Now PCIE_LINK_STATE_L0S is enabled even though it was not initially > enabled. Maybe that's what we want; I dunno. > > pci_disable_link_state() currently returns success/failure, but only > r8169 and mt76 even check, and only rtl_init_one() (r8169) has a > non-trivial reason, so it's conceivable that it could return a bitmask > instead. It's great that you suggested this since it's actually what also I've been started to think should be done instead of this straightforward approach I used in V2. That is, don't have the drivers to get anything directly from LNKCTL but they should get everything through the API provided by the disable/enable calls which makes it easy for the driver to pass the same value back into the enable call. > > Add pci_enable_link_state() to remove ASPM states from the state > > disable mask. > > > > Signed-off-by: Ilpo Järvinen > > --- > > drivers/pci/pcie/aspm.c | 42 +++++++++++++++++++++++++++++++++++++++++ > > include/linux/pci.h | 2 ++ > > 2 files changed, 44 insertions(+) > > > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > > index 91dc95aca90f..f45d18d47c20 100644 > > --- a/drivers/pci/pcie/aspm.c > > +++ b/drivers/pci/pcie/aspm.c > > @@ -1117,6 +1117,48 @@ int pci_disable_link_state(struct pci_dev *pdev, int state) > > } > > EXPORT_SYMBOL(pci_disable_link_state); > > > > +/** > > + * pci_enable_link_state - Re-enable device's link state > > + * @pdev: PCI device > > + * @state: ASPM link states to re-enable > > + * > > + * Enable device's link state that were previously disable so the link is > > "state[s] that were previously disable[d]" alludes to the use case you > have in mind, but I don't think it describes how this function > actually works. This function just makes it possible to enable the > specified states. The @state parameter may have nothing to do with > any previously disabled states. Yes, it's what I've been thinking between the lines. But I see your point that this API didn't make it easy/obvious as is. Would you want me to enforce it too besides altering the API such that the states are actually returned from disable call? (I don't personally find that necessary as long as the API pair itself makes it obvious what the driver is expect to pass there.) -- i. --8323329-1586418908-1697115226=:1692-- 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 3801DCDB46E for ; Thu, 12 Oct 2023 12:54: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-Type:MIME-Version: References:Message-ID:In-Reply-To:Subject:cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=5qn837MRiJG+yugrNfgQBMAuhsR3VTEDaAXFRwesxiQ=; b=pwOcG0G/qZL3RVK3Ah8YmV7jYD PNQQ8HTlaZKCTSQiKoxH/E7U5B/vtKxKjvm0R1YCEE2dgLF5EAZVJpLkobMehwTg43U6QiBD9IIlS bIvN7CHg9Jvh4xZASNDA9IK7zKUUd+AZxeMA2eN5KxhXKT0AouE9Cj/Im9UaNumBrkM0R/yGMQ6XQ 8FTx5yXSGZHafxOKaJdqVJ4VgpqHPju57EPZ5NRxKXhynvFQTi9SjxFQcnnDHlIxplRSJl9KNSDHz TAns+XRqwQ2bW7JYWz+wxKfsJSIOn3F1U2z9EZEBs4OLTexOqzM4c0WJkOW7y354pFGybIQ94OD+7 qakrccmw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qqvCL-0010jw-0o; Thu, 12 Oct 2023 12:53:53 +0000 Received: from mgamail.intel.com ([192.55.52.43]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qqvCG-0010i1-1s; Thu, 12 Oct 2023 12:53:50 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1697115228; x=1728651228; h=date:from:to:cc:subject:in-reply-to:message-id: references:mime-version; bh=gNLNaEec64/5yPf43OhzXjxJ+9f0AwPObPN0gw90rTk=; b=l+KzQ3z0GQLCht0HmiqacGHhpEMtgMdO5q2rCnYomL1KYTbxShJs0zUa h+5o6jAsOs+jcq4yjmFn8OSsA32I1f3sJkuU1HRuXCYN5UuydAY9XG+/q g0o+icxIaCOEvmOZZMxknHpiWRX0Q5REJadAkofO8zObSG4VZiH62DSXy m2bSH2+oxjYLfCv+pw6oaYnN2ZEjXRiZ2/JgSD8lKbQotF9S5OxMN0ZUW 00I3A5HGJIyt6s6j/8RrfxItDzCe3TktTjcLmigG8xsXsaEL/fPaMvgHT c+TViZ8cVXQV9X+EnUTGszKKj7QFRmxEULFp+thRjtCO3VzY0SwpSBlBF Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10861"; a="471169653" X-IronPort-AV: E=Sophos;i="6.03,219,1694761200"; d="scan'208";a="471169653" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Oct 2023 05:53:46 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10861"; a="783688889" X-IronPort-AV: E=Sophos;i="6.03,219,1694761200"; d="scan'208";a="783688889" Received: from asroczyn-mobl.ger.corp.intel.com ([10.249.36.107]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Oct 2023 05:53:41 -0700 Date: Thu, 12 Oct 2023 15:53:39 +0300 (EEST) From: =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= To: Bjorn Helgaas cc: linux-pci@vger.kernel.org, Lorenzo Pieralisi , Rob Herring , =?ISO-8859-2?Q?Krzysztof_Wilczy=F1ski?= , Lukas Wunner , "Rafael J . Wysocki" , 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 05/13] PCI/ASPM: Add pci_enable_link_state() In-Reply-To: <20231011215327.GA1043654@bhelgaas> Message-ID: References: <20231011215327.GA1043654@bhelgaas> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="8323329-1586418908-1697115226=:1692" X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231012_055348_678866_24D31BB7 X-CRM114-Status: GOOD ( 29.48 ) 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-1586418908-1697115226=:1692 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT On Wed, 11 Oct 2023, Bjorn Helgaas wrote: > On Mon, Sep 18, 2023 at 04:10:55PM +0300, Ilpo Järvinen wrote: > > pci_disable_link_state() lacks a symmetric pair. Some drivers want to > > disable ASPM during certain phases of their operation but then > > re-enable it later on. If pci_disable_link_state() is made for the > > device, there is currently no way to re-enable the states that were > > disabled. > > pci_disable_link_state() gives drivers a way to disable specified ASPM > states using a bitmask (PCIE_LINK_STATE_L0S, PCIE_LINK_STATE_L1, > PCIE_LINK_STATE_L1_1, etc), but IIUC the driver can't tell exactly > what changed and can't directly restore the original state, e.g., > > - PCIE_LINK_STATE_L1 enabled initially > - driver calls pci_disable_link_state(PCIE_LINK_STATE_L0S) > - driver calls pci_enable_link_state(PCIE_LINK_STATE_L0S) > - PCIE_LINK_STATE_L0S and PCIE_LINK_STATE_L1 are enabled now > > Now PCIE_LINK_STATE_L0S is enabled even though it was not initially > enabled. Maybe that's what we want; I dunno. > > pci_disable_link_state() currently returns success/failure, but only > r8169 and mt76 even check, and only rtl_init_one() (r8169) has a > non-trivial reason, so it's conceivable that it could return a bitmask > instead. It's great that you suggested this since it's actually what also I've been started to think should be done instead of this straightforward approach I used in V2. That is, don't have the drivers to get anything directly from LNKCTL but they should get everything through the API provided by the disable/enable calls which makes it easy for the driver to pass the same value back into the enable call. > > Add pci_enable_link_state() to remove ASPM states from the state > > disable mask. > > > > Signed-off-by: Ilpo Järvinen > > --- > > drivers/pci/pcie/aspm.c | 42 +++++++++++++++++++++++++++++++++++++++++ > > include/linux/pci.h | 2 ++ > > 2 files changed, 44 insertions(+) > > > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > > index 91dc95aca90f..f45d18d47c20 100644 > > --- a/drivers/pci/pcie/aspm.c > > +++ b/drivers/pci/pcie/aspm.c > > @@ -1117,6 +1117,48 @@ int pci_disable_link_state(struct pci_dev *pdev, int state) > > } > > EXPORT_SYMBOL(pci_disable_link_state); > > > > +/** > > + * pci_enable_link_state - Re-enable device's link state > > + * @pdev: PCI device > > + * @state: ASPM link states to re-enable > > + * > > + * Enable device's link state that were previously disable so the link is > > "state[s] that were previously disable[d]" alludes to the use case you > have in mind, but I don't think it describes how this function > actually works. This function just makes it possible to enable the > specified states. The @state parameter may have nothing to do with > any previously disabled states. Yes, it's what I've been thinking between the lines. But I see your point that this API didn't make it easy/obvious as is. Would you want me to enforce it too besides altering the API such that the states are actually returned from disable call? (I don't personally find that necessary as long as the API pair itself makes it obvious what the driver is expect to pass there.) -- i. --8323329-1586418908-1697115226=:1692 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-1586418908-1697115226=:1692--