linux-kernel - Re: [PATCH] staging: squash lines for simple wrapper functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <wrfjwpigex25.fsf@redhat.com>
Date:   Mon, 12 Sep 2016 15:29:38 -0400
From:   Jes Sorensen <Jes.Sorensen@...hat.com>
To:     Masahiro Yamada <yamada.masahiro@...ionext.com>
Cc:     Greg KH <gregkh@...uxfoundation.org>, devel@...verdev.osuosl.org,
        linux-fbdev@...r.kernel.org,
        Andreas Dilger <andreas.dilger@...el.com>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        linux-kernel@...r.kernel.org, linux-wireless@...r.kernel.org,
        Larry Finger <Larry.Finger@...inger.net>,
        Teddy Wang <teddy.wang@...iconmotion.com>,
        Sudip Mukherjee <sudipm.mukherjee@...il.com>,
        Laura Abbott <labbott@...hat.com>,
        Arve Hjønnevåg <arve@...roid.com>,
        Riley Andrews <riandrews@...roid.com>,
        Oleg Drokin <oleg.drokin@...el.com>,
        lustre-devel@...ts.lustre.org
Subject: Re: [PATCH] staging: squash lines for simple wrapper functions

Masahiro Yamada <yamada.masahiro@...ionext.com> writes:
> Remove unneeded variables and assignments.
>
> While we are here, clean up the following as well:
>   - refactor rtl8723a_get_bcn_valid() a bit
>   - remove unneeded casts in sii164Get{Vendor,Device}ID()
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@...ionext.com>
> ---
>
>  drivers/staging/android/ion/ion.c                        |  6 +-----
>  .../staging/lustre/lustre/obdclass/linux/linux-module.c  |  5 +----
>  drivers/staging/lustre/lustre/ptlrpc/client.c            |  5 +----
>  drivers/staging/lustre/lustre/ptlrpc/events.c            |  5 +----
>  drivers/staging/rtl8723au/core/rtw_wlan_util.c           |  7 +------
>  drivers/staging/rtl8723au/hal/hal_com.c                  |  6 +-----
>  drivers/staging/sm750fb/ddk750_sii164.c                  | 16 ++++------------
>  7 files changed, 10 insertions(+), 40 deletions(-)

1) Do not submit one giant patch modifying multiple drivers in one go
2) drivers/staging/rtl8723au is gone - had you followed 1), you wouldn't
   necessarily have had to respin this patch.
3) Consider if your changes, even if technically correct, actually
   improve the code (see below)

Jes

> diff --git a/drivers/staging/rtl8723au/core/rtw_wlan_util.c b/drivers/staging/rtl8723au/core/rtw_wlan_util.c
> index 694cf17..7ab47f0 100644
> --- a/drivers/staging/rtl8723au/core/rtw_wlan_util.c
> +++ b/drivers/staging/rtl8723au/core/rtw_wlan_util.c
> @@ -1202,12 +1202,7 @@ unsigned int update_supported_rate23a(unsigned char *ptn, unsigned int ptn_sz)
>  
>  unsigned int update_MSC_rate23a(struct ieee80211_ht_cap *pHT_caps)
>  {
> -	unsigned int mask;
> -
> -	mask = pHT_caps->mcs.rx_mask[0] << 12 |
> -		pHT_caps->mcs.rx_mask[1] << 20;
> -
> -	return mask;
> +	return pHT_caps->mcs.rx_mask[0] << 12 | pHT_caps->mcs.rx_mask[1] << 20;
>  }

The use of the mask variable didn't cause any harm, and it was certainly
a lot nicer to read the way it was.

>  int support_short_GI23a(struct rtw_adapter *padapter,
> diff --git a/drivers/staging/rtl8723au/hal/hal_com.c b/drivers/staging/rtl8723au/hal/hal_com.c
> index 9d7b11b..dfbeebe 100644
> --- a/drivers/staging/rtl8723au/hal/hal_com.c
> +++ b/drivers/staging/rtl8723au/hal/hal_com.c
> @@ -708,11 +708,7 @@ void rtl8723a_bcn_valid(struct rtw_adapter *padapter)
>  
>  bool rtl8723a_get_bcn_valid(struct rtw_adapter *padapter)
>  {
> -	bool retval;
> -
> -	retval = (rtl8723au_read8(padapter, REG_TDECTRL + 2) & BIT(0)) ? true : false;
> -
> -	return retval;
> +	return !!(rtl8723au_read8(padapter, REG_TDECTRL + 2) & BIT(0));
>  }

One word: Yuck!

Talk about obfuscating the code for the sake of obfuscation!

>  void rtl8723a_set_beacon_interval(struct rtw_adapter *padapter, u16 interval)
> diff --git a/drivers/staging/sm750fb/ddk750_sii164.c b/drivers/staging/sm750fb/ddk750_sii164.c
> index 67f36e7..28818e1 100644
> --- a/drivers/staging/sm750fb/ddk750_sii164.c
> +++ b/drivers/staging/sm750fb/ddk750_sii164.c
> @@ -36,12 +36,8 @@ static char *gDviCtrlChipName = "Silicon Image SiI 164";
>   */
>  unsigned short sii164GetVendorID(void)
>  {
> -	unsigned short vendorID;
> -
> -	vendorID = ((unsigned short) i2cReadReg(SII164_I2C_ADDRESS, SII164_VENDOR_ID_HIGH) << 8) |
> -		    (unsigned short) i2cReadReg(SII164_I2C_ADDRESS, SII164_VENDOR_ID_LOW);
> -
> -	return vendorID;
> +	return (i2cReadReg(SII164_I2C_ADDRESS, SII164_VENDOR_ID_HIGH) << 8) |
> +		i2cReadReg(SII164_I2C_ADDRESS, SII164_VENDOR_ID_LOW);
>  }
>  
>  /*
> @@ -53,12 +49,8 @@ unsigned short sii164GetVendorID(void)
>   */
>  unsigned short sii164GetDeviceID(void)
>  {
> -	unsigned short deviceID;
> -
> -	deviceID = ((unsigned short) i2cReadReg(SII164_I2C_ADDRESS, SII164_DEVICE_ID_HIGH) << 8) |
> -		    (unsigned short) i2cReadReg(SII164_I2C_ADDRESS, SII164_DEVICE_ID_LOW);
> -
> -	return deviceID;
> +	return (i2cReadReg(SII164_I2C_ADDRESS, SII164_DEVICE_ID_HIGH) << 8) |
> +		i2cReadReg(SII164_I2C_ADDRESS, SII164_DEVICE_ID_LOW);
>  }

Getting rid of the casts would be nice, merging them into a giant
multi-line return line certainly isn't an improvement in my book. That
said, I will leave that to the maintainer of that driver to decide what
is preferred.

Jes