PCI Error Recovery (readX_check)

(I've forgotten to cc-ing, so send again.)

Hi, all

This is a request for comments about "PCI error recovery."

Some little time (six months!) ago, we had a discussion about this topic, and at the last we came to the conclusion that "check the bridge status." Based on this (Linus's idea), I have reconsider the design and refine the implementation. I'd really appreciate it if you could find a problem of the following ideas and could feedback it to me.

-----

At first, again, my goal is:
"Enable some possible error recovery on drivers by error notification."

Today, errors on some type of transaction such as:
- memory mapped I/O write
- DMA from device to memory
- DMA from memory to device
are recoverable because the error (ex. parity error) is visible on the status register of the device.

However, in the case of memory mapped I/O read, the result of the transaction is not visible on the device. Whether there had a error or not is visible on bus bridges locates upper the device.

We have to be careful since there are some platforms that host bus bridges couldn't be used for recovery (ex. host isn't PCI device.)
If so, we have to use a PCI-to-PCI bridge just under the host bridge instead of using the host bridge.

Put simply, we need to check the status of the "highest" bus bridge only in the case of mmio read.

And, the status of the highest bridge is shared by some other devices, so clearing the status from one device could affect checking from the other device. Therefore, to check the status of the highest bridge, we should handle the status from out of device drivers, never in each drivers.

So what I have to consider next is the implementation of notification on mmio read which we should check the highest bridge status.

---

Okay, let's talk about the actual implementation :-)

The requirement is:
"Guarantee the result of mmio read on error while multiple mmio read
by devices under same bridge is running."

To realize this, satisfy all of 1 to 3:

#1:
We have to know which device under the same bridge doing mmio read.

Add a list "working_device" on the pci_dev struct to check the running device. Register the device to the highest bridge device at the beginning of a session containing some mmio reads, and unregister the device at the end of the session.

#2:
Clear the bridge status when no devices under the bridge do mmio read.

Take a lock during a mmio read which could change the status, and during a clearing the status. Logically, rwlock is convenient.
- Processing mmio read: read_lock
- Clearing the status: write_lock
To reduce more load on this lock, check the status before clear it, and skip clearing if the status have no error.

#3:
Send the error status to all drivers in session before clear it.

There is no way to know which device cause a error if there are multiple devices doing mmio read. (Using spinlock instead of rwlock is a possible way, but clearly it would impact on I/O performance, so I ignore the way.) Thus, the best we can do now is send the error to all concerning driver. I mean, notify the error to all devices registered to the "working_device" list of the highest bridge, by updating "err_status" value which newly added to the pci_dev struct. Note that we should take the write_lock from the updating "err_status" to the clearing the bridge status. Or the result of I/O between the updating and the clearing would vanish into the night.

---

I suppose the basic mmio read of RAS-aware driver as like as following:

====================================
int retries = 2;

do {
clear_pci_errors(dev); /* clear bridge status */
val = readX_check(dev, addr); /* memory mapped I/O read */
status = read_pci_errors(dev); /* check bridge status */
} while (status && --retries > 0);
if (status)

	/* error */

====================================

The basic design of new-found functions are:

$1. clear_pci_errors(DEVICE)
- find the highest(host or top PCI-to-PCI) bridge of DEVICE
- check the status of the highest bridge, and if it indicates error(s):
- write_lock
- update each err_status of all devices registered to the working_device list of the highest bridge, by "|=" the value of the bridge status
- clear the bridge status
- write_unlock
- clear the err_status of DEVICE
- register DEVICE to the highest bridge

$2. readX_check(DEVICE, ADDR)
- read_lock
- I/O (read)
- read_unlock

$3. read_pci_errors(DEVICE)
- find the highest bridge of DEVICE
- store the status of the highest bridge as STATUS
- check ( STATUS | DEVICE->err_status )
- return 1 if error (ex. Master/Target Abort, Party Error), else return 0

Note:
This time, here is no initialize for the control register of the highest bridge. The generic initialization could be implemented in the code, but the values are user configurable and occasionally some bus needs specific value, so now I don't write it yet.

Thanks,
H.Seto

I'd suggest changing the locking a bit.

Just make "clear_pci_errors()" take a spinlock on the bridge, and "read_pci_errors()" unlock it. We need to make sure that if multiple devices on the same bridge try to be careful, they can do so without seeing each others errors.

readX_check() itself would do no locking at all, since it is already called with the assumption that the bridge has been locked.

I'd also suggest that you make "clear_pci_errors()" return a cookie for read_pci_errors() to use.

Also, I assume that the thing would support (and please make the documentation clear on it) multiple IO operations between a "clear_pci_errors()" and it's ending "read_pci_errors()" pair. I can well imagine that a driver might do

	int get_data_buffer(struct pci_dev *dev, u32 *buf, int words)
	{
		int i;
		pci_error_t cookie;
		unsigned long offset = pci_resource_start(dev, 0) + DATA_OFFSET;

		cookie = clear_pci_errors(dev);

		/* Get the whole packet of data.. */
		for (i = 0; i < words; i++)
			*buf++ = readl_check(dev, offset);

		/* Did we have any trouble? */
		if (read_pci_errors(dev, cookie))
			return -EIO;

		/* All systems go.. */
		return 0;
	}

to read a "packet" of data from a device. Whatever.

Linus

... Why spinlock?
Are rwlocks not smart way to decrease the impact on I/O performance?

What I can only imagine is... passing somthing like a identifier of looking bridge to driver as cookie, functionally, it's sounds good. ... Are there any other useful usages of the cookie?

Sure.
So taking a spinlock between this pair clearly means long time locking on I/O, this will block all other I/O under same bridge, I think this isn't good situation. Still do we take a spinlock?

Thanks,
H.Seto

Well, I'm not sure about all this... part of the problem is that drivers commonly need to also do IOs from interrupts. And another driver may "pollute" us too, depending on how the HW & bridge are designed. So we really also want to disable interrupts, we may need a "flags" around (could be burried into the cookie stuff though as an arch specific thing)

Most drivers already have such a low level lock though, so we may end up replacing it with a bridge-based lock... but depending on the architecture, that would end up sync'ing lots of drivers on the same lock, which may not be good especially if we have no checking to do...

I don't know what is the best thing to do here... The arch is the one to know what is the granularity of the error management (per slot ? per segment or per domain ?) and so to know what kind of lock is needed...

Ben.

Good point. I believe you _do_ end up having to do that, like it or not.

Because if you don't lock the bridge (or whatever the entity is that keeps track of errors), the whole exercise is kind of pointless. If two drivers try to do error checking at the same time, and will potentially clear the errors of each other, causing the errors to get lost, the whole recovery infrastructure is clearly worthless.

Some serialization will happen. Inevitable. See above.

The "good news" is that I doubt very many drivers will care enough to do this. I suspect you'll only have a few very specific drivers used in fault-tolerant circumstances, where you care more about the errors than about the inevitable serialization.

It will have to depend on the bus setup. Not arch-specific per se, but clearly specific to the bus controllers in question.

Linus

Do we only need to determine there was an error in the IO hierarchy or do we also need to know which device/driver caused the error?

If the latter I agree with linus. If the former, then the error recovery can support asyncronous errors (like the bad DMA address case) and tell all affected (thanks willy) drivers.

grant

Yes, that we could do without locking. The simplest way to do that is to have a global sequence counter, and then the whole thing really boils down to

	typedef unsigned long pci_error_cookie_t;

	extern pci_error_cookie_t error_sequence_number;

	static inline pci_error_cookie_t clear_pci_errors(struct pci_dev *dev)
	{
		pci_error_cookie_t now;

		now = error_sequence_number;
		io_start_error_memory_barrier(dev);
		return now;
	}

	static inline int read_pci_errors(pci_error_cookie_t then, struct pci_dev *dev)
	{
		io_end_error_memory_barrier(dev);
		return (then != error_sequence_number) ? -EIO : 0;
	}

and have the error handler just increment the "error_sequence_number" whenever it happens.

However, the above relies on the PCI error being a NMI in the first place (which it may or may not be), and also on the fact that we need to have some way to make sure we get it if it was pending to avoid races (ie the "io_end_error_memory_barrier()" may have to be pretty expensive and bus-serializing - likely a config space read from the device).

And the above also relies on it being ok to see other peoples errors by mistake.

(Depending on how much information such a PCI error NMI gives the kernel, the "error_sequence_number" could be made per-domain or per-bus, of course, but that's an implementation detail that depends on what the hardware supports).

But I would not be surprised if "clear_pci_errors()" actually has to _clear_ some bit in the bridge device and "read_pci_errors()" has to check the bit afterwards. And if that is the case, I really do believe that you want to lock the whole bridge, because you can't have people clearing the bit when somebody else might be actively using it...

So I think we have to design for the thing potentially being a irq-safe spinlock - possibly even a global one. That's the worst case, and maybe a lot of hardware platforms can do less intrusive things, but if we're looking at generic infrastructure that different drivers are supposed to be able to use (and I assume that's what we want), then we have to make the interfaces generic.

Linus

Yup, but then, the user have to take care that behind a single "error checking" entity (a bridge for example), all devices have such drivers that honor the bridge-level locking and not their own.

On ppc64, I think we always have 1 bridge = 1 slot though, makes things easier (well, provided we don't start to try playing with error coming from slots on the g5).

Right.

Ben.

Yes. I suspect that this all matters in places where you have pretty strict hardware controls anyway, so it's likely not a big deal.

Yes, I'd assume that most high-end hardware (ie the kind that people have who care about these things in the first place) wants to minimize the number of shared error reporting bridges anyway.

Linus

On Wed, Aug 25, 2004 at 05:01:08PM +1000, Benjamin Herrenschmidt wrote:
...

multiple drivers acquiring the same bridge lock? ugh.

Which bridge sees an error may be a parent (or child) of the PCI bridge we are monitoring. I suspect we will have to live with multiple devices being impacted by errors on a bus and the error recovery notify/resyncronize with all impacted devices.

Does anyone expect to recover from devices attempting unmapped DMA? Ie an IOMMU which services multiple PCI busses getting a bad DMA address will cause the next MMIO read by any of the (grandchildren) PCI devices to see an error (MCA on IA64). I'm asking only to determine if this is outside the scope of what the PCI error recovery is trying to support.

Yeah...I guess my comments are along the same vein.

grant