diff mbox series

staging: rtl8723bs: Replace ternary statement with min function

Message ID Y1neXqVYZ8mq8jH+@marshmallow
State New
Headers
Series staging: rtl8723bs: Replace ternary statement with min function |

Commit Message

Emily Peri Oct. 27, 2022, 1:26 a.m. UTC 
  Ternary statements that pick the min of two values can be replaced by
the built-in min() function. This improves readability, since its quicker
to understand min(x, y) than x < y ? x : y. Issue found by coccicheck.

Signed-off-by: Emily Peri <eperi1024@gmail.com>
---
 drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Julia Lawall Oct. 27, 2022, 6:12 a.m. UTC | #1 
On Wed, 26 Oct 2022, Emily Peri wrote:

> Ternary statements that pick the min of two values can be replaced by
> the built-in min() function. This improves readability, since its quicker
> to understand min(x, y) than x < y ? x : y. Issue found by coccicheck.

It looks like a nice change, but I get a compiler error afer the patch,

julia

>
> Signed-off-by: Emily Peri <eperi1024@gmail.com>
> ---
>  drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> index 6aeb169c6ebf..0cf7d9f82b83 100644
> --- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> +++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> @@ -1551,7 +1551,7 @@ static int rtw_cfg80211_set_wpa_ie(struct adapter *padapter, u8 *pie, size_t iel
>
>  		wps_ie = rtw_get_wps_ie(buf, ielen, NULL, &wps_ielen);
>  		if (wps_ie && wps_ielen > 0) {
> -			padapter->securitypriv.wps_ie_len = wps_ielen < MAX_WPS_IE_LEN ? wps_ielen : MAX_WPS_IE_LEN;
> +			padapter->securitypriv.wps_ie_len = min(wps_ielen, MAX_WPS_IE_LEN);
>  			memcpy(padapter->securitypriv.wps_ie, wps_ie, padapter->securitypriv.wps_ie_len);
>  			set_fwstate(&padapter->mlmepriv, WIFI_UNDER_WPS);
>  		} else {
> --
> 2.34.1
>
>
>
Dan Carpenter Oct. 27, 2022, 7:32 a.m. UTC | #2 
On Wed, Oct 26, 2022 at 06:26:54PM -0700, Emily Peri wrote:
> Ternary statements that pick the min of two values can be replaced by
> the built-in min() function. This improves readability, since its quicker
> to understand min(x, y) than x < y ? x : y. Issue found by coccicheck.
> 
> Signed-off-by: Emily Peri <eperi1024@gmail.com>

This breaks the build.  Use min_t(uint, wps_ielen, MAX_WPS_IE_LEN);

regards,
dan carpenter
Emily Peri Oct. 28, 2022, 5:20 p.m. UTC | #3 
On Thu, Oct 27, 2022 at 08:12:14AM +0200, Julia Lawall wrote:
> 
> 
> On Wed, 26 Oct 2022, Emily Peri wrote:
> 
> > Ternary statements that pick the min of two values can be replaced by
> > the built-in min() function. This improves readability, since its quicker
> > to understand min(x, y) than x < y ? x : y. Issue found by coccicheck.
> 
> It looks like a nice change, but I get a compiler error afer the patch,
> 
> julia

Okay, I think I know the issue. Another person commented (Dan Carpenter)
that the function needs to be min_t, I'm guessing so the function
returns the value as the correct type. But one thing I'm wondering is,
why I didn't I get a compiler error when I compiled the kernel myself?

Also-- this is just a general question, when we work on drivers I'm
guessing we don't need to include header files from include/linux,
right? 

> >
> > Signed-off-by: Emily Peri <eperi1024@gmail.com>
> > ---
> >  drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> > index 6aeb169c6ebf..0cf7d9f82b83 100644
> > --- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> > +++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> > @@ -1551,7 +1551,7 @@ static int rtw_cfg80211_set_wpa_ie(struct adapter *padapter, u8 *pie, size_t iel
> >
> >  		wps_ie = rtw_get_wps_ie(buf, ielen, NULL, &wps_ielen);
> >  		if (wps_ie && wps_ielen > 0) {
> > -			padapter->securitypriv.wps_ie_len = wps_ielen < MAX_WPS_IE_LEN ? wps_ielen : MAX_WPS_IE_LEN;
> > +			padapter->securitypriv.wps_ie_len = min(wps_ielen, MAX_WPS_IE_LEN);
> >  			memcpy(padapter->securitypriv.wps_ie, wps_ie, padapter->securitypriv.wps_ie_len);
> >  			set_fwstate(&padapter->mlmepriv, WIFI_UNDER_WPS);
> >  		} else {
> > --
> > 2.34.1
> >
> >
> >
Emily Peri Oct. 28, 2022, 5:52 p.m. UTC | #4 
On Thu, Oct 27, 2022 at 10:32:15AM +0300, Dan Carpenter wrote:
> On Wed, Oct 26, 2022 at 06:26:54PM -0700, Emily Peri wrote:
> > Ternary statements that pick the min of two values can be replaced by
> > the built-in min() function. This improves readability, since its quicker
> > to understand min(x, y) than x < y ? x : y. Issue found by coccicheck.
> > 
> > Signed-off-by: Emily Peri <eperi1024@gmail.com>
> 
> This breaks the build.  Use min_t(uint, wps_ielen, MAX_WPS_IE_LEN);
> 
> regards,
> dan carpenter

Oh! Thanks for the feedback, that makes sense! When you say 'breaks the
build,' do you mean it didn't compile, or the module didn't load (or
something else)? I'm trying to figure out what I did wrong when testing
it.

Best,
Emily
Julia Lawall Oct. 28, 2022, 6:39 p.m. UTC | #5 
On Fri, 28 Oct 2022, Emily Peri wrote:

> On Thu, Oct 27, 2022 at 10:32:15AM +0300, Dan Carpenter wrote:
> > On Wed, Oct 26, 2022 at 06:26:54PM -0700, Emily Peri wrote:
> > > Ternary statements that pick the min of two values can be replaced by
> > > the built-in min() function. This improves readability, since its quicker
> > > to understand min(x, y) than x < y ? x : y. Issue found by coccicheck.
> > >
> > > Signed-off-by: Emily Peri <eperi1024@gmail.com>
> >
> > This breaks the build.  Use min_t(uint, wps_ielen, MAX_WPS_IE_LEN);
> >
> > regards,
> > dan carpenter
>
> Oh! Thanks for the feedback, that makes sense! When you say 'breaks the
> build,' do you mean it didn't compile, or the module didn't load (or
> something else)? I'm trying to figure out what I did wrong when testing
> it.

Normally it means that it didn't compile.  Check that you actually have a
.o file for the affected file.

julia
kernel test robot Oct. 29, 2022, 3:08 a.m. UTC | #6 
Hi Emily,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on staging/staging-testing]

url:    https://github.com/intel-lab-lkp/linux/commits/Emily-Peri/staging-rtl8723bs-Replace-ternary-statement-with-min-function/20221027-092729
patch link:    https://lore.kernel.org/r/Y1neXqVYZ8mq8jH%2B%40marshmallow
patch subject: [PATCH] staging: rtl8723bs: Replace ternary statement with min function
config: powerpc-allyesconfig
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 791a7ae1ba3efd6bca96338e10ffde557ba83920)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install powerpc cross compiling tool for clang build
        # apt-get install binutils-powerpc-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/c222133e1748e829da70718b25eccc968ab1a073
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Emily-Peri/staging-rtl8723bs-Replace-ternary-statement-with-min-function/20221027-092729
        git checkout c222133e1748e829da70718b25eccc968ab1a073
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash drivers/staging/rtl8723bs/ drivers/tty/serial/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c:1554:40: warning: comparison of distinct pointer types ('typeof (wps_ielen) *' (aka 'unsigned int *') and 'typeof ((512)) *' (aka 'int *')) [-Wcompare-distinct-pointer-types]
                           padapter->securitypriv.wps_ie_len = min(wps_ielen, MAX_WPS_IE_LEN);
                                                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/minmax.h:45:19: note: expanded from macro 'min'
   #define min(x, y)       __careful_cmp(x, y, <)
                           ^~~~~~~~~~~~~~~~~~~~~~
   include/linux/minmax.h:36:24: note: expanded from macro '__careful_cmp'
           __builtin_choose_expr(__safe_cmp(x, y), \
                                 ^~~~~~~~~~~~~~~~
   include/linux/minmax.h:26:4: note: expanded from macro '__safe_cmp'
                   (__typecheck(x, y) && __no_side_effects(x, y))
                    ^~~~~~~~~~~~~~~~~
   include/linux/minmax.h:20:28: note: expanded from macro '__typecheck'
           (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
                      ~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~
   1 warning generated.


vim +1554 drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c

  1443	
  1444	static int rtw_cfg80211_set_wpa_ie(struct adapter *padapter, u8 *pie, size_t ielen)
  1445	{
  1446		u8 *buf = NULL;
  1447		int group_cipher = 0, pairwise_cipher = 0;
  1448		int ret = 0;
  1449		int wpa_ielen = 0;
  1450		int wpa2_ielen = 0;
  1451		u8 *pwpa, *pwpa2;
  1452		u8 null_addr[] = {0, 0, 0, 0, 0, 0};
  1453	
  1454		if (!pie || !ielen) {
  1455			/* Treat this as normal case, but need to clear WIFI_UNDER_WPS */
  1456			_clr_fwstate_(&padapter->mlmepriv, WIFI_UNDER_WPS);
  1457			goto exit;
  1458		}
  1459	
  1460		if (ielen > MAX_WPA_IE_LEN+MAX_WPS_IE_LEN+MAX_P2P_IE_LEN) {
  1461			ret = -EINVAL;
  1462			goto exit;
  1463		}
  1464	
  1465		buf = rtw_zmalloc(ielen);
  1466		if (!buf) {
  1467			ret =  -ENOMEM;
  1468			goto exit;
  1469		}
  1470	
  1471		memcpy(buf, pie, ielen);
  1472	
  1473		if (ielen < RSN_HEADER_LEN) {
  1474			ret  = -1;
  1475			goto exit;
  1476		}
  1477	
  1478		pwpa = rtw_get_wpa_ie(buf, &wpa_ielen, ielen);
  1479		if (pwpa && wpa_ielen > 0) {
  1480			if (rtw_parse_wpa_ie(pwpa, wpa_ielen+2, &group_cipher, &pairwise_cipher, NULL) == _SUCCESS) {
  1481				padapter->securitypriv.dot11AuthAlgrthm = dot11AuthAlgrthm_8021X;
  1482				padapter->securitypriv.ndisauthtype = Ndis802_11AuthModeWPAPSK;
  1483				memcpy(padapter->securitypriv.supplicant_ie, &pwpa[0], wpa_ielen+2);
  1484			}
  1485		}
  1486	
  1487		pwpa2 = rtw_get_wpa2_ie(buf, &wpa2_ielen, ielen);
  1488		if (pwpa2 && wpa2_ielen > 0) {
  1489			if (rtw_parse_wpa2_ie(pwpa2, wpa2_ielen+2, &group_cipher, &pairwise_cipher, NULL) == _SUCCESS) {
  1490				padapter->securitypriv.dot11AuthAlgrthm = dot11AuthAlgrthm_8021X;
  1491				padapter->securitypriv.ndisauthtype = Ndis802_11AuthModeWPA2PSK;
  1492				memcpy(padapter->securitypriv.supplicant_ie, &pwpa2[0], wpa2_ielen+2);
  1493			}
  1494		}
  1495	
  1496		if (group_cipher == 0)
  1497			group_cipher = WPA_CIPHER_NONE;
  1498	
  1499		if (pairwise_cipher == 0)
  1500			pairwise_cipher = WPA_CIPHER_NONE;
  1501	
  1502		switch (group_cipher) {
  1503		case WPA_CIPHER_NONE:
  1504			padapter->securitypriv.dot118021XGrpPrivacy = _NO_PRIVACY_;
  1505			padapter->securitypriv.ndisencryptstatus = Ndis802_11EncryptionDisabled;
  1506			break;
  1507		case WPA_CIPHER_WEP40:
  1508			padapter->securitypriv.dot118021XGrpPrivacy = _WEP40_;
  1509			padapter->securitypriv.ndisencryptstatus = Ndis802_11Encryption1Enabled;
  1510			break;
  1511		case WPA_CIPHER_TKIP:
  1512			padapter->securitypriv.dot118021XGrpPrivacy = _TKIP_;
  1513			padapter->securitypriv.ndisencryptstatus = Ndis802_11Encryption2Enabled;
  1514			break;
  1515		case WPA_CIPHER_CCMP:
  1516			padapter->securitypriv.dot118021XGrpPrivacy = _AES_;
  1517			padapter->securitypriv.ndisencryptstatus = Ndis802_11Encryption3Enabled;
  1518			break;
  1519		case WPA_CIPHER_WEP104:
  1520			padapter->securitypriv.dot118021XGrpPrivacy = _WEP104_;
  1521			padapter->securitypriv.ndisencryptstatus = Ndis802_11Encryption1Enabled;
  1522			break;
  1523		}
  1524	
  1525		switch (pairwise_cipher) {
  1526		case WPA_CIPHER_NONE:
  1527			padapter->securitypriv.dot11PrivacyAlgrthm = _NO_PRIVACY_;
  1528			padapter->securitypriv.ndisencryptstatus = Ndis802_11EncryptionDisabled;
  1529			break;
  1530		case WPA_CIPHER_WEP40:
  1531			padapter->securitypriv.dot11PrivacyAlgrthm = _WEP40_;
  1532			padapter->securitypriv.ndisencryptstatus = Ndis802_11Encryption1Enabled;
  1533			break;
  1534		case WPA_CIPHER_TKIP:
  1535			padapter->securitypriv.dot11PrivacyAlgrthm = _TKIP_;
  1536			padapter->securitypriv.ndisencryptstatus = Ndis802_11Encryption2Enabled;
  1537			break;
  1538		case WPA_CIPHER_CCMP:
  1539			padapter->securitypriv.dot11PrivacyAlgrthm = _AES_;
  1540			padapter->securitypriv.ndisencryptstatus = Ndis802_11Encryption3Enabled;
  1541			break;
  1542		case WPA_CIPHER_WEP104:
  1543			padapter->securitypriv.dot11PrivacyAlgrthm = _WEP104_;
  1544			padapter->securitypriv.ndisencryptstatus = Ndis802_11Encryption1Enabled;
  1545			break;
  1546		}
  1547	
  1548		{/* handle wps_ie */
  1549			uint wps_ielen;
  1550			u8 *wps_ie;
  1551	
  1552			wps_ie = rtw_get_wps_ie(buf, ielen, NULL, &wps_ielen);
  1553			if (wps_ie && wps_ielen > 0) {
> 1554				padapter->securitypriv.wps_ie_len = min(wps_ielen, MAX_WPS_IE_LEN);
  1555				memcpy(padapter->securitypriv.wps_ie, wps_ie, padapter->securitypriv.wps_ie_len);
  1556				set_fwstate(&padapter->mlmepriv, WIFI_UNDER_WPS);
  1557			} else {
  1558				_clr_fwstate_(&padapter->mlmepriv, WIFI_UNDER_WPS);
  1559			}
  1560		}
  1561	
  1562		/* TKIP and AES disallow multicast packets until installing group key */
  1563		if (padapter->securitypriv.dot11PrivacyAlgrthm == _TKIP_
  1564			|| padapter->securitypriv.dot11PrivacyAlgrthm == _TKIP_WTMIC_
  1565			|| padapter->securitypriv.dot11PrivacyAlgrthm == _AES_)
  1566			/* WPS open need to enable multicast */
  1567			/*  check_fwstate(&padapter->mlmepriv, WIFI_UNDER_WPS) == true) */
  1568			rtw_hal_set_hwreg(padapter, HW_VAR_OFF_RCR_AM, null_addr);
  1569	
  1570	exit:
  1571		kfree(buf);
  1572		if (ret)
  1573			_clr_fwstate_(&padapter->mlmepriv, WIFI_UNDER_WPS);
  1574		return ret;
  1575	}
  1576
diff mbox series

Patch

diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
index 6aeb169c6ebf..0cf7d9f82b83 100644
--- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
+++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
@@ -1551,7 +1551,7 @@  static int rtw_cfg80211_set_wpa_ie(struct adapter *padapter, u8 *pie, size_t iel
 
 		wps_ie = rtw_get_wps_ie(buf, ielen, NULL, &wps_ielen);
 		if (wps_ie && wps_ielen > 0) {
-			padapter->securitypriv.wps_ie_len = wps_ielen < MAX_WPS_IE_LEN ? wps_ielen : MAX_WPS_IE_LEN;
+			padapter->securitypriv.wps_ie_len = min(wps_ielen, MAX_WPS_IE_LEN);
 			memcpy(padapter->securitypriv.wps_ie, wps_ie, padapter->securitypriv.wps_ie_len);
 			set_fwstate(&padapter->mlmepriv, WIFI_UNDER_WPS);
 		} else {