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

[Wei Fang]

> -----Original Message-----
> From: Alexander Lobakin <aleksander.lobakin@xxxxxxxxx>
> Sent: 2023年2月16日 23:28
> To: Wei Fang <wei.fang@xxxxxxx>
> Cc: Shenwei Wang <shenwei.wang@xxxxxxx>; Clark Wang
> <xiaoning.wang@xxxxxxx>; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx;
> kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; simon.horman@xxxxxxxxxxxx;
> andrew@xxxxxxx; netdev@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>;
> linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH V2 net-next] net: fec: add CBS offload support
> 
> From: Wei Fang <wei.fang@xxxxxxx>
> Date: Thu, 16 Feb 2023 13:03:37 +0000
> 
> >
> >> -----Original Message-----
> >> From: Alexander Lobakin <alexandr.lobakin@xxxxxxxxx>
> >> Sent: 2023年2月15日 0:49
> >> To: Wei Fang <wei.fang@xxxxxxx>
> >> Cc: Shenwei Wang <shenwei.wang@xxxxxxx>; Clark Wang
> >> <xiaoning.wang@xxxxxxx>; davem@xxxxxxxxxxxxx;
> edumazet@xxxxxxxxxx;
> >> kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; simon.horman@xxxxxxxxxxxx;
> >> andrew@xxxxxxx; netdev@xxxxxxxxxxxxxxx; dl-linux-imx
> >> <linux-imx@xxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx
> >> Subject: Re: [PATCH V2 net-next] net: fec: add CBS offload support
> >>
> >> From: Wei Fang <wei.fang@xxxxxxx>
> >> Date: Tue, 14 Feb 2023 09:34:09 +0000
> 
> [...]
> >>>> 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.
> 
My real intention is, if x >= 1.5 * 2 ^ (fls(x) - 1), then x = 2 ^ fls(x), otherwise,
x = 2 ^ (fls(x) - 1). Anyway, I'll check the final implementation can meet my
expectation.

> 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.
> 
Yes, I agree with you, it's better to use the 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