All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jian-Hong Pan <jhp@endlessos.org>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: "Johan Hovold" <johan@kernel.org>,
	"David Box" <david.e.box@linux.intel.com>,
	"Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>,
	"Kuppuswamy Sathyanarayanan"
	<sathyanarayanan.kuppuswamy@linux.intel.com>,
	"Mika Westerberg" <mika.westerberg@linux.intel.com>,
	"Damien Le Moal" <dlemoal@kernel.org>,
	"Nirmal Patel" <nirmal.patel@linux.intel.com>,
	"Jonathan Derrick" <jonathan.derrick@linux.dev>,
	"Paul M Stillwell Jr" <paul.m.stillwell.jr@intel.com>,
	linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux@endlessos.org, "Jian-Hong Pan" <jhp@endlessos.org>
Subject: [PATCH v8 4/4] PCI/ASPM: Fix L1.2 parameters when enable link state
Date: Fri, 19 Jul 2024 16:02:56 +0800	[thread overview]
Message-ID: <20240719080255.10998-2-jhp@endlessos.org> (raw)
In-Reply-To: <20240719075200.10717-2-jhp@endlessos.org>

Currently, when enable link's L1.2 features with __pci_enable_link_state(),
it configs the link directly without ensuring related L1.2 parameters, such
as T_POWER_ON, Common_Mode_Restore_Time, and LTR_L1.2_THRESHOLD have been
programmed.

This leads the link's L1.2 between PCIe Root Port and child device gets
wrong configs when a caller tries to enabled it.

Here is a failed example on ASUS B1400CEAE with enabled VMD:

10000:e0:06.0 PCI bridge: Intel Corporation 11th Gen Core Processor PCIe Controller (rev 01) (prog-if 00 [Normal decode])
    ...
    Capabilities: [200 v1] L1 PM Substates
        L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+ L1_PM_Substates+
        	  PortCommonModeRestoreTime=45us PortTPowerOnTime=50us
        L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1-
        	   T_CommonMode=45us LTR1.2_Threshold=101376ns
        L1SubCtl2: T_PwrOn=50us

10000:e1:00.0 Non-Volatile memory controller: Sandisk Corp WD Blue SN550 NVMe SSD (rev 01) (prog-if 02 [NVM Express])
    ...
    Capabilities: [900 v1] L1 PM Substates
        L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1- L1_PM_Substates+
                  PortCommonModeRestoreTime=32us PortTPowerOnTime=10us
        L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1-
                   T_CommonMode=0us LTR1.2_Threshold=0ns
        L1SubCtl2: T_PwrOn=10us

According to "PCIe r6.0, sec 5.5.4", before enabling ASPM L1.2 on the PCIe
Root Port and the child NVMe, they should be programmed with the same
LTR1.2_Threshold value. However, they have different values in this case.

Invoke aspm_calc_l12_info() to program the L1.2 parameters properly before
enable L1.2 bits of L1 PM Substates Control Register in
__pci_enable_link_state().

Link: https://bugzilla.kernel.org/show_bug.cgi?id=218394
Signed-off-by: Jian-Hong Pan <jhp@endlessos.org>
---
v2:
- Prepare the PCIe LTR parameters before enable L1 Substates

v3:
- Only enable supported features for the L1 Substates part

v4:
- Focus on fixing L1.2 parameters, instead of re-initializing whole L1SS

v5:
- Fix typo and commit message
- Split introducing aspm_get_l1ss_cap() to "PCI/ASPM: Introduce
  aspm_get_l1ss_cap()"

v6:
- Skipped

v7:
- Pick back and rebase on the new version kernel
- Drop the link state flag check. And, always config link state's timing
  parameters

v8:
- Because pcie_aspm_get_link() might return the link as NULL, move
  getting the link's parent and child devices after check the link is
  not NULL. This avoids NULL memory access.

 drivers/pci/pcie/aspm.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 5db1044c9895..55ff1d26fcea 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1411,9 +1411,15 @@ EXPORT_SYMBOL(pci_disable_link_state);
 static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
 {
 	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
+	u32 parent_l1ss_cap, child_l1ss_cap;
+	struct pci_dev *parent, *child;
 
 	if (!link)
 		return -EINVAL;
+
+	parent = link->pdev;
+	child = link->downstream;
+
 	/*
 	 * A driver requested that ASPM be enabled on this device, but
 	 * if we don't have permission to manage ASPM (e.g., on ACPI
@@ -1428,6 +1434,15 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
 	if (!locked)
 		down_read(&pci_bus_sem);
 	mutex_lock(&aspm_lock);
+	/*
+	 * Ensure L1.2 parameters: Common_Mode_Restore_Times, T_POWER_ON and
+	 * LTR_L1.2_THRESHOLD are programmed properly before enable bits for
+	 * L1.2, per PCIe r6.0, sec 5.5.4.
+	 */
+	parent_l1ss_cap = aspm_get_l1ss_cap(parent);
+	child_l1ss_cap = aspm_get_l1ss_cap(child);
+	aspm_calc_l12_info(link, parent_l1ss_cap, child_l1ss_cap);
+
 	link->aspm_default = pci_calc_aspm_enable_mask(state);
 	pcie_config_aspm_link(link, policy_to_aspm_state(link));
 
-- 
2.45.2


  parent reply	other threads:[~2024-07-19  8:04 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-07-19  7:52 [PATCH v8 0/4] PCI: vmd: Enable PCI PM's L1 substates of remapped PCIe Root Port and NVMe Jian-Hong Pan
2024-07-19  7:55 ` [PATCH v8 1/4] PCI: vmd: Set PCI devices to D0 before enable PCI PM's L1 substates Jian-Hong Pan
2024-07-19  7:57 ` [PATCH v8 2/4] PCI/ASPM: Add notes about enabling PCI-PM L1SS to pci_enable_link_state(_locked) Jian-Hong Pan
2024-07-19  7:59 ` [PATCH v8 3/4] PCI/ASPM: Introduce aspm_get_l1ss_cap() Jian-Hong Pan
2024-07-19  8:02 ` Jian-Hong Pan [this message]
2024-08-02  8:24   ` [PATCH v8 4/4] PCI/ASPM: Fix L1.2 parameters when enable link state Jian-Hong Pan
2024-08-05 18:24     ` Nirmal Patel
2024-08-07  4:23       ` Jian-Hong Pan
2024-08-05 20:26     ` David E. Box
2024-08-07 10:05       ` Jian-Hong Pan
2024-08-07 11:18         ` Ilpo Järvinen
2024-08-07 23:27           ` David E. Box
2024-08-08  9:48             ` Ilpo Järvinen
2024-08-12  8:18               ` Jian-Hong Pan
2024-09-02 15:43                 ` Ilpo Järvinen
2024-09-03 10:31                   ` Jian-Hong Pan
2024-09-23  8:41                   ` Jian-Hong Pan
2024-09-23 13:44                     ` Ilpo Järvinen
2024-09-03 15:17                 ` Nirmal Patel
2024-09-04  3:56                   ` Jian-Hong Pan
2024-09-20 16:03                 ` Nirmal Patel
2024-09-23 10:57                   ` Jian-Hong Pan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240719080255.10998-2-jhp@endlessos.org \
    --to=jhp@endlessos.org \
    --cc=david.e.box@linux.intel.com \
    --cc=dlemoal@kernel.org \
    --cc=helgaas@kernel.org \
    --cc=ilpo.jarvinen@linux.intel.com \
    --cc=johan@kernel.org \
    --cc=jonathan.derrick@linux.dev \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux@endlessos.org \
    --cc=mika.westerberg@linux.intel.com \
    --cc=nirmal.patel@linux.intel.com \
    --cc=paul.m.stillwell.jr@intel.com \
    --cc=sathyanarayanan.kuppuswamy@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.