----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1316/#review3213 -----------------------------------------------------------
Nice! I really like patches that result in a net reduction of non-comment LOC... src/dev/dma_device.hh <http://reviews.gem5.org/r/1316/#comment3340> Why the switch from list to deque? Not that I have much preference, just wondering why it matters enough to change. src/dev/dma_device.hh <http://reviews.gem5.org/r/1316/#comment3339> I don't see where we use 'uint' anywhere else in the code, and I don't believe it's a standard typedef. If signedness really matters, use 'unsigned' or 'unsigned int' (we seem to use the former predominantly, and the latter just occasionally). src/dev/dma_device.cc <http://reviews.gem5.org/r/1316/#comment3341> I see that you're relocating the panic message at the bottom of the function to a more sensible location, but the whole check seems redundant wrt the assert(state) below... if senderState is null, it will pass through dynamic_cast unmodified and we'll catch it there. Thus I'd be in favor of just getting rid of this statement as effectively being an assertion that won't get compiled out with gem5.fast. src/dev/dma_device.cc <http://reviews.gem5.org/r/1316/#comment3342> Is the distinction about whether we use the size in the request or the packet significant? Shouldn't they be the same? This comment makes it sound like that distinction is meaningful... - Steve Reinhardt On July 28, 2012, 10:24 a.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1316/ > ----------------------------------------------------------- > > (Updated July 28, 2012, 10:24 a.m.) > > > Review request for Default. > > > Description > ------- > > Changeset 9146:a02007211735 > --------------------------- > DMA: Refactor the DMA device and align timing and atomic > > This patch does a bunch of house-keeping updates on the DMA, including > indentation, and formatting, but most importantly breaks out the > response handling such that it can be shared between the atomic and > timing modes. It also removes a potential bug caused by the atomic > handling of responses only deleting the allocated request (pkt->req) > once the DMA action completes instead of doing so for every packet. > > Before this patch, the handling of responses was near identical for > atomic and timing, but the code was simply duplicated. With this > patch, the handleResp method deals with the responses in both cases. > > There are further updates to make after removing the NACKs, but that > will be part of a separate follow-up patch. This patch does not change > the behaviour of any regression. > > > Diffs > ----- > > src/dev/dma_device.hh 64d4c9d8a384 > src/dev/dma_device.cc 64d4c9d8a384 > > Diff: http://reviews.gem5.org/r/1316/diff/ > > > Testing > ------- > > util/regress all passing (disregarding t1000 and eio) > > > Thanks, > > Andreas Hansson > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
