This is the bug/patch tracker for Rockbox. Click here for more information.
Quick links: Bugs · Patches · Rockbox frontpage
FS#11774 - Unmount all disks before entering USB slave mode to prevent filesystem corruption
Attached to Project:
Rockbox
Opened by sideral (sideral) - Wednesday, 24 November 2010, 14:44 GMT+2
Last edited by Frank Gevaerts (fg) - Sunday, 28 November 2010, 16:25 GMT+2
Opened by sideral (sideral) - Wednesday, 24 November 2010, 14:44 GMT+2
Last edited by Frank Gevaerts (fg) - Sunday, 28 November 2010, 16:25 GMT+2
|
DetailsI recently enabled Rockbox-native USB slave mode on my Sansa ClipV2 through the patch provided in
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? 
0001-Unmount-all-disks-before-going-into-USB-slav...
(4.3 KiB)
b/firmware/common/dir_uncached.c | 4 ++-- b/firmware/common/disk.c | 25 ++++++++++++++++++++++++- b/firmware/common/file.c | 4 ++-- b/firmware/drivers/fat.c | 4 ++-- b/firmware/usb.c | 2 ++ 5 files changed, 32 insertions(+), 7 deletions(-) |
This task depends upon
Closed by Frank Gevaerts (fg)
Sunday, 28 November 2010, 16:25 GMT+2
Reason for closing: Accepted
Additional comments about closing: Committed as r28693
Sunday, 28 November 2010, 16:25 GMT+2
Reason for closing: Accepted
Additional comments about closing: Committed as r28693
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 !
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?
#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 ?