Re: [Qemu-devel] [PATCH arm-devs v1 10/13] net/cadence_gem: Fix small packet FCS stripping

[Peter Crosthwaite]

On Mon, Dec 2, 2013 at 10:26 PM, Peter Maydell peter.mayd...@linaro.org wrote:
 On 2 December 2013 07:14, Peter Crosthwaite
 peter.crosthwa...@xilinx.com wrote:
 The minimum packet size is 64, however this is before FCS stripping
 occurs. So when FCS stripping the minimum packet size is 60. Fix.

 Reported-by: Deepika Dhamija deep...@xilinx.com
 Signed-off-by: Peter Crosthwaite peter.crosthwa...@xilinx.com
 ---

  hw/net/cadence_gem.c | 13 -
  1 file changed, 8 insertions(+), 5 deletions(-)

 diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
 index eb0fa95..babd39d 100644
 --- a/hw/net/cadence_gem.c
 +++ b/hw/net/cadence_gem.c
 @@ -674,6 +674,14 @@ static ssize_t gem_receive(NetClientState *nc, const 
 uint8_t *buf, size_t size)
  rxbuf_offset = (s-regs[GEM_NWCFG]  GEM_NWCFG_BUFF_OFST_M) 
 GEM_NWCFG_BUFF_OFST_S;

 +/* Pad to minimum length. Assume FCS field is stripped, logic
 + * below will increment it to the real minimum of 64 when
 + * not FCS stripping
 + */
 +if (size  60) {
 +size = 60;
 +}
 +
  /* The configure size of each receive buffer.  Determines how many
   * buffers needed to hold this packet.
   */
 @@ -707,11 +715,6 @@ static ssize_t gem_receive(NetClientState *nc, const 
 uint8_t *buf, size_t size)
  size += 4;
  }

 -/* Pad to minimum length */
 -if (size  64) {
 -size = 64;
 -}
 -

 This change moves the padding of size from below the point where
 we initialize bytes_to_copy to above it, so now bytes_to_copy will
 get the padded value rather than the unpadded value. If this is deliberate
 it should probably be spelled out somewhere. (See also comments on
 earlier patch.)


So I cant see a good reason for that change. Reverted - just moved the
added hunk to below the bytes_to_copy =. Stress tests and linux tests
still pass.

Regards,
Peter

 thanks
 -- PMM

Re: [Qemu-devel] [PATCH arm-devs v1 10/13] net/cadence_gem: Fix small packet FCS stripping

[Peter Maydell]

On 2 December 2013 07:14, Peter Crosthwaite
peter.crosthwa...@xilinx.com wrote:
 The minimum packet size is 64, however this is before FCS stripping
 occurs. So when FCS stripping the minimum packet size is 60. Fix.

 Reported-by: Deepika Dhamija deep...@xilinx.com
 Signed-off-by: Peter Crosthwaite peter.crosthwa...@xilinx.com
 ---

  hw/net/cadence_gem.c | 13 -
  1 file changed, 8 insertions(+), 5 deletions(-)

 diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
 index eb0fa95..babd39d 100644
 --- a/hw/net/cadence_gem.c
 +++ b/hw/net/cadence_gem.c
 @@ -674,6 +674,14 @@ static ssize_t gem_receive(NetClientState *nc, const 
 uint8_t *buf, size_t size)
  rxbuf_offset = (s-regs[GEM_NWCFG]  GEM_NWCFG_BUFF_OFST_M) 
 GEM_NWCFG_BUFF_OFST_S;

 +/* Pad to minimum length. Assume FCS field is stripped, logic
 + * below will increment it to the real minimum of 64 when
 + * not FCS stripping
 + */
 +if (size  60) {
 +size = 60;
 +}
 +
  /* The configure size of each receive buffer.  Determines how many
   * buffers needed to hold this packet.
   */
 @@ -707,11 +715,6 @@ static ssize_t gem_receive(NetClientState *nc, const 
 uint8_t *buf, size_t size)
  size += 4;
  }

 -/* Pad to minimum length */
 -if (size  64) {
 -size = 64;
 -}
 -

This change moves the padding of size from below the point where
we initialize bytes_to_copy to above it, so now bytes_to_copy will
get the padded value rather than the unpadded value. If this is deliberate
it should probably be spelled out somewhere. (See also comments on
earlier patch.)

thanks
-- PMM