[SeaBIOS] [PATCH] Support for booting from LSI Logic LSI53C1030, SAS1068, SAS1068e

[Kevin O'Connor]

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:
> 
> 1) reset the adapter, which would probably not have a timeout so that in
> the end there is no difference from the current code;
> 
> 2) 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