On Thu, 2018-08-16 at 13:26 +0530, poza@xxxxxxxxxxxxxx wrote: > > Please, at the very least revert the spec changes. They are utterly > > wrong. > > > > The driver MUST remain active during the recovery process *including* > > fatal errors. > > > > Only if the recovery fails and the driver gives us may you chose to > > unplug the device (though there is little point). > > > > What you have designed will work fine for network drivers but will not > > work for storage. > > > > Ben. > > Hi Ben, > > If you look at original DPC driver it was doing exactly the same as it > is doing now. > so with or without new changes DPC driver had the same behavior from > beginning. Yes and that behaviour is utterly wrong. > commit 26e515713342b6f7c553aa3c66b21c6ab7cf82af Yup, I noticed. We don't use DPC, all of that is subsumed into our EEH infrastructure, so we didn't notice. But now that the wrongness is spreading it's starting to show, especially the changes to the spec. We shouldn't be too focused on ERR_FATAL vs NON_FATAL. HW has a knack for misreporting these things anyway. All we really care about is whether the device was isolated and data was lost or whether it's just an informational corrected error. The latter should just be reported without impact. The former should lead to a recovery procedure. The design of the error callback was specifially so that you *could* reset the link (or even PERST the whole device & retrain) *without* having to unbind the driver or worse, soft-unplug the device. Specifically beacause once you do the latter, you lose the links to the mounted filesystems and those cannot be re-established. So the spec needs to be reverted, DPC fixed and AER as well to do the right thing here. In fact it would be nice if we could make some of that logic common with EEH. Sam (CC) has been cleaning up some of our EEH code which is, to be honest, a bloody mess, so we might be able to comprehend and refactor it more easily. The basic idea is that regardless of the type of error, you first notify the driver that an error occured. At that point, the io channel might have been frozen already, so the driver can make no expectation that the device is still reachable, which matches what DPC does I believe. The driver can ask for re-enabling the channel if it thinks it can attempt a recovery but the core doesn't have to honor it and can chose to just reset the device. Similarly, in case of re-enabling, the driver can decide that this didn't work out and request an escalation to a reset. So typically, a non-fatal could go through the re-enable phase (in the case where DPC didn't actually block the channel at all), while non- fatal would always go to reset. The only case where we would "unplug" on the linux side is when the driver doesn't have error handling callbacks. Cheers, Ben.
