dead code in pl080 functions
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(
{
...
}
if this DEADCODE is not the author's original purpose,
Then there must be something in logic goes wrong, pl080 have NEVER works correctly ?
description: | updated |
Changed in qemu: | |
status: | New → Confirmed |
Changed in qemu: | |
status: | Fix Committed → Fix Released |
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