Address error and recovery for AER and DPC

drivers/pci/pcie/portdrv_core.c +5 -0

+	device_for_each_child(&dev->dev, &pdrvs, find_service_iter);
+
+	drv = pdrvs.drv;
+	return drv;
+}

drivers/pci/pcie/portdrv_core.c static int remove_iter(struct device *dev, void *data) · line 451

Quoted 5 lines · Oza Pawandeep

+struct pcie_port_service_driver *pcie_port_find_service(struct pci_dev *dev,

drivers/pci/pcie/portdrv_core.c +3 -0

+							u32 service)
+{
+	struct pcie_port_service_driver *drv = NULL;

Remove initialization

drivers/pci/pcie/portdrv_core.c static int remove_iter(struct device *dev, void *data) · line 454

Quoted 3 lines

drivers/pci/pcie/portdrv_core.c +3 -0

+	struct portdrv_service_data pdrvs;
+
+	pdrvs.service = service;

initialize pdrvs.drv = NULL here.

Quoted 5 lines

drivers/pci/pcie/portdrv_core.c +5 -0

+	device_for_each_child(&dev->dev, &pdrvs, find_service_iter);
+
+	drv = pdrvs.drv;
+	return drv;
+}

This patch set brings in error handling support for DPC

The current implementation of AER and error message broadcasting to the EP driver is tightly coupled and limited to AER service driver. It is important to factor out broadcasting and other link handling callbacks. So that not only when AER gets triggered, but also when DPC get triggered (for e.g. ERR_FATAL), callbacks are handled appropriately.

DPC should enumerate the devices after recovering the link, which is achieved by implementing error_resume callback.

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.

Quoted 7 lines · Bjorn Helgaas

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

Quoted 3 lines

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?

Quoted 8 lines · Sinan Kaya

Quoted 7 lines · Bjorn Helgaas

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

Quoted 8 lines

Quoted 3 lines

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. Calling a driver's error handler for the wrong device in an unknown state may have undefined results. Enumerating the slot from scratch should be safe, and will assign resources, tune bus settings, and bind to the matching driver.

Per spec, DPC is the recommended way for handling surprise removal events and even recommends DPC capable slots *not* set 'Surprise' in Slot Capabilities so that removals are always handled by DPC. This service driver was developed with that use in mind.

Quoted 25 lines · Keith Busch

Quoted 8 lines · Sinan Kaya

Quoted 7 lines · Bjorn Helgaas

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

Quoted 8 lines

Quoted 3 lines

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. Calling a driver's error handler for the wrong device in an unknown state may have undefined results. Enumerating the slot from scratch should be safe, and will assign resources, tune bus settings, and bind to the matching driver.

Quoted 4 lines

Per spec, DPC is the recommended way for handling surprise removal events and even recommends DPC capable slots *not* set 'Surprise' in Slot Capabilities so that removals are always handled by DPC. This service driver was developed with that use in mind.

Now it begs the question, that

after DPC trigger

should we enumerate the devices, ?
or
error handling callbacks, followed by stop devices followed by enumeration ? or
error handling callbacks, followed by enumeration ? (no stop devices)

Quoted 33 lines · On Mon, Mar 12, 2018 at 08:16:38PM +0530, [email protected] wrote:

Quoted 25 lines · Keith Busch

Quoted 8 lines · Sinan Kaya

Quoted 7 lines · Bjorn Helgaas

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

Quoted 8 lines

Quoted 3 lines

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. Calling a driver's error handler for the wrong device in an unknown state may have undefined results. Enumerating the slot from scratch should be safe, and will assign resources, tune bus settings, and bind to the matching driver.

Quoted 4 lines

Per spec, DPC is the recommended way for handling surprise removal events and even recommends DPC capable slots *not* set 'Surprise' in Slot Capabilities so that removals are always handled by DPC. This service driver was developed with that use in mind.

Now it begs the question, that

Quoted 7 lines

after DPC trigger

should we enumerate the devices, ?
or
error handling callbacks, followed by stop devices followed by enumeration ? or
error handling callbacks, followed by enumeration ? (no stop devices)

I'm not sure I understand. The link is disabled while DPC is triggered, so if anything, you'd want to un-enumerate everything below the contained port (that's what it does today).

After releasing a slot from DPC, the link is allowed to retrain. If there is a working device on the other side, a link up event occurs. That event is handled by the pciehp driver, and that schedules enumeration no matter what you do to the DPC driver.

Quoted 5 lines · Keith Busch

I'm not sure I understand. The link is disabled while DPC is triggered, so if anything, you'd want to un-enumerate everything below the contained
port (that's what it does today).

Quoted 5 lines

After releasing a slot from DPC, the link is allowed to retrain. If there
is a working device on the other side, a link up event occurs. That event is handled by the pciehp driver, and that schedules enumeration no matter what you do to the DPC driver.

yes, that is what i current, but this patch-set makes DPC aware of error handling driver callbacks.

besides, in absence of pciehp there is nobody to do enumeration.

And, I was talking about pci_stop_and_remove_bus_device() in dpc. if DPC calls driver's error callbacks, is it required to stop the devices ?

Quoted 7 lines

Quoted 5 lines

After releasing a slot from DPC, the link is allowed to retrain. If there
is a working device on the other side, a link up event occurs. That event is handled by the pciehp driver, and that schedules enumeration no matter what you do to the DPC driver.

yes, that is what i current, but this patch-set makes DPC aware of error handling driver callbacks.

I've been questioning the utility of doing that since the very first version of this patch set.

I was just writing a reply to you. You acted first :)

Quoted 10 lines · Keith Busch

Quoted 7 lines

Quoted 5 lines

After releasing a slot from DPC, the link is allowed to retrain. If there
is a working device on the other side, a link up event occurs. That event is handled by the pciehp driver, and that schedules enumeration no matter what you do to the DPC driver.

yes, that is what i current, but this patch-set makes DPC aware of error handling driver callbacks.

I've been questioning the utility of doing that since the very first version of this patch set.

I think we should all agree that shutting down the device drivers with active work is not safe. There could be outstanding work that the endpoint driver needs to take care of.

That was the motivation for this change so that we give endpoint drivers an error callback when something goes wrong.

The rest is implementation detail that we can all figure out.

Quoted 12 lines · Sinan Kaya

Quoted 10 lines · Bjorn Helgaas

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.

Quoted 4 lines

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.

Calling a driver's error handler for the wrong device in an unknown state may have undefined results. Enumerating the slot from scratch should be safe, and will assign resources, tune bus settings, and bind to the matching driver.

Per spec, DPC is the recommended way for handling surprise removal events and even recommends DPC capable slots *not* set 'Surprise' in Slot Capabilities so that removals are always handled by DPC. This service driver was developed with that use in mind.

[+cc Alex]

Quoted 13 lines · Keith Busch

Quoted 12 lines · Sinan Kaya

Quoted 10 lines · Bjorn Helgaas

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.

Quoted 8 lines

Quoted 4 lines

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

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.

Along this line, we had a question a while back about logging AER information after a DPC trigger. Obviously we can't collect any logged information from the downstream devices while link is down, but I noticed the AER status bits are RW1CS, which means they're sticky and are not modified by hot reset or FLR.

So we may be able to read and log the AER information after the DPC driver brings the link back up. We may want to do the same with AER, i.e., reset the downstream devices first, then collect the AER status bits after the link comes back up.

Quoted 4 lines

Calling a driver's error handler for the wrong device in an unknown state may have undefined results. Enumerating the slot from scratch should be safe, and will assign resources, tune bus settings, and bind to the matching driver.

I agree with this; I think this is heading toward doing remove/re-enumerate on AER errors as well because it has also reset the device.

Quoted 4 lines

Per spec, DPC is the recommended way for handling surprise removal events and even recommends DPC capable slots *not* set 'Surprise' in Slot Capabilities so that removals are always handled by DPC. This service driver was developed with that use in mind.

Thanks for this tip. The only thing I've found so far is the mention of Surprise Down triggering DPC in the last paragraph of sec 6.7.5. Are there other references I should look at? I haven't found the recommendation about not setting 'Surprise' in Slot Capabilities yet.

[1] Tangent: I have similar concerns with the device reset paths. We currently save some config state, reset the device, and restore the config state. It's theoretically possible that the device was replaced or came up with different firmware after the reset, so I would really prefer to remove and re-enumerate there, too. But that would make it hard for things up the stack that want to reset the device but not re-setup the whole stack.

I wonder if DPC is going to cause trouble for that scenario. That's not an argument against DPC, but it might be a stronger reason to figure out how to deal with remove/re-enumerate in those stacks.

Quoted 25 lines · Bjorn Helgaas

[+cc Alex]

Quoted 13 lines · Keith Busch

Quoted 12 lines · Sinan Kaya

Quoted 10 lines · Bjorn Helgaas

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 8 lines

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.

Quoted 10 lines

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.

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?

Quoted 23 lines · Sinan Kaya

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.

Quoted 2 lines

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?

I'm on board with this, but I think you mean "Hotplug Capable" rather than "Hotplug Surprise". The former may co-exist with DPC and handles the link-ups, leaving DPC responsible for handling the link-down.

The "Hotplug Surprise" capability is recommended to not co-exist with DPC since that capability may suppress the "surprise link down" notification that DPC needs to see in order to trigger.

If the "Hotplug Capable" bit is not set, detecting the link-up after a DPC event would have to fall under a different component's responsibility, so I think I'm finally seeing your point.

Quoted 11 lines · Bjorn Helgaas

Quoted 4 lines · Keith Busch

Per spec, DPC is the recommended way for handling surprise removal events and even recommends DPC capable slots *not* set 'Surprise' in Slot Capabilities so that removals are always handled by DPC. This service driver was developed with that use in mind.

Thanks for this tip. The only thing I've found so far is the mention of Surprise Down triggering DPC in the last paragraph of sec 6.7.5. Are there other references I should look at? I haven't found the recommendation about not setting 'Surprise' in Slot Capabilities yet.

No problem, it's in the "IMPLEMENTATION NOTE" at the end of 6.2.10.4, "Avoid Disabled Link and Hot-Plug Surprise Use with DPC".

Outside the spec, Microsemi as one of the PCI-SIG contributors and early adopters of the capability published a white paper "Hot and Surprise Plug Recommendations for Enterprise PCIe" providing guidance for OS drivers using DPC. We originally developed to that guidance. The paper unfortunately appears to be pay-walled now. :(

Quoted 10 lines · Oza Pawandeep

This patch set brings in error handling support for DPC

The current implementation of AER and error message broadcasting to the EP driver is tightly coupled and limited to AER service driver. It is important to factor out broadcasting and other link handling callbacks. So that not only when AER gets triggered, but also when DPC get triggered (for e.g. ERR_FATAL), callbacks are handled appropriately.

DPC should enumerate the devices after recovering the link, which is achieved by implementing error_resume callback.

The main point of this series is to call the driver error handling callbacks (error_detected(), mmio_enabled(), slot_reset(), etc.) when DPC events occur. We've always called them for AER events, but prior to this series, we didn't call them for DPC events.

That's a good thing -- we should treat DPC events as much like AER events as possible.

This series does make it more obvious that there's still a big difference between AER and DPC handling -- for DPC, we remove and re-enumerate all the devices, but we don't for AER.

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.

Bjorn