FS#9721 - No error check after writes in ata.c
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
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.
Tuesday, 24 November 2009, 18:09 GMT
Reason for closing: Fixed
Additional comments about closing: r23741