Re: [PATCH net v3 3/3] rtase: Fix a type error in min_t

[Simon Horman <horms@kernel.org>]

+ David Laight

On Thu, Apr 17, 2025 at 04:56:59PM +0800, Justin Lai wrote:
> Fix a type error in min_t.
> 
> Fixes: a36e9f5cfe9e ("rtase: Add support for a pci table in this module")
> Signed-off-by: Justin Lai <justinlai0215@realtek.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/net/ethernet/realtek/rtase/rtase_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/rtase/rtase_main.c b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> index 55b8d3666153..bc856fb3d6f3 100644
> --- a/drivers/net/ethernet/realtek/rtase/rtase_main.c
> +++ b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> @@ -1923,7 +1923,7 @@ static u16 rtase_calc_time_mitigation(u32 time_us)
>  	u8 msb, time_count, time_unit;
>  	u16 int_miti;
>  
> -	time_us = min_t(int, time_us, RTASE_MITI_MAX_TIME);
> +	time_us = min_t(u32, time_us, RTASE_MITI_MAX_TIME);

Hi Justin, Andrew, David, all,

I may be on the wrong track here, but near the top of minmax.h I see:

/*
 * min()/max()/clamp() macros must accomplish several things:
 *
 * - Avoid multiple evaluations of the arguments (so side-effects like
 *   "x++" happen only once) when non-constant.
 * - Perform signed v unsigned type-checking (to generate compile
 *   errors instead of nasty runtime surprises).
 * - Unsigned char/short are always promoted to signed int and can be
 *   compared against signed or unsigned arguments.
 * - Unsigned arguments can be compared against non-negative signed constants.
 * - Comparison of a signed argument against an unsigned constant fails
 *   even if the constant is below __INT_MAX__ and could be cast to int.
 */

So, considering the 2nd last point, I think we can simply use min()
both above and below. Which would avoid the possibility of
casting to the wrong type again in future.

Also, aside from which call is correct. Please add some colour
to the commit message describing why this is a bug if it is
to be treated as a fix for net rather than a clean-up for net-next.

>  
>  	if (time_us > RTASE_MITI_TIME_COUNT_MASK) {
>  		msb = fls(time_us);
> @@ -1945,7 +1945,7 @@ static u16 rtase_calc_packet_num_mitigation(u16 pkt_num)
>  	u8 msb, pkt_num_count, pkt_num_unit;
>  	u16 int_miti;
>  
> -	pkt_num = min_t(int, pkt_num, RTASE_MITI_MAX_PKT_NUM);
> +	pkt_num = min_t(u16, pkt_num, RTASE_MITI_MAX_PKT_NUM);
>  
>  	if (pkt_num > 60) {
>  		pkt_num_unit = RTASE_MITI_MAX_PKT_NUM_IDX;
> -- 
> 2.34.1
>