dead code in pl080 functions

Bug #1637974 reported by jeyyej
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
QEMU
Fix Released
Undecided
Unassigned

Bug Description

pl080 is arm dma controller virtual device.
source code path: hw/dma/pl080.c
I find there are two same dead code in pl080_read and pl080_write,
Here's the code, comments are my opinion:
=========================
240 static uint64_t pl080_read(void *opaque, hwaddr offset,
241 unsigned size)
242 {
243 PL080State *s = (PL080State *)opaque;
244 uint32_t i;
245 uint32_t mask;
246
247 if (offset >= 0xfe0 && offset < 0x1000) {
248 if (s->nchannels == 8) {
249 return pl080_id[(offset - 0xfe0) >> 2];
250 } else {
251 return pl081_id[(offset - 0xfe0) >> 2];
252 }
253 }
254 if (offset >= 0x100 && offset < 0x200) { //// here offset is limited in 0x100~0x200
255 i = (offset & 0xe0) >> 5;
256 if (i >= s->nchannels)
257 goto bad_offset;
258 switch (offset >> 2) { //// then here, offset>>2 is in range 64~128
259 case 0: /* SrcAddr */ //// while the switch case is 0,1,2,3,4,
260 return s->chan[i].src; //// so, NONE of the switch case would be selected !
261 case 1: /* DestAddr */ //// this switch is A DEAD CODE, it is contradictory with if.
262 return s->chan[i].dest;
263 case 2: /* LLI */
264 return s->chan[i].lli;
265 case 3: /* Control */
266 return s->chan[i].ctrl;
267 case 4: /* Configuration */
268 return s->chan[i].conf;
269 default:
270 goto bad_offset;
271 }
272 }
        .....................................
=============================================

I guess, switch statement should like this :
switch((offset-0x100)>>2)
{
...
}
if this DEADCODE is not the author's original purpose,
Then there must be something in logic goes wrong, pl080 have NEVER works correctly ?

Tags: arm deadcode
jeyyej (jeyyej)
description: updated
Revision history for this message
Peter Maydell (pmaydell) wrote : Re: [Qemu-devel] [Bug 1637974] [NEW] dead code in pl080 functions

On 31 October 2016 at 10:48, jeyyej <email address hidden> wrote:
> Public bug reported:
>
> pl080 is arm dma controller virtual device.
> source code path: hw/dma/pl080.c
> I find there are two same dead code in pl080_read and pl080_write,

> + if this DEADCODE is not the author's original purpose,
> + Then there must be something in logic goes wrong, pl080 have NEVER works correctly ?

Yes, this code is incorrect. However the only registers
affected are those that set up the DMA channels, and
the code for actually doing DMA transfers will assert if
the guest ever tries to use it (see the hw_error() call in
pl080_run()), so even if the guest could set up the DMA
channels it would run into problems later.

This device is only used in the versatilepb board, and
I don't think Linux attempts to use it for DMA, so nobody's
ever needed to care that it's actually totally broken
for anything beyond "shouldn't crash the kernel when it
tries to probe for and initialize it".

thanks
-- PMM

Revision history for this message
jeyyej (jeyyej) wrote :

@Peter Maydell Oh, I see, but would you try to fix this code ?

Revision history for this message
Peter Maydell (pmaydell) wrote :

I have no requirement for a working pl080, since I have no guest code that requires one. If somebody else (you?) wants to send a patch that fixes it, I'm happy to review it.

Peter Maydell (pmaydell)
Changed in qemu:
status: New → Confirmed
Revision history for this message
Thomas Huth (th-huth) wrote :
Changed in qemu:
status: Confirmed → Fix Committed
Thomas Huth (th-huth)
Changed in qemu:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.