Re: [flashrom] [PATCH] Remove potential endless loops from satasii.c.

[Carl-Daniel Hailfinger]

Am 25.08.2012 04:09 schrieb Stefan Tauner:
> This is based on the idea from the "Make satasii driver more robust" patch
> Signed-off-by: Carl-Daniel Hailfinger
>
> It is missing the BAR access changes, but factors out the wait loop and
> replaces all endless loops instead of just a few.
>
> Signed-off-by: Stefan Tauner

The refactoring is a really good idea and I like it.

Acked-by: Carl-Daniel Hailfinger

(The fixup patch for BAR5 is a separate issue and I have to reinvestiage
what I tried to debug/fix back then.)
> i am not sure if we really want to ignore the hung status register.
> Normally, i would have really aborted on timeouts, but since this was not
> part of the original patch i kept the ignoring behavior.

I don't think we can abort on timeouts. satasii_chip_writeb() returns
void. We would have to change all chip_write[bwl]() and all
chip_read[bwl]() functions to allow for error handling. Not sure if that
is worth it.

Regards,
Carl-Daniel

[PATCH] Remove potential endless loops from satasii.c.

[Stefan Tauner]

On Wed, 29 Aug 2012 01:53:34 +0200
Carl-Daniel Hailfinger  wrote:
> Am 25.08.2012 04:09 schrieb Stefan Tauner:
> > This is based on the idea from the "Make satasii driver more robust" patch
> > Signed-off-by: Carl-Daniel Hailfinger
> >
> > It is missing the BAR access changes, but factors out the wait loop and
> > replaces all endless loops instead of just a few.
> >
> > Signed-off-by: Stefan Tauner
>
> The refactoring is a really good idea and I like it.
>
> Acked-by: Carl-Daniel Hailfinger

thanks!
> (The fixup patch for BAR5 is a separate issue and I have to reinvestiage
> what I tried to debug/fix back then.)

ok i'll leave the fixup patch on patchwork as a reminder for you.
> > i am not sure if we really want to ignore the hung status register.
> > Normally, i would have really aborted on timeouts, but since this was not
> > part of the original patch i kept the ignoring behavior.
>
> I don't think we can abort on timeouts. satasii_chip_writeb() returns
> void. We would have to change all chip_write[bwl]() and all
> chip_read[bwl]() functions to allow for error handling. Not sure if that
> is worth it.

well... we can not report an error back, but we certainly could return
early. i have not even tried to figure out what would happen in that
case though, so i committed this for now (r1588).

another thing that puzzles me a bit is: why do we need to sync with the
status register before AND after each read/write? do other functions
depend on this behavior/a synchronized state?