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  wrote:
> On 2 December 2013 07:14, Peter Crosthwaite
>  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 
>> Signed-off-by: Peter Crosthwaite 
>> ---
>>
>>  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
 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 
> Signed-off-by: Peter Crosthwaite 
> ---
>
>  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

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

[Peter Crosthwaite]

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 
Signed-off-by: Peter Crosthwaite 
---

 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;
-}
-
 DB_PRINT("config bufsize: %d packet size: %ld\n", rxbufsize, size);
 
 while (bytes_to_copy) {
-- 
1.8.4.4