From: Lukas Wunner <email@example.com> To: Yicong Yang <firstname.lastname@example.org> Cc: Bjorn Helgaas <email@example.com>, Sathyanarayanan Kuppuswamy <firstname.lastname@example.org>, Dan Williams <email@example.com>, Ethan Zhao <firstname.lastname@example.org>, Sinan Kaya <email@example.com>, Ashok Raj <firstname.lastname@example.org>, Keith Busch <email@example.com>, firstname.lastname@example.org, Russell Currey <email@example.com>, Oliver O'Halloran <firstname.lastname@example.org>, Stuart Hayes <email@example.com>, Mika Westerberg <firstname.lastname@example.org>, Linuxarm <email@example.com> Subject: Re: [PATCH] PCI: pciehp: Ignore Link Down/Up caused by DPC Date: Fri, 30 Apr 2021 14:15:49 +0200 [thread overview] Message-ID: <20210430121549.GA10784@wunner.de> (raw) In-Reply-To: <firstname.lastname@example.org> On Fri, Apr 30, 2021 at 04:47:54PM +0800, Yicong Yang wrote: > On 2021/4/30 3:42, Lukas Wunner wrote: > > The 3000 msec were chosen arbitrarily. I couldn't imagine that > > it would ever take longer than that. The spec does not seem to > > mandate a time limit for DPC recovery. But we do need a timeout > > because the DPC Trigger Status bit may never clear and then pciehp > > would wait indefinitely. This can happen if dpc_wait_rp_inactive() > > fails or perhaps because the hardware is buggy. > > The DPC recovery process will not be blocked indefinitely. What about > wait until the DPC process is finished or until the dpc_reset_link() > is finished? We can try to up the link if the DPC recovery failed. As I've indicated above, there's a condition when DPC never completes: According to the PCIe Base Spec, sec. 6.2.10, "After DPC has been triggered in a Root Port that supports RP Extensions for DPC, the Root Port may require some time to quiesce and clean up its internal activities, such as those associated with DMA read Requests. When the DPC Trigger Status bit is Set and the DPC RP Busy bit is Set, software must leave the Root Port in DPC until the DPC RP Busy bit reads 0b." The spec does not mandate a time limit until DPC RP Busy clears: "The DPC RP Busy bit is a means for hardware to indicate to software that the RP needs to remain in DPC containment while the RP does some internal cleanup and quiescing activities. While the details of these activities are implementation specific, the activities will typically complete within a few microseconds or less. However, under worst-case conditions such as those that might occur with certain internal errors in large systems, the busy period might extend substantially, possibly into multiple seconds." Thus, dpc_wait_rp_inactive() polls every 10 msec (for up to HZ, i.e. 1 sec) whether PCI_EXP_DPC_RP_BUSY has been cleared. If it does not clear within 1 sec, the function returns -EBUSY to its caller dpc_reset_link(). Note that according to the spec, we're not allowed to clear the Trigger Status bit as long as the DPC RP Busy bit is set, hence dpc_reset_link() errors out without clearing PCI_EXP_DPC_STATUS_TRIGGER. Because pciehp waits for that bit to clear, it may end up waiting indefinitely for DPC to complete. That's not acceptable, so we do need a timeout to allow pciehp to progress. > I noticed the hotplug interrupt arrives prior to the DPC and then the > wait begins. DPC will clear the Trigger Status in its irq thread. > So not all the time is elapsed by the hardware recovery, but also by > the software process. Considering it's in the irq thread, if we are > preempted and clear the status slower, then we may be timed out. That is correct. If the system is super busy then there's a chance that pciehp may time out because the DPC IRQ thread wasn't scheduled. So the timeout needs to be long enough to accommodate for that. Thanks, Lukas
next prev parent reply other threads:[~2021-04-30 12:15 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-28 8:52 Lukas Wunner 2021-03-30 20:53 ` Kuppuswamy, Sathyanarayanan 2021-04-28 0:39 ` Kuppuswamy, Sathyanarayanan 2021-04-28 1:42 ` Zhao, Haifeng 2021-04-28 10:08 ` Yicong Yang 2021-04-28 14:40 ` Lukas Wunner 2021-04-29 11:29 ` Yicong Yang 2021-04-29 12:40 ` Zhao, Haifeng 2021-04-29 19:42 ` Lukas Wunner 2021-04-30 8:47 ` Yicong Yang 2021-04-30 12:15 ` Lukas Wunner [this message] 2021-04-29 19:36 ` Keith Busch 2021-04-29 20:16 ` Lukas Wunner 2021-04-29 21:16 ` Keith Busch
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=20210430121549.GA10784@wunner.de \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --subject='Re: [PATCH] PCI: pciehp: Ignore Link Down/Up caused by DPC' \ /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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).