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

[David Laight <david.laight.linux@gmail.com>]

On Tue, 22 Apr 2025 14:28:31 +0100
Simon Horman <horms@kernel.org> wrote:

> + 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.

Indeed.
Using min_t(u16,...) is entirely an 'accident waiting to happen'.
If you are going to cast all the arguments to a function you really
better ensure the type is big enough for all the arguments.
The fact that one is 'u16' in no way indicates that casting the
other(s) won't discard high significant bits.
(and if you want a u16 result it is entirely wrong.)

In this case i don't understand the code at all.
The function is static and is only called once with a compile-time
constant value.
So, AFIACT, should reduce to a compile time constant.

There is also the entire 'issue' of using u16 variables at all.
You might want u16 structure members (to save space or map hardware)
but for local variables they are only likely to increase code size.

	David


> 
> >  
> >  	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);

So a definite NAK on that change.

> >  
> >  	if (pkt_num > 60) {
> >  		pkt_num_unit = RTASE_MITI_MAX_PKT_NUM_IDX;
> > -- 
> > 2.34.1
> >