Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#6259 - Panic if queue overflows

Attached to Project: Rockbox
Opened by Steve Bavin (pondlife) - Sunday, 29 October 2006, 17:42 GMT+2
Last edited by Steve Bavin (pondlife) - Wednesday, 01 August 2007, 17:05 GMT+2
Task Type Patches
Category Operating System/Drivers
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

Here's an additional test to force a panic if a queue overflows. Please can someone check my maths is right before I commit this, I wouldn't want to cause unwanted panic, and it seems to detect a lot of overflows on my H300.
(application/octet-stream)    panic_on_queue_overflow.patch (1.3 KiB) 
 firmware/kernel.c        |    3 +++
 uisimulator/sdl/kernel.c |    5 +++++
 2 files changed, 8 insertions(+)

This task depends upon

Closed by  Steve Bavin (pondlife)
Wednesday, 01 August 2007, 17:05 GMT+2
Reason for closing:  Rejected
Additional comments about closing:  It's rather too easy to overflow a queue (by holding down a key during initial boot for example)... so this is probably not a good idea.
Comment by Dominik Riebeling (bluebrother) - Sunday, 29 October 2006, 19:19 GMT+2
from a quick look into the queuing stuff: mustn't you also wrap the queue value for the test? I.e.
if(q->write == ((q->read + QUEUE_LENGTH) & QUEUE_LENGTH_MASK))
like e.g. in queue_post() at kernel.c:164 as you otherwise would point to somewhere outside of the queue?

(Note that I know nothing of the queuing stuff, but that simply popped into my mind as I read the patch so I might be completely wrong -- just wanted to note this :-)
Comment by Linus Nielsen Feltzing (linusnielsen) - Sunday, 29 October 2006, 20:03 GMT+2
Since you will panic anyway, why not check after q->write has been incremented?

if(q->read == q->write)
panicf("Queue overflowed id=%x", id);
Comment by Steve Bavin (pondlife) - Monday, 30 October 2006, 09:05 GMT+2
Both read and write are forever-incrementing UINTs; they are not restricted by QUEUE_LENGTH_MASK. They will wrap eventually, but so will the proposed (q->read + QUEUE_LENGTH).

As I understand it, q->read == q->write just indicates an empty queue, not an overflowed one.

p.s. Out of interest, my H340 always overflows on shutdown with the standard queue length of 16...
Comment by Linus Nielsen Feltzing (linusnielsen) - Monday, 30 October 2006, 09:33 GMT+2
Hehe, silly me. And I even wrote that code myself... :-)
Comment by Brandon Low (lostlogic) - Monday, 30 October 2006, 18:36 GMT+2
Linus: I knew you did, now I laugh.
Comment by Steve Bavin (pondlife) - Monday, 13 November 2006, 09:47 GMT+2
Here's an updated version that I've been using to find queue overflows for a while now. This one is enabled on logf builds only and keeps track of the queue name so you can actually tell which queue overflowed.

I'll commit this one soon unless anyone objects.
(application/octet-stream)    panic_on_queue_overflow_v2.patch (13.2 KiB) 
 apps/playback.c                       |    8 ++++----
 apps/playlist.c                       |    2 +-
 apps/plugin.h                         |    2 +-
 apps/tagcache.c                       |    2 +-
 apps/plugins/battery_bench.c          |    2 +-
 firmware/backlight.c                  |    2 +-
 firmware/kernel.c                     |   10 +++++++++-
 firmware/mpeg.c                       |    2 +-
 firmware/pcm_record.c                 |    2 +-
 firmware/usb.c                        |    2 +-
 firmware/common/dircache.c            |    2 +-
 firmware/drivers/ata.c                |    2 +-
 firmware/drivers/ata_mmc.c            |    2 +-
 firmware/drivers/button.c             |    2 +-
 firmware/drivers/lcd-h100-remote.c    |    2 +-
 firmware/drivers/lcd-remote-2bit-vi.c |    2 +-
 firmware/export/kernel.h              |   11 ++++++++++-
 uisimulator/sdl/kernel.c              |    5 ++++-
 18 files changed, 41 insertions(+), 21 deletions(-)

Loading...