On 01/29, Ferruh Yigit wrote:
>On 1/26/2020 5:25 PM, David Harton wrote:
>> Modified UPDATE_VF_STAT to properly handle rollover conditions.
>>
>> Fixes: d82170d27918 ("igb: add VF support")
>> Cc: intel.com
>>
>> Signed-off-by: David Harton <dhar...@cisco.com>
>> ---
>> drivers/net/e1000/igb_ethdev.c | 14 +++++++++-----
>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
>> index a3e30dbe5..825663267 100644
>> --- a/drivers/net/e1000/igb_ethdev.c
>> +++ b/drivers/net/e1000/igb_ethdev.c
>> @@ -261,11 +261,15 @@ static int igb_filter_restore(struct rte_eth_dev *dev);
>> /*
>> * Define VF Stats MACRO for Non "cleared on read" register
>> */
>> -#define UPDATE_VF_STAT(reg, last, cur) \
>> -{ \
>> - u32 latest = E1000_READ_REG(hw, reg); \
>> - cur += (latest - last) & UINT_MAX; \
>
>Why this is wrong? Both 'latest' and 'last' are 'u32', so diff should be
>correct
>'u32' value. And it is added to 'u64' 'cur' value. What I am missing?
Per my understanding, stat value read from HW reg would be rolled over after
it reaches its maximum, so we need to handle both normal case (latest >= last)
and rollover case (latest < last) here. Wait for the original author for more
explanation.
>
>> - last = latest; \
>> +#define UPDATE_VF_STAT(reg, last, cur) \
>> +{ \
>> + u32 latest = E1000_READ_REG(hw, reg); \
>> + if (latest >= last) \
>> + cur += (latest - last); \
>> + else \
>> + cur += ((latest + ((uint64_t)1 << 32)) - last); \
>> + cur &= UINT_MAX; \
>
>Why & with UINT_MAX, won't this limit the value to 32bits which has 64bit
>storage?
>
Agree & with UINT_MAX should be removed here.
Thanks,
Xiaolong
>> + last = latest; \
>> }
>>
>> #define IGB_FC_PAUSE_TIME 0x0680
>>
>
