Message ID | a10ee95940f3e344cc94fbab1ea2295d6017b9dd.1399449874.git.peter.crosthwaite@xilinx.com |
---|---|
State | New |
Headers | show |
[...] if (tx_desc_get_last(desc)) { + unsigned desc_first[2]; + /* Modify the 1st descriptor of this packet to be owned by * the processor. */ cpu_physical_memory_read(s->tx_desc_addr, - (uint8_t *)&desc[0], sizeof(desc)); - tx_desc_set_used(desc); + (uint8_t *)&desc_first[0], sizeof(desc)); + tx_desc_set_used(desc_first); cpu_physical_memory_write(s->tx_desc_addr, - (uint8_t *)&desc[0], sizeof(desc)); + (uint8_t *)&desc_first[0], sizeof(desc)); /* Advance the hardare current descriptor past this packet */ This is quite fun. Can we, please, a) s/unsigned/uint8_t/ in the variable declaration, and remove the useless casts and "readdressing" everywhere? and, more interesting, b) s/sizeof(desc)/sizeof(desc_first)/g ? :) Thanks, /mjt
24.05.2014 10:38, Michael Tokarev wrote: > [...] > if (tx_desc_get_last(desc)) { > + unsigned desc_first[2]; > + > /* Modify the 1st descriptor of this packet to be owned by > * the processor. > */ > cpu_physical_memory_read(s->tx_desc_addr, > - (uint8_t *)&desc[0], sizeof(desc)); > - tx_desc_set_used(desc); > + (uint8_t *)&desc_first[0], sizeof(desc)); > + tx_desc_set_used(desc_first); > cpu_physical_memory_write(s->tx_desc_addr, > - (uint8_t *)&desc[0], sizeof(desc)); > + (uint8_t *)&desc_first[0], sizeof(desc)); > /* Advance the hardare current descriptor past this packet */ > > This is quite fun. > > Can we, please, > > a) s/unsigned/uint8_t/ in the variable declaration, > and remove the useless casts and "readdressing" everywhere? Um, no, this is ofcourse wrong - tx_desc_set_used() expects unsigned. Still it's possible to remove "readdressing", instead of (uint8_t *)&desc[0] use (uint8_t *)desc The second point stands.. ;) > and, more interesting, > > b) s/sizeof(desc)/sizeof(desc_first)/g > ? :) > > Thanks, > > /mjt >
diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c index e34b25e..1c36e48 100644 --- a/hw/net/cadence_gem.c +++ b/hw/net/cadence_gem.c @@ -917,14 +917,16 @@ static void gem_transmit(GemState *s) /* Last descriptor for this packet; hand the whole thing off */ if (tx_desc_get_last(desc)) { + unsigned desc_first[2]; + /* Modify the 1st descriptor of this packet to be owned by * the processor. */ cpu_physical_memory_read(s->tx_desc_addr, - (uint8_t *)&desc[0], sizeof(desc)); - tx_desc_set_used(desc); + (uint8_t *)&desc_first[0], sizeof(desc)); + tx_desc_set_used(desc_first); cpu_physical_memory_write(s->tx_desc_addr, - (uint8_t *)&desc[0], sizeof(desc)); + (uint8_t *)&desc_first[0], sizeof(desc)); /* Advance the hardare current descriptor past this packet */ if (tx_desc_get_wrap(desc)) { s->tx_desc_addr = s->regs[GEM_TXQBASE];
The local variable "desc" was being used to read-modify-write the first descriptor (of a multi-desc packet) upon packet completion. desc however continues to be used by the code as the current descriptor. Give this first desc RMW it's own local variable to avoid trampling. Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> --- hw/net/cadence_gem.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)