| Message ID | f1d81a88e4d531c00a27b77d9648711429bd1642.1385967754.git.peter.crosthwaite@xilinx.com |
|---|---|
| State | New |
| Headers | show |
On 2 December 2013 07:11, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > There was a replication of the rx descriptor address walking logic. > Reorder the flow control to remove. This refactoring also obsoletes > the local variables packet_desc_addr and last_desc_addr. > > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > --- > > hw/net/cadence_gem.c | 39 ++++++++++----------------------------- > 1 file changed, 10 insertions(+), 29 deletions(-) > > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c > index 3f30caf..73ac0d8 100644 > --- a/hw/net/cadence_gem.c > +++ b/hw/net/cadence_gem.c > @@ -586,7 +586,6 @@ static int gem_mac_address_filter(GemState *s, const uint8_t *packet) > static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size) > { > unsigned desc[2]; > - hwaddr packet_desc_addr, last_desc_addr; > GemState *s; > unsigned rxbufsize, bytes_to_copy; > unsigned rxbuf_offset; > @@ -667,17 +666,16 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size) > > DB_PRINT("config bufsize: %d packet size: %ld\n", rxbufsize, size); > > - packet_desc_addr = s->rx_desc_addr; > - while (1) { > - DB_PRINT("read descriptor 0x%x\n", (unsigned)packet_desc_addr); > + while (bytes_to_copy) { > + DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr); Can we get here with bytes_to_copy == 0 ? The logic sets size to a minimum of 64 bytes but we've already set up bytes_to_copy before that. -- PMM
On Mon, Dec 2, 2013 at 10:12 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 2 December 2013 07:11, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> There was a replication of the rx descriptor address walking logic. >> Reorder the flow control to remove. This refactoring also obsoletes >> the local variables packet_desc_addr and last_desc_addr. >> >> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >> --- >> >> hw/net/cadence_gem.c | 39 ++++++++++----------------------------- >> 1 file changed, 10 insertions(+), 29 deletions(-) >> >> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c >> index 3f30caf..73ac0d8 100644 >> --- a/hw/net/cadence_gem.c >> +++ b/hw/net/cadence_gem.c >> @@ -586,7 +586,6 @@ static int gem_mac_address_filter(GemState *s, const uint8_t *packet) >> static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size) >> { >> unsigned desc[2]; >> - hwaddr packet_desc_addr, last_desc_addr; >> GemState *s; >> unsigned rxbufsize, bytes_to_copy; >> unsigned rxbuf_offset; >> @@ -667,17 +666,16 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size) >> >> DB_PRINT("config bufsize: %d packet size: %ld\n", rxbufsize, size); >> >> - packet_desc_addr = s->rx_desc_addr; >> - while (1) { >> - DB_PRINT("read descriptor 0x%x\n", (unsigned)packet_desc_addr); >> + while (bytes_to_copy) { >> + DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr); > > Can we get here with bytes_to_copy == 0 ? The logic sets size to a minimum > of 64 bytes but we've already set up bytes_to_copy before that. > Should we be able to get here? Wouldn't that means you are trying to recieve a length 0 packet? Could just convert to do loop to receive a length 0 packet when gem_recieve is called with size = 0. Regards, Peter > -- PMM >
diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c index 3f30caf..73ac0d8 100644 --- a/hw/net/cadence_gem.c +++ b/hw/net/cadence_gem.c @@ -586,7 +586,6 @@ static int gem_mac_address_filter(GemState *s, const uint8_t *packet) static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size) { unsigned desc[2]; - hwaddr packet_desc_addr, last_desc_addr; GemState *s; unsigned rxbufsize, bytes_to_copy; unsigned rxbuf_offset; @@ -667,17 +666,16 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size) DB_PRINT("config bufsize: %d packet size: %ld\n", rxbufsize, size); - packet_desc_addr = s->rx_desc_addr; - while (1) { - DB_PRINT("read descriptor 0x%x\n", (unsigned)packet_desc_addr); + while (bytes_to_copy) { + DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr); /* read current descriptor */ - cpu_physical_memory_read(packet_desc_addr, + cpu_physical_memory_read(s->rx_desc_addr, (uint8_t *)&desc[0], sizeof(desc)); /* Descriptor owned by software ? */ if (rx_desc_get_ownership(desc) == 1) { DB_PRINT("descriptor 0x%x owned by sw.\n", - (unsigned)packet_desc_addr); + (unsigned)s->rx_desc_addr); s->regs[GEM_RXSTATUS] |= GEM_RXSTATUS_NOBUF; s->regs[GEM_ISR] |= GEM_INT_RXUSED & ~(s->regs[GEM_IMR]); /* Handle interrupt consequences */ @@ -693,7 +691,7 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size) */ if (rx_desc_get_buffer(desc) == 0) { DB_PRINT("Invalid RX buffer (NULL) for descriptor 0x%x\n", - (unsigned)packet_desc_addr); + (unsigned)s->rx_desc_addr); } /* Copy packet data to emulated DMA buffer */ @@ -713,36 +711,19 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size) } rx_desc_set_ownership(desc); /* Descriptor write-back. */ - cpu_physical_memory_write(packet_desc_addr, + cpu_physical_memory_write(s->rx_desc_addr, (uint8_t *)&desc[0], sizeof(desc)); - if (bytes_to_copy == 0) { - break; - } - /* Next descriptor */ if (rx_desc_get_wrap(desc)) { - packet_desc_addr = s->regs[GEM_RXQBASE]; + DB_PRINT("wrapping RX descriptor list\n"); + s->rx_desc_addr = s->regs[GEM_RXQBASE]; } else { - packet_desc_addr += 8; + DB_PRINT("incrementing RX descriptor list\n"); + s->rx_desc_addr += 8; } } - DB_PRINT("set length: %ld, EOF on descriptor 0x%x\n", size, - (unsigned)packet_desc_addr); - - /* Advance RX packet descriptor Q */ - last_desc_addr = packet_desc_addr; - packet_desc_addr = s->rx_desc_addr; - s->rx_desc_addr = last_desc_addr; - if (rx_desc_get_wrap(desc)) { - s->rx_desc_addr = s->regs[GEM_RXQBASE]; - DB_PRINT("wrapping RX descriptor list\n"); - } else { - DB_PRINT("incrementing RX descriptor list\n"); - s->rx_desc_addr += 8; - } - /* Count it */ gem_receive_updatestats(s, buf, size);
There was a replication of the rx descriptor address walking logic. Reorder the flow control to remove. This refactoring also obsoletes the local variables packet_desc_addr and last_desc_addr. Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> --- hw/net/cadence_gem.c | 39 ++++++++++----------------------------- 1 file changed, 10 insertions(+), 29 deletions(-)