On Sat, 2021-02-27 at 13:14 +0100, Arnd Bergmann wrote:
> On Fri, Feb 12, 2021 at 3:59 AM Saeed Mahameed <sa...@kernel.org>
> wrote:
> >
> > From: Parav Pandit <pa...@nvidia.com>
> >
> > rate_bytes_ps is a 64-bit field. It passed as 32-bit field to
> > apply_police_params(). Due to this when police rate is higher
> > than 4Gbps, 32-bit calculation ignores the carry. This results
> > in incorrect rate configurationn the device.
> >
> > Fix it by performing 64-bit calculation.
>
> I just stumbled over this commit while looking at an unrelated
> problem.
>
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > index dd0bfbacad47..717fbaa6ce73 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > @@ -5040,7 +5040,7 @@ static int apply_police_params(struct
> > mlx5e_priv *priv, u64 rate,
> > */
> > if (rate) {
> > rate = (rate * BITS_PER_BYTE) + 500000;
> > - rate_mbps = max_t(u32, do_div(rate, 1000000), 1);
> > + rate_mbps = max_t(u64, do_div(rate, 1000000), 1);
>
> I think there are still multiple issues with this line:
>
> - Before commit 1fe3e3166b35 ("net/mlx5e: E-switch, Fix rate
> calculation for
> overflow"), it was trying to calculate rate divided by 1000000, but
> now
> it uses the remainder of the division rather than the quotient. I
> assume
> this was meant to use div_u64() instead of do_div().
>
Yes, I already have a patch lined up to fix this issue.
Thanks for spotting this.
> - Both div_u64() and do_div() return a 32-bit number, and '1' is a
> constant
> that also comfortably fits into a 32-bit number, so changing the
> max_t
> to return a 64-bit type has no effect on the result
>
as of the above comment, we shouldn't be using the return value of
do_div().
> - The maximum of an arbitrary unsigned integer and '1' is either one
> or zero,
> so there doesn't need to be an expensive division here at all.
> From the
> comment it sounds like the intention was to use 'min_t()' instead
> of 'max_t()'.
> It has however used 'max_t' since the code was first introduced.
>
if the input rate is less that 1mbps then the quotient will be 0,
otherwise we want the quotient, and we don't allow 0, so max_t(rate, 1)
should be used, what am I missing ?
