On Wed, Jan 27, 2016 at 05:38:28PM +0100, Paolo Bonzini wrote:
On 27/01/2016 16:57, Kevin O'Connor wrote:
On Wed, Jan 27, 2016 at 03:51:02PM +0100, Paolo Bonzini wrote:
- for (;;) {
u32 istatus = inl(iobase + MPT_REG_ISTATUS);if (istatus & MPT_IMASK_REPLY) {u32 resp = inl(iobase + MPT_REG_REP_Q);/* another read to turn interrupt off */inl(iobase + MPT_REG_REP_Q);if (resp == MPT_CONTEXT_MAGIC) {return DISK_RET_SUCCESS;} else if (resp & 0x80000000) {outl((u32)&reply_msg[0], iobase + MPT_REG_REP_Q);return DISK_RET_EBADTRACK;}}usleep(50);- }
This could loop forever on a hardware error. I know other SeaBIOS drivers do this as well, but I'd prefer if new code would have some eventual timeout (eg, 30 seconds) and fail the request.
It's a bit more complicated than that; the request is outstanding and I cannot send another one until it completes. So I would have to do one of two things:
- reset the adapter, which would probably not have a timeout so that in
the end there is no difference from the current code;
- bring the disk offline entirely in the case of a timeout, which can
possibly be a cure worse than the diseases.
So for something as simple as INT 13h I think anything more complex than such a loop has no actual benefit. Of course that would be different in a more complex environment.
Oh, I understand and agree that recovery isn't worth while. My concern is that a hardware error here will appear as a silent hang. Breaking out of the loop eventually, calling warn_timeout(), and returning an error code has the benefit of some debugging from seabios and likely some strong error messages from the calling app.
As I'm more interested in the debugging then the recovery, a simple addition like this would be an improvement IMO:
u32 end = calc_timeout(30000); // 30 second max timeout for (;;) { ... if (timer_check(end)) { warn_timeout(); return DISK_RET_ECONTROLLER; } usleep(50); }
-Kevin