LKML Archive on lore.kernel.org help / color / mirror / Atom feed
From: Sinan Kaya <firstname.lastname@example.org> To: Keith Busch <email@example.com>, Bjorn Helgaas <firstname.lastname@example.org> Cc: Oza Pawandeep <email@example.com>, Bjorn Helgaas <firstname.lastname@example.org>, Philippe Ombredanne <email@example.com>, Thomas Gleixner <firstname.lastname@example.org>, Greg Kroah-Hartman <email@example.com>, Kate Stewart <firstname.lastname@example.org>, email@example.com, firstname.lastname@example.org, Dongdong Liu <email@example.com>, Wei Zhang <firstname.lastname@example.org>, Timur Tabi <email@example.com>, Alex Williamson <firstname.lastname@example.org> Subject: Re: [PATCH v12 0/6] Address error and recovery for AER and DPC Date: Mon, 12 Mar 2018 23:47:12 -0400 [thread overview] Message-ID: <email@example.com> (raw) In-Reply-To: <20180312232626.GI18494@localhost.localdomain> On 3/12/2018 7:26 PM, Keith Busch wrote: > On Mon, Mar 12, 2018 at 02:47:30PM -0500, Bjorn Helgaas wrote: >> [+cc Alex] >> >> On Mon, Mar 12, 2018 at 08:25:51AM -0600, Keith Busch wrote: >>> On Sun, Mar 11, 2018 at 11:03:58PM -0400, Sinan Kaya wrote: >>>> On 3/11/2018 6:03 PM, Bjorn Helgaas wrote: >>>>> On Wed, Feb 28, 2018 at 10:34:11PM +0530, Oza Pawandeep wrote: >>>> >>>>> That difference has been there since the beginning of DPC, so it has >>>>> nothing to do with *this* series EXCEPT for the fact that it really >>>>> complicates the logic you're adding to reset_link() and >>>>> broadcast_error_message(). >>>>> >>>>> We ought to be able to simplify that somehow because the only real >>>>> difference between AER and DPC should be that DPC automatically >>>>> disables the link and AER does it in software. >>>> >>>> I agree this should be possible. Code execution path should be almost >>>> identical to fatal error case. >>>> >>>> Is there any reason why you went to stop driver path, Keith? >>> >>> The fact is the link is truly down during a DPC event. When the link >>> is enabled again, you don't know at that point if the device(s) on the >>> other side have changed. >> >> When DPC is triggered, the port takes the link down. When we handle >> an uncorrectable (nonfatal or fatal) AER event, software takes the >> link down. >> >> In both cases, devices on the other side are at least reset. Whenever >> the link goes down, it's conceivable the device could be replaced with >> a different one before the link comes back up. Is this why you remove >> and re-enumerate? (See tangent  below.) > > Yes. Truthfully, DPC events due to changing topologies was the motivating > use case when we initially developed this. We were also going for > simplicity (at least initially), and remove + re-enumerate seemed > safe without concerning this driver with other capability regsiters, or > coordinating with/depending on other drivers. For example, a successful > reset may depend on any particular driver calling pci_restore_state from > a good saved state. The spec is recommending code to use "Hotplug Surprise" to differentiate these two cases we are looking for. The use case Keith is looking for is for hotplug support. The case I and Oza are more interested is for error handling on platforms with no hotplug support. According to the spec, if "Hotplug Surprise" is set in slot capabilities, then hotplug driver handles link up and DPC driver doesn't interfere with its operation. Hotplug driver observes link up interrupt like it is doing today. When link up event is observed, hotplug driver will do the enumeration. If "Hotplug Surprise" bit is not set, it is the job of the DPC driver to bring up the link. I believe this path should follow the AER driver path as there is a very well defined error reporting and recovery framework in the code. The link comes back up automatically when DPC driver handles its interrupt very similar to what secondary bus reset does for AER. I don't believe there is a hotplug possibility under this condition since it is not supported to begin with. Should we plumb the "Hotplug Surprise" condition into the code to satisfy both cases and leave the error handling path according to this code series? > >> The point is that from the device's hardware perspective, these >> scenarios are the same (it sent a ERR_NONFATAL or ERR_FATAL message >> and it sees the link go down). I think we should make them the same >> on the software side, too: the driver should see the same callbacks, >> in the same order, whether we're doing AER or DPC. >> >> If removing and re-enumerating is the right thing for DPC, I think >> that means it's also the right thing for AER. >> -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
next prev parent reply other threads:[~2018-03-13 3:47 UTC|newest] Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-02-28 17:04 [PATCH v12 0/6] Address error and recovery for AER and DPC Oza Pawandeep 2018-02-28 17:04 ` [PATCH v12 1/6] PCI/AER: Rename error recovery to generic PCI naming Oza Pawandeep 2018-02-28 17:04 ` [PATCH v12 2/6] PCI/AER: Factor out error reporting from AER Oza Pawandeep 2018-02-28 17:04 ` [PATCH v12 3/6] PCI/PORTDRV: Implement generic find service Oza Pawandeep 2018-03-06 14:02 ` Sinan Kaya 2018-03-08 7:56 ` poza 2018-02-28 17:04 ` [PATCH v12 4/6] PCI/DPC: Unify and plumb error handling into DPC Oza Pawandeep 2018-02-28 17:04 ` [PATCH v12 5/6] PCI: Unify wait for link active into generic PCI Oza Pawandeep 2018-02-28 17:04 ` [PATCH v12 6/6] PCI/DPC: Enumerate the devices after DPC trigger event Oza Pawandeep 2018-03-11 22:03 ` [PATCH v12 0/6] Address error and recovery for AER and DPC Bjorn Helgaas 2018-03-12 3:03 ` Sinan Kaya 2018-03-12 14:25 ` Keith Busch 2018-03-12 14:46 ` poza 2018-03-12 14:58 ` Keith Busch 2018-03-12 15:34 ` poza 2018-03-12 17:33 ` Keith Busch 2018-03-12 17:41 ` Sinan Kaya 2018-03-12 17:56 ` Keith Busch 2018-03-12 19:47 ` Bjorn Helgaas 2018-03-12 23:26 ` Keith Busch 2018-03-13 3:47 ` Sinan Kaya [this message] 2018-03-14 20:50 ` Keith Busch 2018-03-14 21:00 ` Sinan Kaya 2018-05-08 19:25 ` Bjorn Helgaas 2018-03-12 14:01 ` poza
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 \ --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 \ --firstname.lastname@example.org \ --email@example.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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).