Re: [PATCH V2 net-next] net: fec: add CBS offload support

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Wei Fang <wei.fang@nxp.com>
Date: Thu, 16 Feb 2023 13:03:37 +0000

> 
>> -----Original Message-----
>> From: Alexander Lobakin <alexandr.lobakin@intel.com>
>> Sent: 2023年2月15日 0:49
>> To: Wei Fang <wei.fang@nxp.com>
>> Cc: Shenwei Wang <shenwei.wang@nxp.com>; Clark Wang
>> <xiaoning.wang@nxp.com>; davem@davemloft.net; edumazet@google.com;
>> kuba@kernel.org; pabeni@redhat.com; simon.horman@corigine.com;
>> andrew@lunn.ch; netdev@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
>> linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH V2 net-next] net: fec: add CBS offload support
>>
>> From: Wei Fang <wei.fang@nxp.com>
>> Date: Tue, 14 Feb 2023 09:34:09 +0000

[...]

>>>>> +	int idleslope[FEC_ENET_MAX_TX_QS];
>>>>> +	int sendslope[FEC_ENET_MAX_TX_QS];
>>>>
>>>> Can they actually be negative? (probably I'll see it below)
>>>>
>>> idleslope and sendslope are used to save the parameters passed in from user
>> space.
>>> And the sendslope should always be negative.
>>
>> Parameters coming from userspace must be validated before saving them
>> anywhere.
>> Also, if sendslope is always negative as you say, then just negate it when you
>> read it from userspace and store as u32.
> Sorry, I still don't understand why u32 is necessary to store parameters from user
> space? In addition, people who understand 802.1Qav may be confused when they
> see that sendslope is a u32 type.

I didn't say you need to store any userspace param as unsigned, I only
say that there's no point in using signed types when the value range
belongs to only either positive or negative space.
I'm not insisting in this particular case, I guess you pick what should
look more intuitive here.

> 
>>
>>>
>>>>> +};
>>>>> +
>>>>>  /* The FEC buffer descriptors track the ring buffers.  The rx_bd_base and

[...]

>>>> Oh okay. Then rounddown_pow_of_two() is what you're looking for.
>>>>
>>>> 	power = rounddown_pow_of_two(idle_slope);
>>>>
>>>> Or even just use one variable, @idle_slope.
>>>>
>>> Thanks for the reminder, I think I should use roundup_pow_of_two().
>>
>> But your code does what rounddown_pow_of_two() does, not roundup.
>> Imagine that you have 0b1111, then your code will turn it into 0b1000, not
>> 0b10000. Or am I missing something?
>>
> 0b1111 is nearest to 0b10000, so it should be turned into 0x10000.

fls() + BIT() won't give you the *nearest* pow-2, have you checked what
your code does return? Check with 0xff and then 0x101 and you'll be
surprised, it doesn't work that way.

I'd highly suggest you introducing not only round_closest(), but also
round_closest_pow_of_two(), as your driver might not be the sole user of
such generic functionality.

> 
>>>
>>>>> +	idle_slope = DIV_ROUND_CLOSEST(idle_slope, power) * power;
>>>>> +
>>>>> +	return idle_slope;
>>>>
>>>> You can return DIV_ROUND_ ... right away, without assignning first.
>>>> Also, I'm thinking of that this might be a generic helper. We have
>>>> roundup() and rounddown(), this could be something like "round_closest()"?
[...]

Thanks,
Olek