FS#9935 - audio_get_buffer() doesn't make the buffering code mark the buffer as invalid soon enough
Attached to Project:
Rockbox
Opened by Frank Gevaerts (fg) - Saturday, 21 February 2009, 15:06 GMT
Last edited by Frank Gevaerts (fg) - Wednesday, 25 February 2009, 22:52 GMT
Opened by Frank Gevaerts (fg) - Saturday, 21 February 2009, 15:06 GMT
Last edited by Frank Gevaerts (fg) - Wednesday, 25 February 2009, 22:52 GMT
|
DetailsWhen enabling the "software" USB stack (-DUSE_ROCKBOX_USB), the buffering code (sometimes) crashes with a data abort in update_data_counters() on USB disconnect. This happens especially if the USB connection was established while playback was active.
The software USB Storage implementation uses audio_get_buffer() to get memory to use. audio_get_buffer() is called *after* SYS_USB_CONNECTED, so the buffering thread is blocked in usb_wait_for_disconnect() (buffering.c:1385) at that time. On disconnect, the buffering thread leaves the switch block and unconditionally calls update_data_counters(), which assumes valid data in the buffer. References : http://www.rockbox.org/irc/log-20090219#21:57:44 and http://forums.rockbox.org/index.php?topic=20664.0 |
This task depends upon
Closed by Frank Gevaerts (fg)
Wednesday, 25 February 2009, 22:52 GMT
Reason for closing: Accepted
Additional comments about closing: Committed (to allow enabling USB)
Wednesday, 25 February 2009, 22:52 GMT
Reason for closing: Accepted
Additional comments about closing: Committed (to allow enabling USB)
- move audio_get_buffer() from playback to buffering (and rename it) - it can then "invalidate" all open buffering handles
- ensure that all buffering functions are safe to call with an invalidated handle (i.e they return a null-block, or error) without crashing.
- make sure that there's a standard way for the client to detect an invalidated handle (either a new function or as part of the previous point).
- (bonus point) send a system wide event when the buffer is grabbed so that playback (and any other users) can reset their state.
It simply adds a call to buffering_reset() in audio_get_buffer().
The bigger issue is that I'm not sure whether the audio_stop_playback call that happens on USB connect actually manages to close all the handles: if the buffering thread has acknowledged the USB connect before the audio thread, I guess the answer is no and I would even expect a deadlock.