All of lore.kernel.org
 help / color / mirror / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: stable@vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	patches@lists.linux.dev, Sheng Bi <windy.bi.enflame@gmail.com>,
	Ravi Kishore Koppuravuri <ravi.kishore.koppuravuri@intel.com>,
	Lukas Wunner <lukas@wunner.de>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Mika Westerberg <mika.westerberg@linux.intel.com>,
	Kuppuswamy Sathyanarayanan
	<sathyanarayanan.kuppuswamy@linux.intel.com>
Subject: [PATCH 5.4 58/60] PCI: Unify delay handling for reset and resume
Date: Mon, 20 Mar 2023 15:55:07 +0100	[thread overview]
Message-ID: <20230320145433.342750665@linuxfoundation.org> (raw)
In-Reply-To: <20230320145430.861072439@linuxfoundation.org>

From: Lukas Wunner <lukas@wunner.de>

commit ac91e6980563ed53afadd925fa6585ffd2bc4a2c upstream.

Sheng Bi reports that pci_bridge_secondary_bus_reset() may fail to wait
for devices on the secondary bus to become accessible after reset:

Although it does call pci_dev_wait(), it erroneously passes the bridge's
pci_dev rather than that of a child.  The bridge of course is always
accessible while its secondary bus is reset, so pci_dev_wait() returns
immediately.

Sheng Bi proposes introducing a new pci_bridge_secondary_bus_wait()
function which is called from pci_bridge_secondary_bus_reset():

https://lore.kernel.org/linux-pci/20220523171517.32407-1-windy.bi.enflame@gmail.com/

However we already have pci_bridge_wait_for_secondary_bus() which does
almost exactly what we need.  So far it's only called on resume from
D3cold (which implies a Fundamental Reset per PCIe r6.0 sec 5.8).
Re-using it for Secondary Bus Resets is a leaner and more rational
approach than introducing a new function.

That only requires a few minor tweaks:

- Amend pci_bridge_wait_for_secondary_bus() to await accessibility of
  the first device on the secondary bus by calling pci_dev_wait() after
  performing the prescribed delays.  pci_dev_wait() needs two parameters,
  a reset reason and a timeout, which callers must now pass to
  pci_bridge_wait_for_secondary_bus().  The timeout is 1 sec for resume
  (PCIe r6.0 sec 6.6.1) and 60 sec for reset (commit 821cdad5c46c ("PCI:
  Wait up to 60 seconds for device to become ready after FLR")).
  Introduce a PCI_RESET_WAIT macro for the 1 sec timeout.

- Amend pci_bridge_wait_for_secondary_bus() to return 0 on success or
  -ENOTTY on error for consumption by pci_bridge_secondary_bus_reset().

- Drop an unnecessary 1 sec delay from pci_reset_secondary_bus() which
  is now performed by pci_bridge_wait_for_secondary_bus().  A static
  delay this long is only necessary for Conventional PCI, so modern
  PCIe systems benefit from shorter reset times as a side effect.

Fixes: 6b2f1351af56 ("PCI: Wait for device to become ready after secondary bus reset")
Link: https://lore.kernel.org/r/da77c92796b99ec568bd070cbe4725074a117038.1673769517.git.lukas@wunner.de
Reported-by: Sheng Bi <windy.bi.enflame@gmail.com>
Tested-by: Ravi Kishore Koppuravuri <ravi.kishore.koppuravuri@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Cc: stable@vger.kernel.org # v4.17+
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/pci/pci-driver.c |    4 +--
 drivers/pci/pci.c        |   54 ++++++++++++++++++++---------------------------
 drivers/pci/pci.h        |   10 +++++++-
 3 files changed, 35 insertions(+), 33 deletions(-)

--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -946,7 +946,7 @@ static int pci_pm_resume_noirq(struct de
 	pcie_pme_root_status_cleanup(pci_dev);
 
 	if (!skip_bus_pm && prev_state == PCI_D3cold)
-		pci_bridge_wait_for_secondary_bus(pci_dev);
+		pci_bridge_wait_for_secondary_bus(pci_dev, "resume", PCI_RESET_WAIT);
 
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_resume_early(dev);
@@ -1355,7 +1355,7 @@ static int pci_pm_runtime_resume(struct
 	pci_fixup_device(pci_fixup_resume, pci_dev);
 
 	if (prev_state == PCI_D3cold)
-		pci_bridge_wait_for_secondary_bus(pci_dev);
+		pci_bridge_wait_for_secondary_bus(pci_dev, "resume", PCI_RESET_WAIT);
 
 	if (pm && pm->runtime_resume)
 		rc = pm->runtime_resume(dev);
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4483,7 +4483,7 @@ static int pci_dev_wait(struct pci_dev *
 			return -ENOTTY;
 		}
 
-		if (delay > 1000)
+		if (delay > PCI_RESET_WAIT)
 			pci_info(dev, "not ready %dms after %s; waiting\n",
 				 delay - 1, reset_type);
 
@@ -4492,7 +4492,7 @@ static int pci_dev_wait(struct pci_dev *
 		pci_read_config_dword(dev, PCI_COMMAND, &id);
 	}
 
-	if (delay > 1000)
+	if (delay > PCI_RESET_WAIT)
 		pci_info(dev, "ready %dms after %s\n", delay - 1,
 			 reset_type);
 
@@ -4727,24 +4727,31 @@ static int pci_bus_max_d3cold_delay(cons
 /**
  * pci_bridge_wait_for_secondary_bus - Wait for secondary bus to be accessible
  * @dev: PCI bridge
+ * @reset_type: reset type in human-readable form
+ * @timeout: maximum time to wait for devices on secondary bus (milliseconds)
  *
  * Handle necessary delays before access to the devices on the secondary
- * side of the bridge are permitted after D3cold to D0 transition.
+ * side of the bridge are permitted after D3cold to D0 transition
+ * or Conventional Reset.
  *
  * For PCIe this means the delays in PCIe 5.0 section 6.6.1. For
  * conventional PCI it means Tpvrh + Trhfa specified in PCI 3.0 section
  * 4.3.2.
+ *
+ * Return 0 on success or -ENOTTY if the first device on the secondary bus
+ * failed to become accessible.
  */
-void pci_bridge_wait_for_secondary_bus(struct pci_dev *dev)
+int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type,
+				      int timeout)
 {
 	struct pci_dev *child;
 	int delay;
 
 	if (pci_dev_is_disconnected(dev))
-		return;
+		return 0;
 
 	if (!pci_is_bridge(dev))
-		return;
+		return 0;
 
 	down_read(&pci_bus_sem);
 
@@ -4756,14 +4763,14 @@ void pci_bridge_wait_for_secondary_bus(s
 	 */
 	if (!dev->subordinate || list_empty(&dev->subordinate->devices)) {
 		up_read(&pci_bus_sem);
-		return;
+		return 0;
 	}
 
 	/* Take d3cold_delay requirements into account */
 	delay = pci_bus_max_d3cold_delay(dev->subordinate);
 	if (!delay) {
 		up_read(&pci_bus_sem);
-		return;
+		return 0;
 	}
 
 	child = list_first_entry(&dev->subordinate->devices, struct pci_dev,
@@ -4772,14 +4779,12 @@ void pci_bridge_wait_for_secondary_bus(s
 
 	/*
 	 * Conventional PCI and PCI-X we need to wait Tpvrh + Trhfa before
-	 * accessing the device after reset (that is 1000 ms + 100 ms). In
-	 * practice this should not be needed because we don't do power
-	 * management for them (see pci_bridge_d3_possible()).
+	 * accessing the device after reset (that is 1000 ms + 100 ms).
 	 */
 	if (!pci_is_pcie(dev)) {
 		pci_dbg(dev, "waiting %d ms for secondary bus\n", 1000 + delay);
 		msleep(1000 + delay);
-		return;
+		return 0;
 	}
 
 	/*
@@ -4796,11 +4801,11 @@ void pci_bridge_wait_for_secondary_bus(s
 	 * configuration requests if we only wait for 100 ms (see
 	 * https://bugzilla.kernel.org/show_bug.cgi?id=203885).
 	 *
-	 * Therefore we wait for 100 ms and check for the device presence.
-	 * If it is still not present give it an additional 100 ms.
+	 * Therefore we wait for 100 ms and check for the device presence
+	 * until the timeout expires.
 	 */
 	if (!pcie_downstream_port(dev))
-		return;
+		return 0;
 
 	if (pcie_get_speed_cap(dev) <= PCIE_SPEED_5_0GT) {
 		pci_dbg(dev, "waiting %d ms for downstream link\n", delay);
@@ -4810,14 +4815,11 @@ void pci_bridge_wait_for_secondary_bus(s
 			delay);
 		if (!pcie_wait_for_link_delay(dev, true, delay)) {
 			/* Did not train, no need to wait any further */
-			return;
+			return -ENOTTY;
 		}
 	}
 
-	if (!pci_device_is_present(child)) {
-		pci_dbg(child, "waiting additional %d ms to become accessible\n", delay);
-		msleep(delay);
-	}
+	return pci_dev_wait(child, reset_type, timeout - delay);
 }
 
 void pci_reset_secondary_bus(struct pci_dev *dev)
@@ -4836,15 +4838,6 @@ void pci_reset_secondary_bus(struct pci_
 
 	ctrl &= ~PCI_BRIDGE_CTL_BUS_RESET;
 	pci_write_config_word(dev, PCI_BRIDGE_CONTROL, ctrl);
-
-	/*
-	 * Trhfa for conventional PCI is 2^25 clock cycles.
-	 * Assuming a minimum 33MHz clock this results in a 1s
-	 * delay before we can consider subordinate devices to
-	 * be re-initialized.  PCIe has some ways to shorten this,
-	 * but we don't make use of them yet.
-	 */
-	ssleep(1);
 }
 
 void __weak pcibios_reset_secondary_bus(struct pci_dev *dev)
@@ -4863,7 +4856,8 @@ int pci_bridge_secondary_bus_reset(struc
 {
 	pcibios_reset_secondary_bus(dev);
 
-	return pci_dev_wait(dev, "bus reset", PCIE_RESET_READY_POLL_MS);
+	return pci_bridge_wait_for_secondary_bus(dev, "bus reset",
+						 PCIE_RESET_READY_POLL_MS);
 }
 EXPORT_SYMBOL_GPL(pci_bridge_secondary_bus_reset);
 
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -47,6 +47,13 @@ int pci_bus_error_reset(struct pci_dev *
 #define PCI_PM_D3COLD_WAIT      100
 #define PCI_PM_BUS_WAIT         50
 
+/*
+ * Following exit from Conventional Reset, devices must be ready within 1 sec
+ * (PCIe r6.0 sec 6.6.1).  A D3cold to D0 transition implies a Conventional
+ * Reset (PCIe r6.0 sec 5.8).
+ */
+#define PCI_RESET_WAIT		1000	/* msec */
+
 /**
  * struct pci_platform_pm_ops - Firmware PM callbacks
  *
@@ -107,7 +114,8 @@ void pci_allocate_cap_save_buffers(struc
 void pci_free_cap_save_buffers(struct pci_dev *dev);
 bool pci_bridge_d3_possible(struct pci_dev *dev);
 void pci_bridge_d3_update(struct pci_dev *dev);
-void pci_bridge_wait_for_secondary_bus(struct pci_dev *dev);
+int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type,
+				      int timeout);
 
 static inline void pci_wakeup_event(struct pci_dev *dev)
 {



  parent reply	other threads:[~2023-03-20 15:03 UTC|newest]

Thread overview: 71+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-20 14:54 [PATCH 5.4 00/60] 5.4.238-rc1 review Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 01/60] ext4: fix cgroup writeback accounting with fs-layer encryption Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 02/60] xfrm: Allow transport-mode states with AF_UNSPEC selector Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 03/60] drm/panfrost: Dont sync rpm suspension after mmu flushing Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 04/60] cifs: Move the in_send statistic to __smb_send_rqst() Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 05/60] drm/meson: fix 1px pink line on GXM when scaling video overlay Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 06/60] clk: HI655X: select REGMAP instead of depending on it Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 07/60] docs: Correct missing "d_" prefix for dentry_operations member d_weak_revalidate Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 08/60] scsi: mpt3sas: Fix NULL pointer access in mpt3sas_transport_port_add() Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 09/60] ALSA: hda - add Intel DG1 PCI and HDMI ids Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 10/60] ALSA: hda - controller is in GPU on the DG1 Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 11/60] ALSA: hda: Add Alderlake-S PCI ID and HDMI codec vid Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 12/60] ALSA: hda: Add Intel DG2 " Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 13/60] ALSA: hda: Match only Intel devices with CONTROLLER_IN_GPU() Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 14/60] netfilter: nft_redir: correct value of inet type `.maxattrs` Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 15/60] scsi: core: Fix a comment in function scsi_host_dev_release() Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 16/60] scsi: core: Fix a procfs host directory removal regression Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 17/60] tcp: tcp_make_synack() can be called from process context Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 18/60] nfc: pn533: initialize struct pn533_out_arg properly Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 19/60] ipvlan: Make skb->skb_iif track skb->dev for l3s mode Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 20/60] i40e: Fix kernel crash during reboot when adapter is in recovery mode Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 21/60] qed/qed_dev: guard against a possible division by zero Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 22/60] net: tunnels: annotate lockless accesses to dev->needed_headroom Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 23/60] net: phy: smsc: bail out in lan87xx_read_status if genphy_read_status fails Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 24/60] nfc: st-nci: Fix use after free bug in ndlc_remove due to race condition Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 25/60] net: usb: smsc75xx: Limit packet length to skb->len Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 26/60] nvmet: avoid potential UAF in nvmet_req_complete() Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 27/60] block: sunvdc: add check for mdesc_grab() returning NULL Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 28/60] ipv4: Fix incorrect table ID in IOCTL path Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 29/60] net: usb: smsc75xx: Move packet length check to prevent kernel panic in skb_pull Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 30/60] net/iucv: Fix size of interrupt data Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 31/60] ethernet: sun: add check for the mdesc_grab() Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 32/60] hwmon: (adt7475) Display smoothing attributes in correct order Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 33/60] hwmon: (adt7475) Fix masking of hysteresis registers Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 34/60] hwmon: (xgene) Fix use after free bug in xgene_hwmon_remove due to race condition Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 35/60] hwmon: (ina3221) return prober error code Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 36/60] media: m5mols: fix off-by-one loop termination error Greg Kroah-Hartman
2023-03-20 14:54   ` Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 37/60] mmc: atmel-mci: fix race between stop command and start of next command Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 38/60] jffs2: correct logic when creating a hole in jffs2_write_begin Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 39/60] rust: arch/um: Disable FP/SIMD instruction to match x86 Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 40/60] ext4: fail ext4_iget if special inode unallocated Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 41/60] ext4: fix task hung in ext4_xattr_delete_inode Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 42/60] drm/amdkfd: Fix an illegal memory access Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 43/60] sh: intc: Avoid spurious sizeof-pointer-div warning Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 44/60] ext4: fix possible double unlock when moving a directory Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 45/60] tty: serial: fsl_lpuart: skip waiting for transmission complete when UARTCTRL_SBK is asserted Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 46/60] interconnect: fix mem leak when freeing nodes Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 47/60] tracing: Check field value in hist_field_name() Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 48/60] tracing: Make tracepoint lockdep check actually test something Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 49/60] KVM: nVMX: add missing consistency checks for CR0 and CR4 Greg Kroah-Hartman
2023-03-20 14:54 ` [PATCH 5.4 50/60] ftrace: Fix invalid address access in lookup_rec() when index is 0 Greg Kroah-Hartman
2023-03-20 14:55 ` [PATCH 5.4 51/60] fbdev: stifb: Provide valid pixelclock and add fb_check_var() checks Greg Kroah-Hartman
2023-03-20 14:55 ` [PATCH 5.4 52/60] x86/mm: Fix use of uninitialized buffer in sme_enable() Greg Kroah-Hartman
2023-03-20 14:55 ` [PATCH 5.4 53/60] Revert "treewide: Replace DECLARE_TASKLET() with DECLARE_TASKLET_OLD()" Greg Kroah-Hartman
2023-03-20 14:55 ` [PATCH 5.4 54/60] treewide: Replace DECLARE_TASKLET() with DECLARE_TASKLET_OLD() Greg Kroah-Hartman
2023-03-20 17:40   ` Tom Saeger
2023-03-20 14:55 ` [Intel-gfx] [PATCH 5.4 55/60] drm/i915: Dont use stolen memory for ring buffers with LLC Greg Kroah-Hartman
2023-03-20 14:55   ` Greg Kroah-Hartman
2023-03-20 14:55 ` [PATCH 5.4 56/60] serial: 8250_em: Fix UART port type Greg Kroah-Hartman
2023-03-20 14:55 ` [PATCH 5.4 57/60] s390/ipl: add missing intersection check to ipl_report handling Greg Kroah-Hartman
2023-03-20 14:55 ` Greg Kroah-Hartman [this message]
2023-03-20 14:55 ` [PATCH 5.4 59/60] HID: core: Provide new max_buffer_size attribute to over-ride the default Greg Kroah-Hartman
2023-03-20 14:55 ` [PATCH 5.4 60/60] HID: uhid: Over-ride the default maximum data buffer value with our own Greg Kroah-Hartman
2023-03-20 17:19 ` [PATCH 5.4 00/60] 5.4.238-rc1 review Chris Paterson
2023-03-20 18:46 ` Florian Fainelli
2023-03-20 20:36 ` Naresh Kamboju
2023-03-20 23:14 ` Shuah Khan
2023-03-20 23:37 ` Shuah Khan
2023-03-21  5:07 ` Harshit Mogalapalli
2023-03-21 11:54 ` Jon Hunter

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=20230320145433.342750665@linuxfoundation.org \
    --to=gregkh@linuxfoundation.org \
    --cc=bhelgaas@google.com \
    --cc=lukas@wunner.de \
    --cc=mika.westerberg@linux.intel.com \
    --cc=patches@lists.linux.dev \
    --cc=ravi.kishore.koppuravuri@intel.com \
    --cc=sathyanarayanan.kuppuswamy@linux.intel.com \
    --cc=stable@vger.kernel.org \
    --cc=windy.bi.enflame@gmail.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.