From nobody Fri Nov 22 22:39:27 2024 Received: from mail-pf1-f174.google.com (mail-pf1-f174.google.com [209.85.210.174]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 310E9183CC7 for ; Fri, 15 Nov 2024 07:24:34 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.210.174 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1731655478; cv=none; b=sXwdpp6amVt0SqxoEocAg1XR30OcYNtYxJGTdIzfj9EQbhjyvHNscf/G+TpxR88+HA7ZCeA7bHzsHXzXbdi7D719GyM2Ph6RTVIbp7Pq80QxGhY1MBd3foAWGcg1H3MixisZDHRzQ2okxI3NES+4WClLZJ1hM05xHO2jlvqSgKI= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1731655478; c=relaxed/simple; bh=NL5xVxsuzvL3vH8bbRzYLv+5EaPNoZfot6E1kNnXNvU=; h=From:To:Cc:Subject:Date:Message-ID:MIME-Version:Content-Type; b=rKMbFUXEgQ3ByOCVZYjC0F5eg/Psl7oA1S3SaZ18FRhYhT6gzDOiUD7iuGskoMUhSQ/xU1/TZ3F/bZ0Z8vZu296cjDT2ysrbpV4JUGVJD7IRWNkaTEEOQAgb6Up39dJhJLils1s5rFUqmDOrHyCNyu/5KOkfRTjR9kiSM3+Kt3U= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=endlessos.org; spf=pass smtp.mailfrom=endlessos.org; dkim=pass (2048-bit key) header.d=endlessos.org header.i=@endlessos.org header.b=OPlKUeEO; arc=none smtp.client-ip=209.85.210.174 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=endlessos.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=endlessos.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=endlessos.org header.i=@endlessos.org header.b="OPlKUeEO" Received: by mail-pf1-f174.google.com with SMTP id d2e1a72fcca58-7240fa50694so380351b3a.1 for ; Thu, 14 Nov 2024 23:24:34 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=endlessos.org; s=google; t=1731655474; x=1732260274; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=Nb+RffXXFUXRR+60cUKB6Fw/MnIHpohg44rBpl2XD8g=; b=OPlKUeEOEtis6//6CEXw3E48G7lFCZbMbfSdhx0CYtcKdwEEXDuUEJSwgA1uko68eo rkP8qqHsLdkd1+NXQtDadI8GLxGdMUih41PuQznU5sRXHfSNS0wFQ6uybzf+ycbww03g Ju28S7Ht9knQE9Ik8d56ucGgYBiFElZMPcPOoeeJeo7fDgwd1UdEXt6MWz8Wz8oSi8IT 0EN4RBa88a/+NOakY5CZRgplszdCiTcJlhg9syuu4NZ9mUbcqFU+SV0IBJ27iiNclHcl B0umfBlnsOOPr5uu0wBVdG39U95Sb0+8dNP6km7BW5SAzJlgNlrdJDt+jk+rBzdMAzu6 U4ng== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1731655474; x=1732260274; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=Nb+RffXXFUXRR+60cUKB6Fw/MnIHpohg44rBpl2XD8g=; b=qZuk2F2Gl7CUS6zYFOWTdoI3AEPx8tSFLzHd4ssXO/P/Jnv1imqzIq5M9zlQxlULYN IyYK18RXwWv93VEiwVWe832caIWaPYgVkRSE9agdYjqf4jtlzmxzVhcZPjk4rRhkFrgN FRTbEwJ6sCtzz0uWyixA5qbEikXlbOY/udxFGuUa6zYDdQ1Hva9Yeg1qKMImyNfDCx/S onHlLB+Pq4qA85oq0IidVY2RaqoOrCDncHG/Aq74+8hjr0eYEYtSFvhk5/8qrhNhNJu8 SRW3Jg5gLvMeOvI+e6hC1Gup7VTC76ozgYAXhclvurGNdIcf4ciaIWy0hExn1oZoDiNH 6vLw== X-Forwarded-Encrypted: i=1; AJvYcCUs4MuT2Z+Zr9krqnVDjPOvLuthCfZ0ocrSYEvA9jnBQtyumgL0dOb96esOTcEaLpoBKbm+LH5Bid7qDck=@vger.kernel.org X-Gm-Message-State: AOJu0YwEw+ltorO8CAXWeZTUgYfpJ/2tSkLbDNrrQNglF72juanECwz8 fnwJYU9V2WVxRghHdCabsuEHRtZ/ELN/1w6eR4Uzmi57OBNHZjQcQoYKT/VJVB8= X-Google-Smtp-Source: AGHT+IE/129tVOmbMdhuBPXaY/T2Fqs6kV2/8QP2wxNSSFmLjdIbTfYG1v8hd30zA7v2zr2y5GBmYA== X-Received: by 2002:a05:6a00:c91:b0:71e:693c:107c with SMTP id d2e1a72fcca58-72476baf03bmr2019990b3a.11.1731655474096; Thu, 14 Nov 2024 23:24:34 -0800 (PST) Received: from localhost.localdomain ([123.51.167.56]) by smtp.googlemail.com with ESMTPSA id d2e1a72fcca58-724771c0d37sm756766b3a.104.2024.11.14.23.24.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 14 Nov 2024 23:24:33 -0800 (PST) From: Jian-Hong Pan To: Bjorn Helgaas , =?UTF-8?q?Krzysztof=20Wilczy=C5=84ski?= Cc: Kuppuswamy Sathyanarayanan , Nirmal Patel , linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, linux@endlessos.org, Jian-Hong Pan , =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= , "David E. Box" Subject: [PATCH v13] PCI/ASPM: Make pci_save_aspm_l1ss_state save both child and parent's L1SS configuration Date: Fri, 15 Nov 2024 15:22:02 +0800 Message-ID: <20241115072200.37509-3-jhp@endlessos.org> X-Mailer: git-send-email 2.47.0 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable PCI devices' parameters on the VMD bus have been programmed properly originally. But, cleared after pci_reset_bus() and have not been restored correctly. This leads the link's L1.2 between PCIe Root Port and child device gets wrong configs. Here is a failed example on ASUS B1400CEAE with enabled VMD. Both PCIe bridge and NVMe device should have the same LTR1.2_Threshold value. However, they are configured as different values in this case: 10000:e0:06.0 PCI bridge [0604]: Intel Corporation 11th Gen Core Processor = PCIe Controller [8086:9a09] (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_Substat= es+ PortCommonModeRestoreTime=3D45us PortTPowerOnTime=3D50us L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1- T_CommonMode=3D0us LTR1.2_Threshold=3D0ns L1SubCtl2: T_PwrOn=3D0us 10000:e1:00.0 Non-Volatile memory controller [0108]: Sandisk Corp WD Blue S= N550 NVMe SSD [15b7:5009] (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_Substat= es+ PortCommonModeRestoreTime=3D32us PortTPowerOnTime=3D10us L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1- T_CommonMode=3D0us LTR1.2_Threshold=3D101376ns L1SubCtl2: T_PwrOn=3D50us Here is VMD mapped PCI device tree: -+-[0000:00]-+-00.0 Intel Corporation Device 9a04 | ... \-[10000:e0]-+-06.0-[e1]----00.0 Sandisk Corp WD Blue SN550 NVMe SSD \-17.0 Intel Corporation Tiger Lake-LP SATA Controller When pci_reset_bus() resets the bus [e1] of the NVMe, it only saves and restores NVMe's state before and after reset. Then, when it restores the NVMe's state, ASPM code restores L1SS for both the parent bridge and the NVMe in pci_restore_aspm_l1ss_state(). The NVMe's L1SS is restored correctly. But, the parent bridge's L1SS is restored with a wrong value 0x0 because the parent bridge's L1SS wasn't saved by pci_save_aspm_l1ss_state() before reset. To avoid pci_restore_aspm_l1ss_state() restore wrong value to the parent's L1SS config like this example, make pci_save_aspm_l1ss_state() save the parent's L1SS config, if the PCI device has a parent. Link: https://lore.kernel.org/linux-pci/CAPpJ_eexU0gCHMbXw_z924WxXw0+B6SdS4= eG9oGpEX1wmnMLkQ@mail.gmail.com/ Closes: https://bugzilla.kernel.org/show_bug.cgi?id=3D218394 Fixes: 17423360a27a ("PCI/ASPM: Save L1 PM Substates Capability for suspend= /resume") Suggested-by: Ilpo J=C3=A4rvinen Signed-off-by: Jian-Hong Pan Reviewed-by: Ilpo J=C3=A4rvinen Reviewed-by: David E. Box --- v9: - Drop the v8 fix about drivers/pci/pcie/aspm.c. Use this in VMD instead. v10: - Drop the v9 fix about drivers/pci/controller/vmd.c - Fix in PCIe ASPM to make it symmetric between pci_save_aspm_l1ss_state() and pci_restore_aspm_l1ss_state() v11: - Introduce __pci_save_aspm_l1ss_state as a resusable helper function which is same as the original pci_configure_aspm_l1ss - Make pci_save_aspm_l1ss_state invoke __pci_save_aspm_l1ss_state for both child and parent devices - Smooth the commit message v12: - Update the commit message v13: - Tweak the commit message to make it more like a general fix - When pci_alloc_dev() prepares the pci_dev, it sets the pci_dev's bus. So, let pci_save_aspm_l1ss_state() access pdev's bus directly. - Add comment in pci_save_aspm_l1ss_state() to describe why it does not save both the PCIe device and the parent's L1SS config like pci_restore_aspm_l1ss_state() directly. drivers/pci/pcie/aspm.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index 28567d457613..0bcd060aab32 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -79,7 +79,7 @@ void pci_configure_aspm_l1ss(struct pci_dev *pdev) ERR_PTR(rc)); } =20 -void pci_save_aspm_l1ss_state(struct pci_dev *pdev) +static void __pci_save_aspm_l1ss_state(struct pci_dev *pdev) { struct pci_cap_saved_state *save_state; u16 l1ss =3D pdev->l1ss; @@ -101,6 +101,22 @@ void pci_save_aspm_l1ss_state(struct pci_dev *pdev) pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL1, cap++); } =20 +void pci_save_aspm_l1ss_state(struct pci_dev *pdev) +{ + struct pci_dev *parent =3D pdev->bus->self; + + __pci_save_aspm_l1ss_state(pdev); + + /* + * Save parent's L1 substate configuration, if the parent has not saved + * state. It avoids pci_restore_aspm_l1ss_state() restore wrong value to + * parent's L1 substate configuration. However, the parent might be + * nothing, if pdev is a PCI bridge. + */ + if (parent && !parent->state_saved) + __pci_save_aspm_l1ss_state(parent); +} + void pci_restore_aspm_l1ss_state(struct pci_dev *pdev) { struct pci_cap_saved_state *pl_save_state, *cl_save_state; --=20 2.47.0