staging: rtl8723bs: replace ternary comparison with min_t()

[PATCH v2] staging: rtl8723bs: replace ternary comparison with min_t()

Posted by William Hansen-Baird 6 days, 20 hours ago

Replace ternary comparison for setting copy_len with the min_t() function
from linux/minmax.h.
Change is purely cosmetic, however makes the line
easier to read without having to reason about logic.

---
v2:
Drop unrelated style changes and fix style issues by running checkpatch.
Keep min_t() to make the signedness of the comparison explicit.
(copy_len is an integer, wpa_ie_len is an integer but sizeof yields
size_t)

Signed-off-by: William Hansen-Baird <william.hansen.baird@gmail.com>
---
 drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index 8e41eb61448f..d36f6c3bc9b4 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
@@ -4,6 +4,7 @@
  * Copyright(c) 2007 - 2012 Realtek Corporation. All rights reserved.
  *
  ******************************************************************************/
+#include <linux/minmax.h>
 #include <drv_types.h>
 #include <rtw_wifi_regd.h>
 #include <hal_btcoex.h>
@@ -1024,7 +1025,7 @@ static unsigned short rtw_parse_assoc_security_ies(struct adapter *padapter,
 			pstat->flags |= WLAN_STA_WPS;
 			copy_len = 0;
 		} else {
-			copy_len = ((wpa_ie_len+2) > sizeof(pstat->wpa_ie)) ? (sizeof(pstat->wpa_ie)):(wpa_ie_len+2);
+			copy_len = min_t(int, sizeof(pstat->wpa_ie), wpa_ie_len + 2);
 		}
 
 
-- 
2.52.0

Re: [PATCH v2] staging: rtl8723bs: replace ternary comparison with min_t()

Posted by Dan Carpenter 6 days, 18 hours ago

On Sat, Jan 31, 2026 at 08:31:46AM -0500, William Hansen-Baird wrote:
> Replace ternary comparison for setting copy_len with the min_t() function
> from linux/minmax.h.
> Change is purely cosmetic, however makes the line
> easier to read without having to reason about logic.
> 
> ---
> v2:
> Drop unrelated style changes and fix style issues by running checkpatch.
> Keep min_t() to make the signedness of the comparison explicit.
> (copy_len is an integer, wpa_ie_len is an integer but sizeof yields
> size_t)
> 

This stuff is supposed to got after the --- cut off line.

> Signed-off-by: William Hansen-Baird <william.hansen.baird@gmail.com>
> ---
  ^^^

>  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> index 8e41eb61448f..d36f6c3bc9b4 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> @@ -4,6 +4,7 @@
>   * Copyright(c) 2007 - 2012 Realtek Corporation. All rights reserved.
>   *
>   ******************************************************************************/
> +#include <linux/minmax.h>
>  #include <drv_types.h>
>  #include <rtw_wifi_regd.h>
>  #include <hal_btcoex.h>

These are supposed to go in alphabetical order, kind of.

> @@ -1024,7 +1025,7 @@ static unsigned short rtw_parse_assoc_security_ies(struct adapter *padapter,
>  			pstat->flags |= WLAN_STA_WPS;
>  			copy_len = 0;
>  		} else {
> -			copy_len = ((wpa_ie_len+2) > sizeof(pstat->wpa_ie)) ? (sizeof(pstat->wpa_ie)):(wpa_ie_len+2);
> +			copy_len = min_t(int, sizeof(pstat->wpa_ie), wpa_ie_len + 2);

Use umin().  "int" is wrong because we don't want negative values
stored in copy_len.

regards,
dan carpenter

Re: [PATCH v2] staging: rtl8723bs: replace ternary comparison with min_t()

Posted by David Laight 6 days, 17 hours ago

On Sat, 31 Jan 2026 17:43:10 +0300
Dan Carpenter <dan.carpenter@linaro.org> wrote:

> On Sat, Jan 31, 2026 at 08:31:46AM -0500, William Hansen-Baird wrote:
> > Replace ternary comparison for setting copy_len with the min_t() function
> > from linux/minmax.h.
> > Change is purely cosmetic, however makes the line
> > easier to read without having to reason about logic.
> > 
> > ---
> > v2:
> > Drop unrelated style changes and fix style issues by running checkpatch.
> > Keep min_t() to make the signedness of the comparison explicit.
> > (copy_len is an integer, wpa_ie_len is an integer but sizeof yields
> > size_t)
> >   
> 
> This stuff is supposed to got after the --- cut off line.
> 
> > Signed-off-by: William Hansen-Baird <william.hansen.baird@gmail.com>
> > ---  
>   ^^^
> 
> >  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> > index 8e41eb61448f..d36f6c3bc9b4 100644
> > --- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> > +++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> > @@ -4,6 +4,7 @@
> >   * Copyright(c) 2007 - 2012 Realtek Corporation. All rights reserved.
> >   *
> >   ******************************************************************************/
> > +#include <linux/minmax.h>
> >  #include <drv_types.h>
> >  #include <rtw_wifi_regd.h>
> >  #include <hal_btcoex.h>  
> 
> These are supposed to go in alphabetical order, kind of.

I'd always put system/global headers first, but that doesn't seem to be the standard here.

> 
> > @@ -1024,7 +1025,7 @@ static unsigned short rtw_parse_assoc_security_ies(struct adapter *padapter,
> >  			pstat->flags |= WLAN_STA_WPS;
> >  			copy_len = 0;
> >  		} else {
> > -			copy_len = ((wpa_ie_len+2) > sizeof(pstat->wpa_ie)) ? (sizeof(pstat->wpa_ie)):(wpa_ie_len+2);
> > +			copy_len = min_t(int, sizeof(pstat->wpa_ie), wpa_ie_len + 2);  
> 
> Use umin().  "int" is wrong because we don't want negative values
> stored in copy_len.

And the correct way to fix min() bleating is to change the type of the variables.
Using min_t() ought to be frowned up and only used as a last resort.

In this case I think all the variables declared with wpa_ie_len can/should be
unsigned int.

	David

> 
> regards,
> dan carpenter
> 
>

Re: [PATCH v2] staging: rtl8723bs: replace ternary comparison with min_t()

Posted by Dan Carpenter 6 days, 16 hours ago

On Sat, Jan 31, 2026 at 03:55:25PM +0000, David Laight wrote:
> > > @@ -1024,7 +1025,7 @@ static unsigned short rtw_parse_assoc_security_ies(struct adapter *padapter,
> > >  			pstat->flags |= WLAN_STA_WPS;
> > >  			copy_len = 0;
> > >  		} else {
> > > -			copy_len = ((wpa_ie_len+2) > sizeof(pstat->wpa_ie)) ? (sizeof(pstat->wpa_ie)):(wpa_ie_len+2);
> > > +			copy_len = min_t(int, sizeof(pstat->wpa_ie), wpa_ie_len + 2);  
> > 
> > Use umin().  "int" is wrong because we don't want negative values
> > stored in copy_len.
> 
> And the correct way to fix min() bleating is to change the type of the variables.
> Using min_t() ought to be frowned up and only used as a last resort.
> 
> In this case I think all the variables declared with wpa_ie_len can/should be
> unsigned int.
> 

That also works.

regards,
dan carpenter

Re: [PATCH v2] staging: rtl8723bs: replace ternary comparison with min_t()

Posted by William Hansen-Baird 6 days, 11 hours ago

On Sat, Jan 31, 2026 at 08:35:55PM +0300, Dan Carpenter wrote:
> On Sat, Jan 31, 2026 at 03:55:25PM +0000, David Laight wrote:
> > > > @@ -1024,7 +1025,7 @@ static unsigned short rtw_parse_assoc_security_ies(struct adapter *padapter,
> > > >  			pstat->flags |= WLAN_STA_WPS;
> > > >  			copy_len = 0;
> > > >  		} else {
> > > > -			copy_len = ((wpa_ie_len+2) > sizeof(pstat->wpa_ie)) ? (sizeof(pstat->wpa_ie)):(wpa_ie_len+2);
> > > > +			copy_len = min_t(int, sizeof(pstat->wpa_ie), wpa_ie_len + 2);  
> > > 
> > > Use umin().  "int" is wrong because we don't want negative values
> > > stored in copy_len.
> > 
> > And the correct way to fix min() bleating is to change the type of the variables.
> > Using min_t() ought to be frowned up and only used as a last resort.
> > 
> > In this case I think all the variables declared with wpa_ie_len can/should be
> > unsigned int.
> > 
> 
> That also works.
> 
Hi Dan and David,

Thanks for your help. After reviewing the code, I do see a few checks 
ensuring wpa_ie_len is never negative.
Therefore umin() is safe in this case, and for v3 I'll update the patch accordingly.

Would you prefer I leave it at that, or is a broader change to 
make wpa_ie_len unsigned worth doing in a separate patch?

Thanks,
William

Re: [PATCH v2] staging: rtl8723bs: replace ternary comparison with min_t()

Posted by David Laight 6 days, 10 hours ago

On Sat, 31 Jan 2026 16:43:54 -0500
William Hansen-Baird <william.hansen.baird@gmail.com> wrote:

> On Sat, Jan 31, 2026 at 08:35:55PM +0300, Dan Carpenter wrote:
> > On Sat, Jan 31, 2026 at 03:55:25PM +0000, David Laight wrote:  
> > > > > @@ -1024,7 +1025,7 @@ static unsigned short rtw_parse_assoc_security_ies(struct adapter *padapter,
> > > > >  			pstat->flags |= WLAN_STA_WPS;
> > > > >  			copy_len = 0;
> > > > >  		} else {
> > > > > -			copy_len = ((wpa_ie_len+2) > sizeof(pstat->wpa_ie)) ? (sizeof(pstat->wpa_ie)):(wpa_ie_len+2);
> > > > > +			copy_len = min_t(int, sizeof(pstat->wpa_ie), wpa_ie_len + 2);    
> > > > 
> > > > Use umin().  "int" is wrong because we don't want negative values
> > > > stored in copy_len.  
> > > 
> > > And the correct way to fix min() bleating is to change the type of the variables.
> > > Using min_t() ought to be frowned up and only used as a last resort.
> > > 
> > > In this case I think all the variables declared with wpa_ie_len can/should be
> > > unsigned int.
> > >   
> > 
> > That also works.
> >   
> Hi Dan and David,
> 
> Thanks for your help. After reviewing the code, I do see a few checks 
> ensuring wpa_ie_len is never negative.
> Therefore umin() is safe in this case, and for v3 I'll update the patch accordingly.

umin() is a bit better than min_t().

A subtle change is doing 'wpa_ie_len + 2u'.

> Would you prefer I leave it at that, or is a broader change to 
> make wpa_ie_len unsigned worth doing in a separate patch?

I scanned the code some of the variables seem to be different signedness 
in different functions.

But a 'false error' from min() is really an indication you should be looking
at changing the types of the variables themselves.

My main thought looking at that code is that functions are far too long
and have far too many local variables.
You also pretty much don't want to use u8/u16 for locals just because the
values are small, it doesn't (really) save space and can increase code size.

	David

> 
> Thanks,
> William