[PATCH net v3 0/3] Modify overflow detection, expand ivec->name, and correct type in min_t

[PATCH net v3 0/3] Modify overflow detection, expand ivec->name, and correct type in min_t

[Justin Lai]

This patch set mainly involves fixing the kernel test
robot issue and the type error in min_t.
Details are as follows:
1. Modify the condition used to detect overflow in
rtase_calc_time_mitigation
2. Increase the size of ivec->name
3. Fix a type error in min_t

v1 -> v2:
- Nothing has changed, and it is simply being posted again because the
initial version was posted incompletely.

v2 -> v3:
- Modify the subjects of the cover letter and the patches.

Justin Lai (3):
  rtase: Modify the condition used to detect overflow in
    rtase_calc_time_mitigation
  rtase: Increase the size of ivec->name
  rtase: Fix a type error in min_t

 drivers/net/ethernet/realtek/rtase/rtase.h      | 2 +-
 drivers/net/ethernet/realtek/rtase/rtase_main.c | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

-- 
2.34.1

[PATCH net v3 1/3] rtase: Modify the condition used to detect overflow in rtase_calc_time_mitigation

[Justin Lai]

Fix the following compile error reported by the kernel test
robot by modifying the condition used to detect overflow in
rtase_calc_time_mitigation.

In file included from include/linux/mdio.h:10:0,
                  from drivers/net/ethernet/realtek/rtase/rtase_main.c:58:
 In function 'u16_encode_bits',
     inlined from 'rtase_calc_time_mitigation.constprop' at drivers/net/
     ethernet/realtek/rtase/rtase_main.c:1915:13,
     inlined from 'rtase_init_software_variable.isra.41' at drivers/net/
     ethernet/realtek/rtase/rtase_main.c:1961:13,
     inlined from 'rtase_init_one' at drivers/net/ethernet/realtek/
     rtase/rtase_main.c:2111:2:
>> include/linux/bitfield.h:178:3: error: call to '__field_overflow'
      declared with attribute error: value doesn't fit into mask
    __field_overflow();     \
    ^~~~~~~~~~~~~~~~~~
 include/linux/bitfield.h:198:2: note: in expansion of macro
 '____MAKE_OP'
   ____MAKE_OP(u##size,u##size,,)
   ^~~~~~~~~~~
 include/linux/bitfield.h:200:1: note: in expansion of macro
 '__MAKE_OP'
  __MAKE_OP(16)
  ^~~~~~~~~

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202503182158.nkAlbJWX-lkp@intel.com/
Fixes: a36e9f5cfe9e ("rtase: Add support for a pci table in this module")
Signed-off-by: Justin Lai <justinlai0215@realtek.com>
---
 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 2aacc1996796..55b8d3666153 100644
--- a/drivers/net/ethernet/realtek/rtase/rtase_main.c
+++ b/drivers/net/ethernet/realtek/rtase/rtase_main.c
@@ -1925,8 +1925,8 @@ static u16 rtase_calc_time_mitigation(u32 time_us)
 
 	time_us = min_t(int, time_us, RTASE_MITI_MAX_TIME);
 
-	msb = fls(time_us);
-	if (msb >= RTASE_MITI_COUNT_BIT_NUM) {
+	if (time_us > RTASE_MITI_TIME_COUNT_MASK) {
+		msb = fls(time_us);
 		time_unit = msb - RTASE_MITI_COUNT_BIT_NUM;
 		time_count = time_us >> (msb - RTASE_MITI_COUNT_BIT_NUM);
 	} else {
-- 
2.34.1

Re: [PATCH net v3 1/3] rtase: Modify the condition used to detect overflow in rtase_calc_time_mitigation

[Simon Horman]

On Thu, Apr 17, 2025 at 04:56:57PM +0800, Justin Lai wrote:
> Fix the following compile error reported by the kernel test
> robot by modifying the condition used to detect overflow in
> rtase_calc_time_mitigation.
> 
> In file included from include/linux/mdio.h:10:0,
>                   from drivers/net/ethernet/realtek/rtase/rtase_main.c:58:
>  In function 'u16_encode_bits',
>      inlined from 'rtase_calc_time_mitigation.constprop' at drivers/net/
>      ethernet/realtek/rtase/rtase_main.c:1915:13,
>      inlined from 'rtase_init_software_variable.isra.41' at drivers/net/
>      ethernet/realtek/rtase/rtase_main.c:1961:13,
>      inlined from 'rtase_init_one' at drivers/net/ethernet/realtek/
>      rtase/rtase_main.c:2111:2:
> >> include/linux/bitfield.h:178:3: error: call to '__field_overflow'
>       declared with attribute error: value doesn't fit into mask
>     __field_overflow();     \
>     ^~~~~~~~~~~~~~~~~~
>  include/linux/bitfield.h:198:2: note: in expansion of macro
>  '____MAKE_OP'
>    ____MAKE_OP(u##size,u##size,,)
>    ^~~~~~~~~~~
>  include/linux/bitfield.h:200:1: note: in expansion of macro
>  '__MAKE_OP'
>   __MAKE_OP(16)
>   ^~~~~~~~~
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202503182158.nkAlbJWX-lkp@intel.com/
> Fixes: a36e9f5cfe9e ("rtase: Add support for a pci table in this module")
> Signed-off-by: Justin Lai <justinlai0215@realtek.com>

Hi Justin,

FWIIW, I note that this problem is reported by GCC 7.5.0 on sparc64 but not
by GCC 14.2.0. And I think that is because in the end the values passed to
u16_encode_bits (line 1915 in the trace above) are the same with and
without this patch. That is to say, this the compiler error above is a
false positive of sorts.

But I believe GCC 7.5.0 is a supported compiler version for sparc64.
And this does result in an error, without W=1 or any other extra KCFLAGS
set. So I agree this is appropriate to treat as a fix for net.

And in any case, fix or no fix, it seems nice to limit the scope of
the initialisation of msb.

Reviewed-by: Simon Horman <horms@kernel.org>

RE: [PATCH net v3 1/3] rtase: Modify the condition used to detect overflow in rtase_calc_time_mitigation

[Justin Lai]

> On Thu, Apr 17, 2025 at 04:56:57PM +0800, Justin Lai wrote:
> > Fix the following compile error reported by the kernel test robot by
> > modifying the condition used to detect overflow in
> > rtase_calc_time_mitigation.
> >
> > In file included from include/linux/mdio.h:10:0,
> >                   from
> drivers/net/ethernet/realtek/rtase/rtase_main.c:58:
> >  In function 'u16_encode_bits',
> >      inlined from 'rtase_calc_time_mitigation.constprop' at drivers/net/
> >      ethernet/realtek/rtase/rtase_main.c:1915:13,
> >      inlined from 'rtase_init_software_variable.isra.41' at drivers/net/
> >      ethernet/realtek/rtase/rtase_main.c:1961:13,
> >      inlined from 'rtase_init_one' at drivers/net/ethernet/realtek/
> >      rtase/rtase_main.c:2111:2:
> > >> include/linux/bitfield.h:178:3: error: call to '__field_overflow'
> >       declared with attribute error: value doesn't fit into mask
> >     __field_overflow();     \
> >     ^~~~~~~~~~~~~~~~~~
> >  include/linux/bitfield.h:198:2: note: in expansion of macro
> > '____MAKE_OP'
> >    ____MAKE_OP(u##size,u##size,,)
> >    ^~~~~~~~~~~
> >  include/linux/bitfield.h:200:1: note: in expansion of macro
> > '__MAKE_OP'
> >   __MAKE_OP(16)
> >   ^~~~~~~~~
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes:
> > https://lore.kernel.org/oe-kbuild-all/202503182158.nkAlbJWX-lkp@intel.
> > com/
> > Fixes: a36e9f5cfe9e ("rtase: Add support for a pci table in this
> > module")
> > Signed-off-by: Justin Lai <justinlai0215@realtek.com>
> 
> Hi Justin,
> 
> FWIIW, I note that this problem is reported by GCC 7.5.0 on sparc64 but not by
> GCC 14.2.0. And I think that is because in the end the values passed to
> u16_encode_bits (line 1915 in the trace above) are the same with and without
> this patch. That is to say, this the compiler error above is a false positive of
> sorts.
> 
> But I believe GCC 7.5.0 is a supported compiler version for sparc64.
> And this does result in an error, without W=1 or any other extra KCFLAGS set.
> So I agree this is appropriate to treat as a fix for net.
> 
> And in any case, fix or no fix, it seems nice to limit the scope of the
> initialisation of msb.
> 
> Reviewed-by: Simon Horman <horms@kernel.org>

Hi Simon,

Thank you for your review. I will repost this patch separately in the
net tree.

Thanks,
Justin

[PATCH net v3 2/3] rtase: Increase the size of ivec->name

[Justin Lai]

Fix the following compile warning reported by the kernel test robot by
increasing the size of ivec->name.

drivers/net/ethernet/realtek/rtase/rtase_main.c: In function 'rtase_open':
>> drivers/net/ethernet/realtek/rtase/rtase_main.c:1117:52: warning:
'%i' directive output may be truncated writing between 1 and 10 bytes
into a region of size between 7 and 22 [-Wformat-truncation=]
     snprintf(ivec->name, sizeof(ivec->name), "%s_int%i",
                                                     ^~
 drivers/net/ethernet/realtek/rtase/rtase_main.c:1117:45: note:
 directive argument in the range [0, 2147483647]
     snprintf(ivec->name, sizeof(ivec->name), "%s_int%i",
                                              ^~~~~~~~~~
 drivers/net/ethernet/realtek/rtase/rtase_main.c:1117:4: note:
 'snprintf' output between 6 and 30 bytes into a destination of
 size 26
     snprintf(ivec->name, sizeof(ivec->name), "%s_int%i",
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       tp->dev->name, i);
       ~~~~~~~~~~~~~~~~~

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202503182158.nkAlbJWX-lkp@intel.com/
Fixes: a36e9f5cfe9e ("rtase: Add support for a pci table in this module")
Signed-off-by: Justin Lai <justinlai0215@realtek.com>
---
 drivers/net/ethernet/realtek/rtase/rtase.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/rtase/rtase.h b/drivers/net/ethernet/realtek/rtase/rtase.h
index 2bbfcad613ab..1e63b5826da1 100644
--- a/drivers/net/ethernet/realtek/rtase/rtase.h
+++ b/drivers/net/ethernet/realtek/rtase/rtase.h
@@ -259,7 +259,7 @@ union rtase_rx_desc {
 #define RTASE_VLAN_TAG_MASK     GENMASK(15, 0)
 #define RTASE_RX_PKT_SIZE_MASK  GENMASK(13, 0)
 
-#define RTASE_IVEC_NAME_SIZE (IFNAMSIZ + 10)
+#define RTASE_IVEC_NAME_SIZE (IFNAMSIZ + 14)
 
 struct rtase_int_vector {
 	struct rtase_private *tp;
-- 
2.34.1

Re: [PATCH net v3 2/3] rtase: Increase the size of ivec->name

[Simon Horman]

On Thu, Apr 17, 2025 at 04:56:58PM +0800, Justin Lai wrote:
> Fix the following compile warning reported by the kernel test robot by
> increasing the size of ivec->name.
> 
> drivers/net/ethernet/realtek/rtase/rtase_main.c: In function 'rtase_open':
> >> drivers/net/ethernet/realtek/rtase/rtase_main.c:1117:52: warning:
> '%i' directive output may be truncated writing between 1 and 10 bytes
> into a region of size between 7 and 22 [-Wformat-truncation=]
>      snprintf(ivec->name, sizeof(ivec->name), "%s_int%i",
>                                                      ^~
>  drivers/net/ethernet/realtek/rtase/rtase_main.c:1117:45: note:
>  directive argument in the range [0, 2147483647]
>      snprintf(ivec->name, sizeof(ivec->name), "%s_int%i",
>                                               ^~~~~~~~~~
>  drivers/net/ethernet/realtek/rtase/rtase_main.c:1117:4: note:
>  'snprintf' output between 6 and 30 bytes into a destination of
>  size 26
>      snprintf(ivec->name, sizeof(ivec->name), "%s_int%i",
>      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>        tp->dev->name, i);
>        ~~~~~~~~~~~~~~~~~

Hi Justin,

Given that the type of i is u16, it's theoretical range of values is [0, 65536].
(I expect that in practice the range is significantly smaller.)

So the string representation of i should fit in the minumum of 7 bytes available
(only a maximum of 5 are needed).

And I do notice that newer compilers do not seem to warn about this.

So I don't really think this needs updating.
And if so, certainly not as a fix for 'net'.

Also, as an aside, as i is unsigned, the format specifier really ought
to be %u instead of %i. Not that it seems to make any difference here
given the range of values discussed above.

> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202503182158.nkAlbJWX-lkp@intel.com/
> Fixes: a36e9f5cfe9e ("rtase: Add support for a pci table in this module")
> Signed-off-by: Justin Lai <justinlai0215@realtek.com>

-- 
pw-bot: changes-requested

RE: [PATCH net v3 2/3] rtase: Increase the size of ivec->name

[Justin Lai]

> On Thu, Apr 17, 2025 at 04:56:58PM +0800, Justin Lai wrote:
> > Fix the following compile warning reported by the kernel test robot by
> > increasing the size of ivec->name.
> >
> > drivers/net/ethernet/realtek/rtase/rtase_main.c: In function 'rtase_open':
> > >> drivers/net/ethernet/realtek/rtase/rtase_main.c:1117:52: warning:
> > '%i' directive output may be truncated writing between 1 and 10 bytes
> > into a region of size between 7 and 22 [-Wformat-truncation=]
> >      snprintf(ivec->name, sizeof(ivec->name), "%s_int%i",
> >                                                      ^~
> >  drivers/net/ethernet/realtek/rtase/rtase_main.c:1117:45: note:
> >  directive argument in the range [0, 2147483647]
> >      snprintf(ivec->name, sizeof(ivec->name), "%s_int%i",
> >                                               ^~~~~~~~~~
> >  drivers/net/ethernet/realtek/rtase/rtase_main.c:1117:4: note:
> >  'snprintf' output between 6 and 30 bytes into a destination of  size
> > 26
> >      snprintf(ivec->name, sizeof(ivec->name), "%s_int%i",
> >      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >        tp->dev->name, i);
> >        ~~~~~~~~~~~~~~~~~
> 
> Hi Justin,
> 
> Given that the type of i is u16, it's theoretical range of values is [0, 65536].
> (I expect that in practice the range is significantly smaller.)
> 
> So the string representation of i should fit in the minumum of 7 bytes available
> (only a maximum of 5 are needed).
> 
> And I do notice that newer compilers do not seem to warn about this.
> 
> So I don't really think this needs updating.
> And if so, certainly not as a fix for 'net'.
> 
> Also, as an aside, as i is unsigned, the format specifier really ought to be %u
> instead of %i. Not that it seems to make any difference here given the range of
> values discussed above.
> 
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes:
> > https://lore.kernel.org/oe-kbuild-all/202503182158.nkAlbJWX-lkp@intel.
> > com/
> > Fixes: a36e9f5cfe9e ("rtase: Add support for a pci table in this
> > module")
> > Signed-off-by: Justin Lai <justinlai0215@realtek.com>
> 
> --
> pw-bot: changes-requested

Hi Simon,

Thank you for your reply. I will modify the format specifier to %u.
Since the warning from the kernel test robot is a false positive, I
will not address this warning, meaning I will not increase the size
of ivec->name. This patch will be posted to net-next.

Thanks,
Justin

Re: [PATCH net v3 2/3] rtase: Increase the size of ivec->name

[Simon Horman]

On Wed, Apr 23, 2025 at 11:53:12AM +0000, Justin Lai wrote:
> > On Thu, Apr 17, 2025 at 04:56:58PM +0800, Justin Lai wrote:
> > > Fix the following compile warning reported by the kernel test robot by
> > > increasing the size of ivec->name.
> > >
> > > drivers/net/ethernet/realtek/rtase/rtase_main.c: In function 'rtase_open':
> > > >> drivers/net/ethernet/realtek/rtase/rtase_main.c:1117:52: warning:
> > > '%i' directive output may be truncated writing between 1 and 10 bytes
> > > into a region of size between 7 and 22 [-Wformat-truncation=]
> > >      snprintf(ivec->name, sizeof(ivec->name), "%s_int%i",
> > >                                                      ^~
> > >  drivers/net/ethernet/realtek/rtase/rtase_main.c:1117:45: note:
> > >  directive argument in the range [0, 2147483647]
> > >      snprintf(ivec->name, sizeof(ivec->name), "%s_int%i",
> > >                                               ^~~~~~~~~~
> > >  drivers/net/ethernet/realtek/rtase/rtase_main.c:1117:4: note:
> > >  'snprintf' output between 6 and 30 bytes into a destination of  size
> > > 26
> > >      snprintf(ivec->name, sizeof(ivec->name), "%s_int%i",
> > >      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >        tp->dev->name, i);
> > >        ~~~~~~~~~~~~~~~~~
> > 
> > Hi Justin,
> > 
> > Given that the type of i is u16, it's theoretical range of values is [0, 65536].
> > (I expect that in practice the range is significantly smaller.)
> > 
> > So the string representation of i should fit in the minumum of 7 bytes available
> > (only a maximum of 5 are needed).
> > 
> > And I do notice that newer compilers do not seem to warn about this.
> > 
> > So I don't really think this needs updating.
> > And if so, certainly not as a fix for 'net'.
> > 
> > Also, as an aside, as i is unsigned, the format specifier really ought to be %u
> > instead of %i. Not that it seems to make any difference here given the range of
> > values discussed above.
> > 
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Closes:
> > > https://lore.kernel.org/oe-kbuild-all/202503182158.nkAlbJWX-lkp@intel.
> > > com/
> > > Fixes: a36e9f5cfe9e ("rtase: Add support for a pci table in this
> > > module")
> > > Signed-off-by: Justin Lai <justinlai0215@realtek.com>
> > 
> > --
> > pw-bot: changes-requested
> 
> Hi Simon,
> 
> Thank you for your reply. I will modify the format specifier to %u.
> Since the warning from the kernel test robot is a false positive, I
> will not address this warning, meaning I will not increase the size
> of ivec->name. This patch will be posted to net-next.

Thanks, sounds good to me.

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

[Justin Lai]

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

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

[Simon Horman]

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

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

[Justin Lai]

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

Hi Simon,

According to a more detailed clarification, this part is actually an
enhancement and does not cause any issues during operation, so it is
not a real bug. Therefore, I will post this patch in net-next.

Thanks,
Justin

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

[Simon Horman]

On Wed, Apr 23, 2025 at 10:53:53AM +0000, Justin Lai 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.
> > 
> > >
> > >       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
> > >
> 
> Hi Simon,
> 
> According to a more detailed clarification, this part is actually an
> enhancement and does not cause any issues during operation, so it is
> not a real bug. Therefore, I will post this patch in net-next.

Thanks. Please do consider using min() instead of min_t() when you post the
patch to net-next.

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

[David Laight]

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

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

[Justin Lai]

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

Hi David,

The boundary protection mechanism is in place to preserve potential
future scalability. I have modified the code to use min instead of
min_t and have posted it to net-next.

Thanks,
Justin