This is the bug/patch tracker for Rockbox. Click here for more information.
Quick links: Bugs · Patches · Rockbox frontpage
FS#9721 - No error check after writes in ata.c
Attached to Project:
Rockbox
Opened by Boris Gjenero (dreamlayers) - Sunday, 28 December 2008, 19:20 GMT+2
Last edited by Torne Wuff (torne) - Tuesday, 24 November 2009, 19:09 GMT+2
Opened by Boris Gjenero (dreamlayers) - Sunday, 28 December 2008, 19:20 GMT+2
Last edited by Torne Wuff (torne) - Tuesday, 24 November 2009, 19:09 GMT+2
|
DetailsIn 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. 
check_errors_after_transfer.patch
(0.5 KiB)
rockbox-19605-writecheck/firmware/drivers/ata.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) |
This task depends upon
Closed by Torne Wuff (torne)
Tuesday, 24 November 2009, 19:09 GMT+2
Reason for closing: Fixed
Additional comments about closing: r23741
Tuesday, 24 November 2009, 19:09 GMT+2
Reason for closing: Fixed
Additional comments about closing: r23741
FS#9721as a bugfix which is acceptable at this point. Do you anticipate any issues or is this just "ATA-by-the-book" stuff?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.
FS#10798. Once it's been tested it can go in and then this can probably go in too.