Rockbox

  • Status Closed
  • Percent Complete
    100%
  • 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 sideral - 2010-11-24
Last edited by Frank Gevaerts - 2010-11-28

FS#11774 - Unmount all disks before entering USB slave mode to prevent filesystem corruption

I recently enabled Rockbox-native USB slave mode on my Sansa ClipV2 through the patch provided in  FS#11664  - Workaround for usb random failures on amsv2. Ever since then, I have had very stable USB connections, but regular filesystem corruption on the player’s flash drive. This had never happened before, when I still was had to boot into the OF to use its USB support.

As I allow Rockbox to write to my disk for various reasons, such as scrobbler logging and database statistics updates. Also, my flash drive is fairly full, leading to frequent disk-block reallocation. Thus I suspected that either (1) the filesystem wasn’t fully flushed when USB slave mode was entered, or (2) that Rockbox was using stale filesystem data after returning from USB slave mode that would later lead to filesystem corruption. I slightly lean towards explanation 2, because I never observed filesystem corruption when powering off Rockbox (to use the OF’s USB support) even though the shutdown code does not unmount the disks either, AFAIK.

As an experiment, I added support for unmounting all disks before entering USB slave mode – see attached patch. I’ve been running with this patch for a week now, and I haven’t seen any filesystem corruption since then.

Comments?

Closed by  Frank Gevaerts
2010-11-28 15:25
Reason for closing:  Accepted
Additional comments about closing:  

Committed as r28693

amaury pouly commented on 2010-11-24 15:48

That seems rather strange, I’ll have a look at it but I’m pretty sure firmware/usb.c already does something like this.
If it doesn’t, then there might be a problem like a file handle not released (typically a font handle) which might eventually need to crash but I think we solved that issue quite some time ago.

Last thing which is bothering me: usb slave is mode for devices which have a hardware bridge and thus which don’t handle usb in software. As far as I know, this is not the case of the clipv2 thus I ‘m a bit confused by your patch !

Admin
Frank Gevaerts commented on 2010-11-24 17:06

I think this is a good idea. Why do you basically reimplement disk_unmount() inside disk_unmount_all() though, instead of just calling disk_unmount()?

sideral commented on 2010-11-25 00:14

pamaury, I think fg answered your questions already on IRC (http://www.rockbox.org/irc/log-20101124#17:00), so let me go straight to his question:

fg writes:
> Why do you basically reimplement disk_unmount() inside
> disk_unmount_all() though, instead of just calling disk_unmount()?

The implementation is subtly different (look at the “if (voldrive…)” line). Yes, this probably could be factored better, but I cannot just call disk_unmount because that function expects a drive ID and not a volume ID. Calling this function means that we’d have to go through the vol_drive array twice for each drive.

Suggestion: Keep the loops in these function as is and factor out the inner loop’s body. OK?

amaury pouly commented on 2010-11-25 12:15

I don’t see the problem. You can loop on all drives and call disk_umount. Something like:

#ifndef HAVE_MULTIDRIVE
disk_unmount(0);
#else
for(i = 0; i < NUM_DRIVES; i++)
{

#ifdef HAVE_HOTSWAP
  if(storage_present(i))
#endif 
  disk_unmount(i);

}
#endif

Perhaps the hotswap define is even useless. Notice that this is correct since disk_unmount(i) unmounts all volume on drive i. So this will unmounts all volumes on any drive. No ?

sideral commented on 2010-11-27 22:50

OK, I changed the code according to your advice. New patch attached.

amaury pouly commented on 2010-11-27 23:24

This looks ok to me, I hope the ifdef are ok.

Admin
Frank Gevaerts commented on 2010-11-28 15:25

I removed those complicated #ifdefs. They ended up meaning “on every target except mini2440 and application”, which I think isn’t worth it

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing