Re: [PATCH 04/17] bus/dpaa: add port buffer manager stats

[Hemant Agrawal <hemant.agrawal@oss.nxp.com>]

On 07-08-2024 21:08, Ferruh Yigit wrote:
> On 8/1/2024 11:53 AM, Hemant Agrawal wrote:
>> Add BMI statistics and improving the existing extended
>> statistics
>>
>> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
>> Signed-off-by: Gagandeep Singh <g.singh@nxp.com>
>> ---
>>   drivers/bus/dpaa/base/fman/fman_hw.c | 65 +++++++++++++++++++++++++++-
>>   drivers/bus/dpaa/include/fman.h      |  4 +-
>>   drivers/bus/dpaa/include/fsl_fman.h  | 12 +++++
>>   drivers/bus/dpaa/version.map         |  4 ++
>>   drivers/net/dpaa/dpaa_ethdev.c       | 46 +++++++++++++++++---
>>   drivers/net/dpaa/dpaa_ethdev.h       | 12 +++++
>>   6 files changed, 134 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/bus/dpaa/base/fman/fman_hw.c b/drivers/bus/dpaa/base/fman/fman_hw.c
>> index 24a99f7235..27b39a4975 100644
>> --- a/drivers/bus/dpaa/base/fman/fman_hw.c
>> +++ b/drivers/bus/dpaa/base/fman/fman_hw.c
>> @@ -244,8 +244,8 @@ fman_if_stats_get_all(struct fman_if *p, uint64_t *value, int n)
>>   	uint64_t base_offset = offsetof(struct memac_regs, reoct_l);
>>   
>>   	for (i = 0; i < n; i++)
>> -		value[i] = (((u64)in_be32((char *)regs + base_offset + 8 * i) |
>> -				(u64)in_be32((char *)regs + base_offset +
>> +		value[i] = ((u64)in_be32((char *)regs + base_offset + 8 * i) |
>> +				((u64)in_be32((char *)regs + base_offset +
>>   				8 * i + 4)) << 32);
>>
> Above change looks like a bug fix, it is converting from
> "(a | b) << 32"  to  "a | (b << 32)"
>
> Syntax wise a small change that is easy to miss, but impacts the result.
>
> Why not simplify it something like:
> uint64_t a = in_be32((char *)regs + base_offset + 8 * i)
> uint64_t b = in_be32((char *)regs + base_offset + 8 * i + 4)
> value[i] = a | b << 32
>
> Anyway, my point is, should it go to its own patch, with fixes tag and
> stable tag, so it can be backported to stable releases.
>
> <...>
Thanks for the suggestion.  we made it a separate patch for it.