• Status Closed
  • Percent Complete
  • Task Type Bugs
  • Category Drivers
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Daily build (which?)
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by dreamlayers - 2008-12-28
Last edited by torne - 2009-11-24

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

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.

Closed by  torne
2009-11-24 18:09
Reason for closing:  Fixed
Additional comments about closing:   Warning: Undefined array key "typography" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 371 Warning: Undefined array key "camelcase" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 407


MikeS commented on 2009-03-13 03:51

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?

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.

torne commented on 2009-11-22 11:40

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.


Available keyboard shortcuts


Task Details

Task Editing