From: Mike Holden <rockbox_at_mikeholden.uklinux.net>
Date: Thu, 13 Mar 2003 15:10:51 -0000 (GMT)

Björn Stenberg said:
> Mike Holden wrote:
>> Basically in file.c:readwrite(), if we read some head
>> data (cached from an earlier disk read), then we should return that
>> data successfully, even if we subsequently get a disk error. At the
>> moment, we return < 0, even though we have put data in the buffer.
>
> Yes, the code treats read errors atomically, i.e. any error means the
> entire request failed and must be retried. This is a design decision.
> The result buffer is only valid if the return code >= 0.
>
> You are right that there is a bug in that the next successful read()
> call will not return those head bytes that were copied by the previous,
> failed, call. But this will result in up to 511 bytes missing from the
> file, which will produce a short "blip" in the playback, not freeze it.
>

I note you have updated the code, even though I didn't submit the patch.
Your patch was identical to the one I would have submitted, though!

>> There is also another problem, which I believe is probably in the
>> ata_read_sectors() code. I have not yet debugged this (not easy!), but
>> I suspect that the hardware is reading less sectors than we requested,
>> but has still read some sectors.
>
> Yes, that is possible. Errors can occur at any of the requested sectors.
>
>> We are assuming we have read it all
>
> Not if there is an error. On error, we assume nothing has been read and
> submit the complete read request again.
>

My most usual problem when experiencing skip problems on the move is that
the mpeg layer thinks it has got valid data back, and continues to play,
but the data being "played" is blank.

I have been concentrating on ata_read_sectors(), and believe I may have
spotted a race condition in there, in that in certain circumstances around
disk skips, some data will be read successfully, but the code returns that
it has read all the requested data. I will try to explain, but this is
from code examination, rather than actual debugging! Also, the return
codes are a little confusing at times, hence I may have got lost
somewhere, so please bear with me! We sometimes return 0 for success,
sometimes for error, but often use "if (somefunction())" type constructs,
which can be misleading, since the value being tested may not follow the
usual tradition of 0=true, anything else=false.

1. We enter ata_read_sector() and read some data successfully.
2. Mpeg data is read into the buffer.
3. We hit a skip error, and go through the perform_soft_reset() branch.
4. perform_soft_reset() returns 0, which will occur given certain conditions.
5. This takes us through the break;, which exits the while(count) loop.
6. This takes us through the wait_for_end_of_transfer() test.
7. Again the route we take depends on what happens to the disk!
8. If we do not go through the body of the if() statement, we drop out of
the bottom, set the "success" code to 0, break; out of the timeout loop,
clear the led and lock, and return success (0) to the higher level
functions.

This results in the assumption that all the data has been read, when in
fact we may in fact have only read one sector. We then get "blanks" in the
buffer, which the mpeg code "plays", and gives silence.

I'm not too sure of the intended logic around the soft reset stuff, so
haven't got a patch to submit for this one, but it certainly looks like a
certain set of disk error and recovery conditions can result in a
partially blank buffer being returned as success.

-- 
Mike Holden

Received on 2003-03-13