Rockbox

Tasklist

FS#9721 - No error check after writes in ata.c

Attached to Project: Rockbox
Opened by Boris Gjenero (dreamlayers) - Sunday, 28 December 2008, 18:20 GMT
Last edited by Torne Wuff (torne) - Tuesday, 24 November 2009, 18:09 GMT
Task Type Bugs
Category Drivers
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

In ata_write_sectors/_write_sectors in firmware/drivers/ata.c, the only check at the end is wait_for_end_of_transfer(). That function only checks that BSY, RDY (DRDY) and DRQ. This only makes sure that the command finished, not that it succeeded.

The ATA standard defines normal outputs like these for WRITE SECTOR(S) and other data transfer commands:

Device register -
DEV shall indicate the selected device.
Status register -
BSY shall be cleared to zero indicating command completion.
DRDY shall be set to one.
DF (Device Fault) shall be cleared to zero.
DRQ shall be cleared to zero.
ERR shall be cleared to zero.

I don't see why the device register should be checked. The status register should be checked however:
* BSY because if set, the command isn't done and the other status bits are invalid. Check is probably redundant, but it's free.
* DF because it is set for some errors which don't correspond to error register bits.
* DRQ because if set, more data is left to transfer
* ERR obviously

I don't see how DRDY cleared to zero would imply the current command failed, but the standard says it should be set and the current code checks it, so why not check it.

I'm attaching a simple patch. Note that wait_for_end_of_transfer() is also used at the end of ata_read_sectors/_read_sectors. Errors should be posted at the beginning of READ MULTIPLE blocks and the function checks for them properly. However, I see no harm in checking at the end also.

The read function does retries and the write function doesn't. Some write failures can lead to panicf in firmware/drivers/fat.c. Some users may feel the panic makes things worse. If this becomes a problem I have two ideas:
* Instead of panic, stop doing writes. (Some operating systems remount partitions read-only after some errors.)
* Do retries for writes. I think it would be best to do this by merging read and write functions.
This task depends upon

Closed by  Torne Wuff (torne)
Tuesday, 24 November 2009, 18:09 GMT
Reason for closing:  Fixed
Additional comments about closing:  r23741
Comment by Michael Sevakis (MikeS) - Friday, 13 March 2009, 03:51 GMT
I didn't want to mess with common code during a freeze. I can apply  FS#9721  as a bugfix which is acceptable at this point. Do you anticipate any issues or is this just "ATA-by-the-book" stuff?
Comment by Boris Gjenero (dreamlayers) - Friday, 13 March 2009, 05:10 GMT
This is ATA-by-the-book. Only devices which aren't by-the-book can break it.

I have no concerns about the STATUS_ERR bit. If a device sets that when no error has occurred, the device is hopelessly broken.

STATUS_DF used to mean write error, and the Linux kernel has a "nowerr" parameter for ignoring it. Rockbox uses newer ATA devices which were designed after STATUS_DF was standardized and STATUS_DF is already checked in ata_read_sectors, so I don't expect new problems.

I'm only thing I'm concerned about is the lack of write retries. A write failure due to shaking could lead to panicf in fat.c.
Comment by Torne Wuff (torne) - Sunday, 22 November 2009, 11:40 GMT
I have written a patch to unify reading and writing in ata.c:  FS#10798 . Once it's been tested it can go in and then this can probably go in too.

Loading...