LKML Archive on
help / color / mirror / Atom feed
From: Sinan Kaya <>
To: Keith Busch <>, Bjorn Helgaas <>
Cc: Oza Pawandeep <>,
	Bjorn Helgaas <>,
	Philippe Ombredanne <>,
	Thomas Gleixner <>,
	Greg Kroah-Hartman <>,
	Kate Stewart <>,,,
	Dongdong Liu <>, Wei Zhang <>,
	Timur Tabi <>,
	Alex Williamson <>
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: <> (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 [1] 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.

  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:

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

  git send-email \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \

* 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 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).