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().
- 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
- 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.
Arnd
