Re: [Qemu-devel] [PATCH v2 05/15] imx_fec: Use ENET_FTRL to determine truncation length

[Andrey Smirnov]

On Fri, Dec 15, 2017 at 2:15 AM, Philippe Mathieu-Daudé <address@hidden> wrote:
> Hi Andrey,
>
> On 12/14/2017 11:52 AM, Andrey Smirnov wrote:
>> Frame truncation length, TRUNC_FL, is determined by the contents of
>> ENET_FTRL register, so convert the code to use it instead of a
>> hardcoded constant.
>>
>> To avoid the case where TRUNC_FL is greater that ENET_MAX_FRAME_SIZE,
>> increase the value of the latter to its theoretical maximum of 16K.
>>
>> Cc: Peter Maydell <address@hidden>
>> Cc: Jason Wang <address@hidden>
>> Cc: Philippe Mathieu-Daudé <address@hidden>
>> Cc: address@hidden
>> Cc: address@hidden
>> Cc: address@hidden
>> Signed-off-by: Andrey Smirnov <address@hidden>
>> ---
>>  hw/net/imx_fec.c         | 4 ++--
>>  include/hw/net/imx_fec.h | 3 ++-
>>  2 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
>> index 56cb72273c..50da91bf9e 100644
>> --- a/hw/net/imx_fec.c
>> +++ b/hw/net/imx_fec.c
>> @@ -1052,8 +1052,8 @@ static ssize_t imx_enet_receive(NetClientState *nc, 
>> const uint8_t *buf,
>>      crc_ptr = (uint8_t *) &crc;
>>
>>      /* Huge frames are truncted.  */
>> -    if (size > ENET_MAX_FRAME_SIZE) {
>> -        size = ENET_MAX_FRAME_SIZE;
>> +    if (size > s->regs[ENET_FTRL]) {
>
> Shouldn't this be:
>
>        if (size > s->regs[ENET_FTRL] + 1) {
>
>> +        size = s->regs[ENET_FTRL];
>
> and:
>
>            size = s->regs[ENET_FTRL] + 1;
>

I haven't tried to reproduce this in real HW and verify the behavior,
so I can't say for a fact, but the datasheet seems to be pretty clear
about "ENETx_FTRL":

"Frame Truncation Length
Indicates the value a receive frame is truncated, if it is greater
than this value. Must be greater than or
equal to RCR[MAX_FL].
NOTE: Truncation happens at TRUNC_FL. However, when truncation occurs,
the application (FIFO) may
receive less data, guaranteeing that it never receives more than the set limit."

so, having nothing better to go on than above description, I think the
original code is OK.


>>          flags |= ENET_BD_TR | ENET_BD_LG;
>>      }
>>
>> diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
>> index 67993870a2..a390d704a6 100644
>> --- a/include/hw/net/imx_fec.h
>> +++ b/include/hw/net/imx_fec.h
>> @@ -86,7 +86,6 @@
>>  #define ENET_TCCR3             393
>>  #define ENET_MAX               400
>>
>> -#define ENET_MAX_FRAME_SIZE    2032
>>
>>  /* EIR and EIMR */
>>  #define ENET_INT_HB            (1 << 31)
>> @@ -155,6 +154,8 @@
>>  #define ENET_RCR_NLC           (1 << 30)
>>  #define ENET_RCR_GRS           (1 << 31)
>>
>> +#define ENET_MAX_FRAME_SIZE    (1 << ENET_RCR_MAX_FL_LENGTH)
>> +
>>  /* TCR */
>>  #define ENET_TCR_GTS           (1 << 0)
>>  #define ENET_TCR_FDEN          (1 << 2)
>>