Re: [PATCH] staging: Add rtl8723bs sdio wifi driver

Re: [PATCH] staging: Add rtl8723bs sdio wifi driver

[Hans de Goede]

Hi All,

I forgot to add --compose so I could add the cover letter I had
already written, so here it goes:

Hi Greg, all,

I'm hereby submitting the rtl8723bs driver which
Bastien Nocera and Larry Finger have been maintaining at:

https://github.com/hadess/rtl8723bs

For inclusion into staging, like most of the other rtl*
drivers in staging this is a cleaned up version of Realtek's
own driver code for Linux.

I would like to see this added to staging because the
rtl8723bs is found on quite a few systems used by Linux users,
such as on Atom systems (Intel Computestick and various other
Atom based devices) and on many (budget) ARM boards such as
the CHIP.

The plan moving forward with this is for the new clean,
written from scratch, rtl8xxxu driver to eventually gain
support for sdio devices. But there is no clear timeline
for that as Jes is doing that in his spare time, so
I would like to see this driver included in staging for now.

Regards,

Hans

Re: [PATCH] staging: Add rtl8723bs sdio wifi driver

[Larry Finger]

On 03/29/2017 12:47 PM, Hans de Goede wrote:
> The rtl8723bs is found on quite a few systems used by Linux users,
> such as on Atom systems (Intel Computestick and various other
> Atom based devices) and on many (budget) ARM boards such as
> the CHIP.
>
> The plan moving forward with this is for the new clean,
> written from scratch, rtl8xxxu driver to eventually gain
> support for sdio devices. But there is no clear timeline
> for that, so lets add this driver included in staging for now.
>
> Cc: Bastien Nocera <hadess@hadess.net>
> Cc: Larry Finger <Larry.Finger@lwfinger.net>
> Cc: Jes Sorensen <jes.sorensen@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Hans,

I am still building the driver on my CherryTrail tablet, but I did see some 
warnings and errors listed by Smatch as follows:

finger@linux-4v1g:~/wireless-drivers-next> make CHECK=~/smatch/smatch C=2 
drivers/staging/rtl8723bs/
   CHK     include/config/kernel.release
   CHK     include/generated/uapi/linux/version.h
   CHK     include/generated/utsrelease.h
   CHECK   arch/x86/purgatory/purgatory.c
   CHECK   arch/x86/purgatory/sha256.c
   CHECK   arch/x86/purgatory/string.c
   CHK     include/generated/bounds.h
   CHK     include/generated/timeconst.h
   CHK     include/generated/asm-offsets.h
   CALL    scripts/checksyscalls.sh
   CHECK   scripts/mod/empty.c
   CHECK   drivers/staging/rtl8723bs/core/rtw_ap.c
drivers/staging/rtl8723bs/core/rtw_ap.c:382 expire_timeout_chk() warn: 
inconsistent indenting
   CHECK   drivers/staging/rtl8723bs/core/rtw_btcoex.c
   CHECK   drivers/staging/rtl8723bs/core/rtw_cmd.c
   CHECK   drivers/staging/rtl8723bs/core/rtw_debug.c
drivers/staging/rtl8723bs/core/rtw_debug.c:454 proc_get_survey_info() error: we 
previously assumed 'phead' could be null (see line 453)
drivers/staging/rtl8723bs/core/rtw_debug.c:455 proc_get_survey_info() warn: 
variable dereferenced before check 'phead' (see line 454)
   CHECK   drivers/staging/rtl8723bs/core/rtw_efuse.c
   CHECK   drivers/staging/rtl8723bs/core/rtw_io.c
   CHECK   drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
   CHECK   drivers/staging/rtl8723bs/core/rtw_ieee80211.c
drivers/staging/rtl8723bs/core/rtw_ieee80211.c:83 rtw_is_cckrates_included() 
warn: if statement not indented
drivers/staging/rtl8723bs/core/rtw_ieee80211.c:98 rtw_is_cckratesonly_included() 
warn: if statement not indented
   CHECK   drivers/staging/rtl8723bs/core/rtw_mlme.c
drivers/staging/rtl8723bs/core/rtw_mlme.c:862 rtw_survey_event_callback() warn: 
inconsistent indenting
drivers/staging/rtl8723bs/core/rtw_mlme.c:1102 rtw_free_assoc_resources() warn: 
if statement not indented
drivers/staging/rtl8723bs/core/rtw_mlme.c:1165 rtw_indicate_disconnect() warn: 
inconsistent indenting
drivers/staging/rtl8723bs/core/rtw_mlme.c:2830 rtw_restructure_ht_ie() warn: 
inconsistent indenting
drivers/staging/rtl8723bs/core/rtw_mlme.c:2991 rtw_update_ht_cap() warn: 
inconsistent indenting
   CHECK   drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:525 _mgt_dispatcher() warn: 
inconsistent indenting
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:1595 OnAssocReq() error: buffer 
overflow 'pstapriv->sta_aid' 32 <= 32
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:2391 dump_mgntframe_and_wait() 
warn: inconsistent indenting
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:2420 dump_mgntframe_and_wait_ack() 
warn: inconsistent indenting
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:4969 process_80211d() error: 
testing array offset 'i' after use.
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:5738 linked_status_chk() warn: 
inconsistent indenting
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:6459 sitesurvey_cmd_hdl() warn: 
inconsistent indenting
   CHECK   drivers/staging/rtl8723bs/core/rtw_odm.c
drivers/staging/rtl8723bs/core/rtw_odm.c:109 rtw_odm_dbg_comp_msg() warn: if 
statement not indented
drivers/staging/rtl8723bs/core/rtw_odm.c:146 rtw_odm_ability_msg() warn: if 
statement not indented
   CHECK   drivers/staging/rtl8723bs/core/rtw_pwrctrl.c
drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:641 LeaveAllPowerSaveModeDirect() 
warn: inconsistent indenting
   CHECK   drivers/staging/rtl8723bs/core/rtw_recv.c
drivers/staging/rtl8723bs/core/rtw_recv.c:598 portctrl() warn: inconsistent 
indenting
drivers/staging/rtl8723bs/core/rtw_recv.c:838 sta2sta_data_frame() warn: 
inconsistent indenting
drivers/staging/rtl8723bs/core/rtw_recv.c:1547 validate_recv_frame() warn: 
inconsistent indenting
   CHECK   drivers/staging/rtl8723bs/core/rtw_rf.c
   CHECK   drivers/staging/rtl8723bs/core/rtw_security.c
drivers/staging/rtl8723bs/core/rtw_security.c:266 rtw_wep_encrypt() warn: 
inconsistent indenting
drivers/staging/rtl8723bs/core/rtw_security.c:433 rtw_seccalctkipmic() warn: 
inconsistent indenting
drivers/staging/rtl8723bs/core/rtw_security.c:749 rtw_tkip_encrypt() warn: 
inconsistent indenting
drivers/staging/rtl8723bs/core/rtw_security.c:865 rtw_tkip_decrypt() warn: 
inconsistent indenting
drivers/staging/rtl8723bs/core/rtw_security.c:1383 aes_cipher() warn: 
inconsistent indenting
drivers/staging/rtl8723bs/core/rtw_security.c:1415 aes_cipher() warn: 
inconsistent indenting
drivers/staging/rtl8723bs/core/rtw_security.c:1430 aes_cipher() warn: 
inconsistent indenting
drivers/staging/rtl8723bs/core/rtw_security.c:1582 rtw_aes_encrypt() warn: 
inconsistent indenting
drivers/staging/rtl8723bs/core/rtw_security.c:1651 aes_decipher() warn: 
inconsistent indenting
drivers/staging/rtl8723bs/core/rtw_security.c:1739 aes_decipher() warn: 
inconsistent indenting
drivers/staging/rtl8723bs/core/rtw_security.c:1792 aes_decipher() warn: curly 
braces intended?
drivers/staging/rtl8723bs/core/rtw_security.c:1809 aes_decipher() warn: 
inconsistent indenting
   CHECK   drivers/staging/rtl8723bs/core/rtw_sta_mgt.c
drivers/staging/rtl8723bs/core/rtw_sta_mgt.c:25 _rtw_init_stainfo() warn: 
inconsistent indenting
   CHECK   drivers/staging/rtl8723bs/core/rtw_wlan_util.c
drivers/staging/rtl8723bs/core/rtw_wlan_util.c:67 cckrates_included() warn: if 
statement not indented
drivers/staging/rtl8723bs/core/rtw_wlan_util.c:81 cckratesonly_included() warn: 
if statement not indented
drivers/staging/rtl8723bs/core/rtw_wlan_util.c:815 rtw_camid_alloc() warn: 
should '1 << (cam_id)' be a 64 bit type?
   CHECK   drivers/staging/rtl8723bs/core/rtw_xmit.c
drivers/staging/rtl8723bs/core/rtw_xmit.c:277 _rtw_init_xmit_priv() warn: 
inconsistent indenting
drivers/staging/rtl8723bs/core/rtw_xmit.c:294 _rtw_free_xmit_priv() warn: 
inconsistent indenting
drivers/staging/rtl8723bs/core/rtw_xmit.c:295 _rtw_free_xmit_priv() warn: 
inconsistent indenting
drivers/staging/rtl8723bs/core/rtw_xmit.c:946 xmitframe_addmic() warn: 
inconsistent indenting
   CHECK   drivers/staging/rtl8723bs/hal/hal_intf.c
   CHECK   drivers/staging/rtl8723bs/hal/hal_com.c
drivers/staging/rtl8723bs/hal/hal_com.c:1734 rtw_bb_rf_gain_offset() warn: 
inconsistent indenting
   CHECK   drivers/staging/rtl8723bs/hal/hal_com_phycfg.c
drivers/staging/rtl8723bs/hal/hal_com_phycfg.c:2090 
Hal_ChannelPlanToRegulation() warn: inconsistent indenting
   CHECK   drivers/staging/rtl8723bs/hal/hal_btcoex.c
   CHECK   drivers/staging/rtl8723bs/hal/hal_sdio.c
   CHECK   drivers/staging/rtl8723bs/hal/Hal8723BPwrSeq.c
   CHECK   drivers/staging/rtl8723bs/hal/HalPhyRf.c
   CHECK   drivers/staging/rtl8723bs/hal/HalPwrSeqCmd.c
   CHECK   drivers/staging/rtl8723bs/hal/odm.c
   CHECK   drivers/staging/rtl8723bs/hal/odm_CfoTracking.c
   CHECK   drivers/staging/rtl8723bs/hal/odm_debug.c
   CHECK   drivers/staging/rtl8723bs/hal/odm_DIG.c
   CHECK   drivers/staging/rtl8723bs/hal/odm_DynamicBBPowerSaving.c
   CHECK   drivers/staging/rtl8723bs/hal/odm_DynamicTxPower.c
   CHECK   drivers/staging/rtl8723bs/hal/odm_EdcaTurboCheck.c
   CHECK   drivers/staging/rtl8723bs/hal/odm_HWConfig.c
   CHECK   drivers/staging/rtl8723bs/hal/odm_NoiseMonitor.c
   CHECK   drivers/staging/rtl8723bs/hal/odm_PathDiv.c
   CHECK   drivers/staging/rtl8723bs/hal/odm_RegConfig8723B.c
   CHECK   drivers/staging/rtl8723bs/hal/odm_RTL8723B.c
   CHECK   drivers/staging/rtl8723bs/hal/rtl8723b_cmd.c
   CHECK   drivers/staging/rtl8723bs/hal/rtl8723b_dm.c
   CHECK   drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c:518 
rtl8723b_FirmwareDownload() error: we previously assumed 'pFirmware' could be 
null (see line 382)
   CHECK   drivers/staging/rtl8723bs/hal/rtl8723b_phycfg.c
   CHECK   drivers/staging/rtl8723bs/hal/rtl8723b_rf6052.c
   CHECK   drivers/staging/rtl8723bs/hal/rtl8723b_rxdesc.c
   CHECK   drivers/staging/rtl8723bs/hal/rtl8723bs_recv.c
   CHECK   drivers/staging/rtl8723bs/hal/rtl8723bs_xmit.c
   CHECK   drivers/staging/rtl8723bs/hal/sdio_halinit.c
   CHECK   drivers/staging/rtl8723bs/hal/sdio_ops.c
   CHECK   drivers/staging/rtl8723bs/hal/HalBtc8723b1Ant.c
   CHECK   drivers/staging/rtl8723bs/hal/HalBtc8723b2Ant.c
   CHECK   drivers/staging/rtl8723bs/hal/HalHWImg8723B_BB.c
drivers/staging/rtl8723bs/hal/HalHWImg8723B_BB.c:314 
ODM_ReadAndConfig_MP_8723B_AGC_TAB() warn: for statement not indented
drivers/staging/rtl8723bs/hal/HalHWImg8723B_BB.c:583 
ODM_ReadAndConfig_MP_8723B_PHY_REG() warn: for statement not indented
drivers/staging/rtl8723bs/hal/HalHWImg8723B_BB.c:586 
ODM_ReadAndConfig_MP_8723B_PHY_REG() warn: inconsistent indenting
   CHECK   drivers/staging/rtl8723bs/hal/HalHWImg8723B_MAC.c
   CHECK   drivers/staging/rtl8723bs/hal/HalHWImg8723B_RF.c
   CHECK   drivers/staging/rtl8723bs/hal/HalPhyRf_8723B.c
   CHECK   drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c:470 
rtw_cfg80211_ibss_indicate_connect() error: we previously assumed 'scanned' 
could be null (see line 466)
drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c:942 
rtw_cfg80211_set_encryption() warn: inconsistent indenting
drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c:955 
rtw_cfg80211_set_encryption() error: buffer overflow 
'psecuritypriv->dot11DefKey' 4 <= 4
drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c:1017 
rtw_cfg80211_set_encryption() error: buffer overflow 
'padapter->securitypriv.dot118021XGrpKey' 5 <= 5
drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c:1216 
cfg80211_rtw_set_default_key() warn: inconsistent indenting
drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c:2498 
rtw_cfg80211_monitor_if_xmit_entry() error: we previously assumed 'skb' could be 
null (see line 2495)
drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c:2850 cfg80211_rtw_start_ap() 
warn: if statement not indented
drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c:2860 cfg80211_rtw_start_ap() 
warn: if statement not indented
drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c:3417 
rtw_cfg80211_preinit_wiphy() warn: inconsistent indenting
drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c:3547 rtw_wdev_alloc() info: 
ignoring unreachable code.
   CHECK   drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
   CHECK   drivers/staging/rtl8723bs/os_dep/mlme_linux.c
drivers/staging/rtl8723bs/os_dep/mlme_linux.c:149 rtw_os_indicate_disconnect() 
warn: inconsistent indenting
   CHECK   drivers/staging/rtl8723bs/os_dep/osdep_service.c
   CHECK   drivers/staging/rtl8723bs/os_dep/os_intfs.c
drivers/staging/rtl8723bs/os_dep/os_intfs.c:1082 ips_netdrv_open() warn: 
inconsistent indenting
   CHECK   drivers/staging/rtl8723bs/os_dep/recv_linux.c
drivers/staging/rtl8723bs/os_dep/recv_linux.c:353 rtw_recv_indicatepkt() warn: 
variable dereferenced before check 'precv_frame' (see line 312)
   CHECK   drivers/staging/rtl8723bs/os_dep/rtw_proc.c
drivers/staging/rtl8723bs/os_dep/rtw_proc.c:102 rtw_drv_proc_open() warn: 
inconsistent indenting
   CHECK   drivers/staging/rtl8723bs/os_dep/sdio_intf.c
   CHECK   drivers/staging/rtl8723bs/os_dep/sdio_ops_linux.c
   CHECK   drivers/staging/rtl8723bs/os_dep/wifi_regd.c
   CHECK   drivers/staging/rtl8723bs/os_dep/xmit_linux.c
drivers/staging/rtl8723bs/os_dep/xmit_linux.c:42 _rtw_pktfile_read() warn: 
inconsistent indenting

The indent problems can probably be ignored, but many of the others are serious 
enough to be addressed now.

Once this driver is merged into staging, the Outreachy folks will be really busy.

Thanks for posting this driver,

Larry

Re: [PATCH] staging: Add rtl8723bs sdio wifi driver

[Greg Kroah-Hartman]

On Wed, Mar 29, 2017 at 08:20:19PM -0500, Larry Finger wrote:
> On 03/29/2017 12:47 PM, Hans de Goede wrote:
> > The rtl8723bs is found on quite a few systems used by Linux users,
> > such as on Atom systems (Intel Computestick and various other
> > Atom based devices) and on many (budget) ARM boards such as
> > the CHIP.
> > 
> > The plan moving forward with this is for the new clean,
> > written from scratch, rtl8xxxu driver to eventually gain
> > support for sdio devices. But there is no clear timeline
> > for that, so lets add this driver included in staging for now.
> > 
> > Cc: Bastien Nocera <hadess@hadess.net>
> > Cc: Larry Finger <Larry.Finger@lwfinger.net>
> > Cc: Jes Sorensen <jes.sorensen@gmail.com>
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Hans,
> 
> I am still building the driver on my CherryTrail tablet, but I did see some
> warnings and errors listed by Smatch as follows:
> 
> finger@linux-4v1g:~/wireless-drivers-next> make CHECK=~/smatch/smatch C=2
> drivers/staging/rtl8723bs/
>   CHK     include/config/kernel.release
>   CHK     include/generated/uapi/linux/version.h
>   CHK     include/generated/utsrelease.h
>   CHECK   arch/x86/purgatory/purgatory.c
>   CHECK   arch/x86/purgatory/sha256.c
>   CHECK   arch/x86/purgatory/string.c
>   CHK     include/generated/bounds.h
>   CHK     include/generated/timeconst.h
>   CHK     include/generated/asm-offsets.h
>   CALL    scripts/checksyscalls.sh
>   CHECK   scripts/mod/empty.c
>   CHECK   drivers/staging/rtl8723bs/core/rtw_ap.c
> drivers/staging/rtl8723bs/core/rtw_ap.c:382 expire_timeout_chk() warn:
> inconsistent indenting
>   CHECK   drivers/staging/rtl8723bs/core/rtw_btcoex.c
>   CHECK   drivers/staging/rtl8723bs/core/rtw_cmd.c
>   CHECK   drivers/staging/rtl8723bs/core/rtw_debug.c
> drivers/staging/rtl8723bs/core/rtw_debug.c:454 proc_get_survey_info() error:
> we previously assumed 'phead' could be null (see line 453)
> drivers/staging/rtl8723bs/core/rtw_debug.c:455 proc_get_survey_info() warn:
> variable dereferenced before check 'phead' (see line 454)
>   CHECK   drivers/staging/rtl8723bs/core/rtw_efuse.c
>   CHECK   drivers/staging/rtl8723bs/core/rtw_io.c
>   CHECK   drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
>   CHECK   drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> drivers/staging/rtl8723bs/core/rtw_ieee80211.c:83 rtw_is_cckrates_included()
> warn: if statement not indented
> drivers/staging/rtl8723bs/core/rtw_ieee80211.c:98
> rtw_is_cckratesonly_included() warn: if statement not indented
>   CHECK   drivers/staging/rtl8723bs/core/rtw_mlme.c
> drivers/staging/rtl8723bs/core/rtw_mlme.c:862 rtw_survey_event_callback()
> warn: inconsistent indenting
> drivers/staging/rtl8723bs/core/rtw_mlme.c:1102 rtw_free_assoc_resources()
> warn: if statement not indented
> drivers/staging/rtl8723bs/core/rtw_mlme.c:1165 rtw_indicate_disconnect()
> warn: inconsistent indenting
> drivers/staging/rtl8723bs/core/rtw_mlme.c:2830 rtw_restructure_ht_ie() warn:
> inconsistent indenting
> drivers/staging/rtl8723bs/core/rtw_mlme.c:2991 rtw_update_ht_cap() warn:
> inconsistent indenting
>   CHECK   drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:525 _mgt_dispatcher() warn:
> inconsistent indenting
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:1595 OnAssocReq() error:
> buffer overflow 'pstapriv->sta_aid' 32 <= 32
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:2391 dump_mgntframe_and_wait()
> warn: inconsistent indenting
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:2420
> dump_mgntframe_and_wait_ack() warn: inconsistent indenting
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:4969 process_80211d() error:
> testing array offset 'i' after use.
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:5738 linked_status_chk() warn:
> inconsistent indenting
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:6459 sitesurvey_cmd_hdl()
> warn: inconsistent indenting
>   CHECK   drivers/staging/rtl8723bs/core/rtw_odm.c
> drivers/staging/rtl8723bs/core/rtw_odm.c:109 rtw_odm_dbg_comp_msg() warn: if
> statement not indented
> drivers/staging/rtl8723bs/core/rtw_odm.c:146 rtw_odm_ability_msg() warn: if
> statement not indented
>   CHECK   drivers/staging/rtl8723bs/core/rtw_pwrctrl.c
> drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:641
> LeaveAllPowerSaveModeDirect() warn: inconsistent indenting
>   CHECK   drivers/staging/rtl8723bs/core/rtw_recv.c
> drivers/staging/rtl8723bs/core/rtw_recv.c:598 portctrl() warn: inconsistent
> indenting
> drivers/staging/rtl8723bs/core/rtw_recv.c:838 sta2sta_data_frame() warn:
> inconsistent indenting
> drivers/staging/rtl8723bs/core/rtw_recv.c:1547 validate_recv_frame() warn:
> inconsistent indenting
>   CHECK   drivers/staging/rtl8723bs/core/rtw_rf.c
>   CHECK   drivers/staging/rtl8723bs/core/rtw_security.c
> drivers/staging/rtl8723bs/core/rtw_security.c:266 rtw_wep_encrypt() warn:
> inconsistent indenting
> drivers/staging/rtl8723bs/core/rtw_security.c:433 rtw_seccalctkipmic() warn:
> inconsistent indenting
> drivers/staging/rtl8723bs/core/rtw_security.c:749 rtw_tkip_encrypt() warn:
> inconsistent indenting
> drivers/staging/rtl8723bs/core/rtw_security.c:865 rtw_tkip_decrypt() warn:
> inconsistent indenting
> drivers/staging/rtl8723bs/core/rtw_security.c:1383 aes_cipher() warn:
> inconsistent indenting
> drivers/staging/rtl8723bs/core/rtw_security.c:1415 aes_cipher() warn:
> inconsistent indenting
> drivers/staging/rtl8723bs/core/rtw_security.c:1430 aes_cipher() warn:
> inconsistent indenting
> drivers/staging/rtl8723bs/core/rtw_security.c:1582 rtw_aes_encrypt() warn:
> inconsistent indenting
> drivers/staging/rtl8723bs/core/rtw_security.c:1651 aes_decipher() warn:
> inconsistent indenting
> drivers/staging/rtl8723bs/core/rtw_security.c:1739 aes_decipher() warn:
> inconsistent indenting
> drivers/staging/rtl8723bs/core/rtw_security.c:1792 aes_decipher() warn:
> curly braces intended?
> drivers/staging/rtl8723bs/core/rtw_security.c:1809 aes_decipher() warn:
> inconsistent indenting
>   CHECK   drivers/staging/rtl8723bs/core/rtw_sta_mgt.c
> drivers/staging/rtl8723bs/core/rtw_sta_mgt.c:25 _rtw_init_stainfo() warn:
> inconsistent indenting
>   CHECK   drivers/staging/rtl8723bs/core/rtw_wlan_util.c
> drivers/staging/rtl8723bs/core/rtw_wlan_util.c:67 cckrates_included() warn:
> if statement not indented
> drivers/staging/rtl8723bs/core/rtw_wlan_util.c:81 cckratesonly_included()
> warn: if statement not indented
> drivers/staging/rtl8723bs/core/rtw_wlan_util.c:815 rtw_camid_alloc() warn:
> should '1 << (cam_id)' be a 64 bit type?
>   CHECK   drivers/staging/rtl8723bs/core/rtw_xmit.c
> drivers/staging/rtl8723bs/core/rtw_xmit.c:277 _rtw_init_xmit_priv() warn:
> inconsistent indenting
> drivers/staging/rtl8723bs/core/rtw_xmit.c:294 _rtw_free_xmit_priv() warn:
> inconsistent indenting
> drivers/staging/rtl8723bs/core/rtw_xmit.c:295 _rtw_free_xmit_priv() warn:
> inconsistent indenting
> drivers/staging/rtl8723bs/core/rtw_xmit.c:946 xmitframe_addmic() warn:
> inconsistent indenting
>   CHECK   drivers/staging/rtl8723bs/hal/hal_intf.c
>   CHECK   drivers/staging/rtl8723bs/hal/hal_com.c
> drivers/staging/rtl8723bs/hal/hal_com.c:1734 rtw_bb_rf_gain_offset() warn:
> inconsistent indenting
>   CHECK   drivers/staging/rtl8723bs/hal/hal_com_phycfg.c
> drivers/staging/rtl8723bs/hal/hal_com_phycfg.c:2090
> Hal_ChannelPlanToRegulation() warn: inconsistent indenting
>   CHECK   drivers/staging/rtl8723bs/hal/hal_btcoex.c
>   CHECK   drivers/staging/rtl8723bs/hal/hal_sdio.c
>   CHECK   drivers/staging/rtl8723bs/hal/Hal8723BPwrSeq.c
>   CHECK   drivers/staging/rtl8723bs/hal/HalPhyRf.c
>   CHECK   drivers/staging/rtl8723bs/hal/HalPwrSeqCmd.c
>   CHECK   drivers/staging/rtl8723bs/hal/odm.c
>   CHECK   drivers/staging/rtl8723bs/hal/odm_CfoTracking.c
>   CHECK   drivers/staging/rtl8723bs/hal/odm_debug.c
>   CHECK   drivers/staging/rtl8723bs/hal/odm_DIG.c
>   CHECK   drivers/staging/rtl8723bs/hal/odm_DynamicBBPowerSaving.c
>   CHECK   drivers/staging/rtl8723bs/hal/odm_DynamicTxPower.c
>   CHECK   drivers/staging/rtl8723bs/hal/odm_EdcaTurboCheck.c
>   CHECK   drivers/staging/rtl8723bs/hal/odm_HWConfig.c
>   CHECK   drivers/staging/rtl8723bs/hal/odm_NoiseMonitor.c
>   CHECK   drivers/staging/rtl8723bs/hal/odm_PathDiv.c
>   CHECK   drivers/staging/rtl8723bs/hal/odm_RegConfig8723B.c
>   CHECK   drivers/staging/rtl8723bs/hal/odm_RTL8723B.c
>   CHECK   drivers/staging/rtl8723bs/hal/rtl8723b_cmd.c
>   CHECK   drivers/staging/rtl8723bs/hal/rtl8723b_dm.c
>   CHECK   drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c:518
> rtl8723b_FirmwareDownload() error: we previously assumed 'pFirmware' could
> be null (see line 382)
>   CHECK   drivers/staging/rtl8723bs/hal/rtl8723b_phycfg.c
>   CHECK   drivers/staging/rtl8723bs/hal/rtl8723b_rf6052.c
>   CHECK   drivers/staging/rtl8723bs/hal/rtl8723b_rxdesc.c
>   CHECK   drivers/staging/rtl8723bs/hal/rtl8723bs_recv.c
>   CHECK   drivers/staging/rtl8723bs/hal/rtl8723bs_xmit.c
>   CHECK   drivers/staging/rtl8723bs/hal/sdio_halinit.c
>   CHECK   drivers/staging/rtl8723bs/hal/sdio_ops.c
>   CHECK   drivers/staging/rtl8723bs/hal/HalBtc8723b1Ant.c
>   CHECK   drivers/staging/rtl8723bs/hal/HalBtc8723b2Ant.c
>   CHECK   drivers/staging/rtl8723bs/hal/HalHWImg8723B_BB.c
> drivers/staging/rtl8723bs/hal/HalHWImg8723B_BB.c:314
> ODM_ReadAndConfig_MP_8723B_AGC_TAB() warn: for statement not indented
> drivers/staging/rtl8723bs/hal/HalHWImg8723B_BB.c:583
> ODM_ReadAndConfig_MP_8723B_PHY_REG() warn: for statement not indented
> drivers/staging/rtl8723bs/hal/HalHWImg8723B_BB.c:586
> ODM_ReadAndConfig_MP_8723B_PHY_REG() warn: inconsistent indenting
>   CHECK   drivers/staging/rtl8723bs/hal/HalHWImg8723B_MAC.c
>   CHECK   drivers/staging/rtl8723bs/hal/HalHWImg8723B_RF.c
>   CHECK   drivers/staging/rtl8723bs/hal/HalPhyRf_8723B.c
>   CHECK   drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c:470
> rtw_cfg80211_ibss_indicate_connect() error: we previously assumed 'scanned'
> could be null (see line 466)
> drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c:942
> rtw_cfg80211_set_encryption() warn: inconsistent indenting
> drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c:955
> rtw_cfg80211_set_encryption() error: buffer overflow
> 'psecuritypriv->dot11DefKey' 4 <= 4
> drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c:1017
> rtw_cfg80211_set_encryption() error: buffer overflow
> 'padapter->securitypriv.dot118021XGrpKey' 5 <= 5
> drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c:1216
> cfg80211_rtw_set_default_key() warn: inconsistent indenting
> drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c:2498
> rtw_cfg80211_monitor_if_xmit_entry() error: we previously assumed 'skb'
> could be null (see line 2495)
> drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c:2850
> cfg80211_rtw_start_ap() warn: if statement not indented
> drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c:2860
> cfg80211_rtw_start_ap() warn: if statement not indented
> drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c:3417
> rtw_cfg80211_preinit_wiphy() warn: inconsistent indenting
> drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c:3547 rtw_wdev_alloc()
> info: ignoring unreachable code.
>   CHECK   drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
>   CHECK   drivers/staging/rtl8723bs/os_dep/mlme_linux.c
> drivers/staging/rtl8723bs/os_dep/mlme_linux.c:149
> rtw_os_indicate_disconnect() warn: inconsistent indenting
>   CHECK   drivers/staging/rtl8723bs/os_dep/osdep_service.c
>   CHECK   drivers/staging/rtl8723bs/os_dep/os_intfs.c
> drivers/staging/rtl8723bs/os_dep/os_intfs.c:1082 ips_netdrv_open() warn:
> inconsistent indenting
>   CHECK   drivers/staging/rtl8723bs/os_dep/recv_linux.c
> drivers/staging/rtl8723bs/os_dep/recv_linux.c:353 rtw_recv_indicatepkt()
> warn: variable dereferenced before check 'precv_frame' (see line 312)
>   CHECK   drivers/staging/rtl8723bs/os_dep/rtw_proc.c
> drivers/staging/rtl8723bs/os_dep/rtw_proc.c:102 rtw_drv_proc_open() warn:
> inconsistent indenting
>   CHECK   drivers/staging/rtl8723bs/os_dep/sdio_intf.c
>   CHECK   drivers/staging/rtl8723bs/os_dep/sdio_ops_linux.c
>   CHECK   drivers/staging/rtl8723bs/os_dep/wifi_regd.c
>   CHECK   drivers/staging/rtl8723bs/os_dep/xmit_linux.c
> drivers/staging/rtl8723bs/os_dep/xmit_linux.c:42 _rtw_pktfile_read() warn:
> inconsistent indenting
> 
> The indent problems can probably be ignored, but many of the others are
> serious enough to be addressed now.
> 
> Once this driver is merged into staging, the Outreachy folks will be really busy.
> 
> Thanks for posting this driver,

So, no objection from you?  Great, I'll go queue it up later today...

thanks,

greg k-h

Re: [PATCH] staging: Add rtl8723bs sdio wifi driver

[Jes Sorensen]

On 03/30/2017 03:15 AM, Greg Kroah-Hartman wrote:
> On Wed, Mar 29, 2017 at 08:20:19PM -0500, Larry Finger wrote:
>> On 03/29/2017 12:47 PM, Hans de Goede wrote:
>>> The rtl8723bs is found on quite a few systems used by Linux users,
>>> such as on Atom systems (Intel Computestick and various other
>>> Atom based devices) and on many (budget) ARM boards such as
>>> the CHIP.
>>>
>>> The plan moving forward with this is for the new clean,
>>> written from scratch, rtl8xxxu driver to eventually gain
>>> support for sdio devices. But there is no clear timeline
>>> for that, so lets add this driver included in staging for now.
>>>
>>> Cc: Bastien Nocera <hadess@hadess.net>
>>> Cc: Larry Finger <Larry.Finger@lwfinger.net>
>>> Cc: Jes Sorensen <jes.sorensen@gmail.com>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>
>> Hans,
>>
>> I am still building the driver on my CherryTrail tablet, but I did see some
>> warnings and errors listed by Smatch as follows:
>>
[snip]
>> The indent problems can probably be ignored, but many of the others are
>> serious enough to be addressed now.
>>
>> Once this driver is merged into staging, the Outreachy folks will be really busy.
>>
>> Thanks for posting this driver,
>
> So, no objection from you?  Great, I'll go queue it up later today...
>
> thanks,
>
> greg k-h
>

As much as I hate adding more Realtek vendor code to the kernel, I do 
agree this is the right thing to do in the interim. Not sure when I'll 
get to SDIO support in rtl8xxxu, but I will process patches, hint hint :)

Jes

Re: [PATCH] staging: Add rtl8723bs sdio wifi driver

[poma]

On 30.03.2017 16:06, Jes Sorensen wrote:
> On 03/30/2017 03:15 AM, Greg Kroah-Hartman wrote:
>> On Wed, Mar 29, 2017 at 08:20:19PM -0500, Larry Finger wrote:
>>> On 03/29/2017 12:47 PM, Hans de Goede wrote:
>>>> The rtl8723bs is found on quite a few systems used by Linux users,
>>>> such as on Atom systems (Intel Computestick and various other
>>>> Atom based devices) and on many (budget) ARM boards such as
>>>> the CHIP.
>>>>
>>>> The plan moving forward with this is for the new clean,
>>>> written from scratch, rtl8xxxu driver to eventually gain
>>>> support for sdio devices. But there is no clear timeline
>>>> for that, so lets add this driver included in staging for now.
>>>>
>>>> Cc: Bastien Nocera <hadess@hadess.net>
>>>> Cc: Larry Finger <Larry.Finger@lwfinger.net>
>>>> Cc: Jes Sorensen <jes.sorensen@gmail.com>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>
>>> Hans,
>>>
>>> I am still building the driver on my CherryTrail tablet, but I did see some
>>> warnings and errors listed by Smatch as follows:
>>>
> [snip]
>>> The indent problems can probably be ignored, but many of the others are
>>> serious enough to be addressed now.
>>>
>>> Once this driver is merged into staging, the Outreachy folks will be really busy.
>>>
>>> Thanks for posting this driver,
>>
>> So, no objection from you?  Great, I'll go queue it up later today...
>>
>> thanks,
>>
>> greg k-h
>>
> 
> As much as I hate adding more Realtek vendor code to the kernel, I do 
> agree this is the right thing to do in the interim. Not sure when I'll 
> get to SDIO support in rtl8xxxu, but I will process patches, hint hint :)
> 
> Jes
> 

A day after tomorrow? :)

Re: [PATCH] staging: Add rtl8723bs sdio wifi driver

[Larry Finger]

On 03/29/2017 12:47 PM, Hans de Goede wrote:
> The rtl8723bs is found on quite a few systems used by Linux users,
> such as on Atom systems (Intel Computestick and various other
> Atom based devices) and on many (budget) ARM boards such as
> the CHIP.
>
> The plan moving forward with this is for the new clean,
> written from scratch, rtl8xxxu driver to eventually gain
> support for sdio devices. But there is no clear timeline
> for that, so lets add this driver included in staging for now.

Hans,

I started looking at the Smatch errors. This one may be the result of a serious 
problem:

   CHECK   drivers/staging/rtl8723bs/core/rtw_debug.c
drivers/staging/rtl8723bs/core/rtw_debug.c:454 proc_get_survey_info() error: we 
previously assumed 'phead' could be null (see line 453)
drivers/staging/rtl8723bs/core/rtw_debug.c:455 proc_get_survey_info() warn: 
variable dereferenced before check 'phead' (see line 454)

A snippet of the code in question is as follows:

         spin_lock_bh(&(pmlmepriv->scanned_queue.lock));
         phead = get_list_head(queue);
         plist = phead ? get_next(phead) : NULL;
         plist = get_next(phead);
         if ((!phead) || (!plist)) {
                 spin_unlock_bh(&(pmlmepriv->scanned_queue.lock));
                 return 0;
         }

This code comes directly from the hadess repo, but I am suspicious of the double 
call to get_next(phead). I cannot imagine any valid reason to skip every other 
entry on that list.

Larry

Re: [PATCH] staging: Add rtl8723bs sdio wifi driver

[Hans de Goede]

Hi,

On 04/04/2017 08:31 PM, Larry Finger wrote:
> On 03/29/2017 12:47 PM, Hans de Goede wrote:
>> The rtl8723bs is found on quite a few systems used by Linux users,
>> such as on Atom systems (Intel Computestick and various other
>> Atom based devices) and on many (budget) ARM boards such as
>> the CHIP.
>>
>> The plan moving forward with this is for the new clean,
>> written from scratch, rtl8xxxu driver to eventually gain
>> support for sdio devices. But there is no clear timeline
>> for that, so lets add this driver included in staging for now.
>
> Hans,
>
> I started looking at the Smatch errors. This one may be the result of a serious problem:
>
>   CHECK   drivers/staging/rtl8723bs/core/rtw_debug.c
> drivers/staging/rtl8723bs/core/rtw_debug.c:454 proc_get_survey_info() error: we previously assumed 'phead' could be null (see line 453)
> drivers/staging/rtl8723bs/core/rtw_debug.c:455 proc_get_survey_info() warn: variable dereferenced before check 'phead' (see line 454)
>
> A snippet of the code in question is as follows:
>
>         spin_lock_bh(&(pmlmepriv->scanned_queue.lock));
>         phead = get_list_head(queue);
>         plist = phead ? get_next(phead) : NULL;
>         plist = get_next(phead);
>         if ((!phead) || (!plist)) {
>                 spin_unlock_bh(&(pmlmepriv->scanned_queue.lock));
>                 return 0;
>         }
>
> This code comes directly from the hadess repo, but I am suspicious of the double call to get_next(phead). I cannot imagine any valid reason to skip every other entry on that list.

If you look closer and simplify the first getnext line what is written is:

          plist = get_next(phead);
          plist = get_next(phead);

Which indeed looks like it could use improvement, but I don't
think it is seriously broken.

Regards,

Hans

Re: [PATCH] staging: Add rtl8723bs sdio wifi driver

[Larry Finger]

On 04/04/2017 01:53 PM, Hans de Goede wrote:
> Hi,
>
> On 04/04/2017 08:31 PM, Larry Finger wrote:
>> On 03/29/2017 12:47 PM, Hans de Goede wrote:
>>> The rtl8723bs is found on quite a few systems used by Linux users,
>>> such as on Atom systems (Intel Computestick and various other
>>> Atom based devices) and on many (budget) ARM boards such as
>>> the CHIP.
>>>
>>> The plan moving forward with this is for the new clean,
>>> written from scratch, rtl8xxxu driver to eventually gain
>>> support for sdio devices. But there is no clear timeline
>>> for that, so lets add this driver included in staging for now.
>>
>> Hans,
>>
>> I started looking at the Smatch errors. This one may be the result of a
>> serious problem:
>>
>>   CHECK   drivers/staging/rtl8723bs/core/rtw_debug.c
>> drivers/staging/rtl8723bs/core/rtw_debug.c:454 proc_get_survey_info() error:
>> we previously assumed 'phead' could be null (see line 453)
>> drivers/staging/rtl8723bs/core/rtw_debug.c:455 proc_get_survey_info() warn:
>> variable dereferenced before check 'phead' (see line 454)
>>
>> A snippet of the code in question is as follows:
>>
>>         spin_lock_bh(&(pmlmepriv->scanned_queue.lock));
>>         phead = get_list_head(queue);
>>         plist = phead ? get_next(phead) : NULL;
>>         plist = get_next(phead);
>>         if ((!phead) || (!plist)) {
>>                 spin_unlock_bh(&(pmlmepriv->scanned_queue.lock));
>>                 return 0;
>>         }
>>
>> This code comes directly from the hadess repo, but I am suspicious of the
>> double call to get_next(phead). I cannot imagine any valid reason to skip
>> every other entry on that list.
>
> If you look closer and simplify the first getnext line what is written is:
>
>          plist = get_next(phead);
>          plist = get_next(phead);
>
> Which indeed looks like it could use improvement, but I don't
> think it is seriously broken.

My problem was that I thought that get_next() took an entry off the list. Now 
that I actually looked, I see that it just gets the next pointer.

Sorry for the noise,

Larry

Re: [PATCH] staging: Add rtl8723bs sdio wifi driver

[Arend Van Spriel]

On 4-4-2017 20:53, Hans de Goede wrote:
> Hi,
> 
> On 04/04/2017 08:31 PM, Larry Finger wrote:
>> On 03/29/2017 12:47 PM, Hans de Goede wrote:
>>> The rtl8723bs is found on quite a few systems used by Linux users,
>>> such as on Atom systems (Intel Computestick and various other
>>> Atom based devices) and on many (budget) ARM boards such as
>>> the CHIP.
>>>
>>> The plan moving forward with this is for the new clean,
>>> written from scratch, rtl8xxxu driver to eventually gain
>>> support for sdio devices. But there is no clear timeline
>>> for that, so lets add this driver included in staging for now.
>>
>> Hans,
>>
>> I started looking at the Smatch errors. This one may be the result of
>> a serious problem:
>>
>>   CHECK   drivers/staging/rtl8723bs/core/rtw_debug.c
>> drivers/staging/rtl8723bs/core/rtw_debug.c:454 proc_get_survey_info()
>> error: we previously assumed 'phead' could be null (see line 453)
>> drivers/staging/rtl8723bs/core/rtw_debug.c:455 proc_get_survey_info()
>> warn: variable dereferenced before check 'phead' (see line 454)
>>
>> A snippet of the code in question is as follows:
>>
>>         spin_lock_bh(&(pmlmepriv->scanned_queue.lock));
>>         phead = get_list_head(queue);
>>         plist = phead ? get_next(phead) : NULL;
>>         plist = get_next(phead);
>>         if ((!phead) || (!plist)) {
>>                 spin_unlock_bh(&(pmlmepriv->scanned_queue.lock));
>>                 return 0;
>>         }
>>
>> This code comes directly from the hadess repo, but I am suspicious of
>> the double call to get_next(phead). I cannot imagine any valid reason
>> to skip every other entry on that list.
> 
> If you look closer and simplify the first getnext line what is written is:
> 
>          plist = get_next(phead);
>          plist = get_next(phead);
> 
> Which indeed looks like it could use improvement, but I don't
> think it is seriously broken.

So get_list_head(queue) can never return NULL? Otherwise I don't know
how close I need to get for a simplified look ;-)

Gr. AvS

> Regards,
> 
> Hans

Re: [PATCH] staging: Add rtl8723bs sdio wifi driver

[Hans de Goede]

Hi,

On 04/04/2017 11:38 PM, Arend Van Spriel wrote:
>
>
> On 4-4-2017 20:53, Hans de Goede wrote:
>> Hi,
>>
>> On 04/04/2017 08:31 PM, Larry Finger wrote:
>>> On 03/29/2017 12:47 PM, Hans de Goede wrote:
>>>> The rtl8723bs is found on quite a few systems used by Linux users,
>>>> such as on Atom systems (Intel Computestick and various other
>>>> Atom based devices) and on many (budget) ARM boards such as
>>>> the CHIP.
>>>>
>>>> The plan moving forward with this is for the new clean,
>>>> written from scratch, rtl8xxxu driver to eventually gain
>>>> support for sdio devices. But there is no clear timeline
>>>> for that, so lets add this driver included in staging for now.
>>>
>>> Hans,
>>>
>>> I started looking at the Smatch errors. This one may be the result of
>>> a serious problem:
>>>
>>>   CHECK   drivers/staging/rtl8723bs/core/rtw_debug.c
>>> drivers/staging/rtl8723bs/core/rtw_debug.c:454 proc_get_survey_info()
>>> error: we previously assumed 'phead' could be null (see line 453)
>>> drivers/staging/rtl8723bs/core/rtw_debug.c:455 proc_get_survey_info()
>>> warn: variable dereferenced before check 'phead' (see line 454)
>>>
>>> A snippet of the code in question is as follows:
>>>
>>>         spin_lock_bh(&(pmlmepriv->scanned_queue.lock));
>>>         phead = get_list_head(queue);
>>>         plist = phead ? get_next(phead) : NULL;
>>>         plist = get_next(phead);
>>>         if ((!phead) || (!plist)) {
>>>                 spin_unlock_bh(&(pmlmepriv->scanned_queue.lock));
>>>                 return 0;
>>>         }
>>>
>>> This code comes directly from the hadess repo, but I am suspicious of
>>> the double call to get_next(phead). I cannot imagine any valid reason
>>> to skip every other entry on that list.
>>
>> If you look closer and simplify the first getnext line what is written is:
>>
>>          plist = get_next(phead);
>>          plist = get_next(phead);
>>
>> Which indeed looks like it could use improvement, but I don't
>> think it is seriously broken.
>
> So get_list_head(queue) can never return NULL?

I don't know.

> Otherwise I don't know
> how close I need to get for a simplified look ;-)

I simplified things so as to make clear that Larry's worry was
not really a problem, I do agree the smatch complaint is valid.

Regards,

Hans

Re: [PATCH] staging: Add rtl8723bs sdio wifi driver

[Larry Finger]

On 04/04/2017 04:53 PM, Hans de Goede wrote:
> Hi,
>
> On 04/04/2017 11:38 PM, Arend Van Spriel wrote:
>>
>>
>> On 4-4-2017 20:53, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 04/04/2017 08:31 PM, Larry Finger wrote:
>>>> On 03/29/2017 12:47 PM, Hans de Goede wrote:
>>>>> The rtl8723bs is found on quite a few systems used by Linux users,
>>>>> such as on Atom systems (Intel Computestick and various other
>>>>> Atom based devices) and on many (budget) ARM boards such as
>>>>> the CHIP.
>>>>>
>>>>> The plan moving forward with this is for the new clean,
>>>>> written from scratch, rtl8xxxu driver to eventually gain
>>>>> support for sdio devices. But there is no clear timeline
>>>>> for that, so lets add this driver included in staging for now.
>>>>
>>>> Hans,
>>>>
>>>> I started looking at the Smatch errors. This one may be the result of
>>>> a serious problem:
>>>>
>>>>   CHECK   drivers/staging/rtl8723bs/core/rtw_debug.c
>>>> drivers/staging/rtl8723bs/core/rtw_debug.c:454 proc_get_survey_info()
>>>> error: we previously assumed 'phead' could be null (see line 453)
>>>> drivers/staging/rtl8723bs/core/rtw_debug.c:455 proc_get_survey_info()
>>>> warn: variable dereferenced before check 'phead' (see line 454)
>>>>
>>>> A snippet of the code in question is as follows:
>>>>
>>>>         spin_lock_bh(&(pmlmepriv->scanned_queue.lock));
>>>>         phead = get_list_head(queue);
>>>>         plist = phead ? get_next(phead) : NULL;
>>>>         plist = get_next(phead);
>>>>         if ((!phead) || (!plist)) {
>>>>                 spin_unlock_bh(&(pmlmepriv->scanned_queue.lock));
>>>>                 return 0;
>>>>         }
>>>>
>>>> This code comes directly from the hadess repo, but I am suspicious of
>>>> the double call to get_next(phead). I cannot imagine any valid reason
>>>> to skip every other entry on that list.
>>>
>>> If you look closer and simplify the first getnext line what is written is:
>>>
>>>          plist = get_next(phead);
>>>          plist = get_next(phead);
>>>
>>> Which indeed looks like it could use improvement, but I don't
>>> think it is seriously broken.
>>
>> So get_list_head(queue) can never return NULL?
>
> I don't know.
>
>> Otherwise I don't know
>> how close I need to get for a simplified look ;-)
>
> I simplified things so as to make clear that Larry's worry was
> not really a problem, I do agree the smatch complaint is valid.

I think the following fixes the problem:

diff --git a/drivers/staging/rtl8723bs/core/rtw_debug.c 
b/drivers/staging/rtl8723bs/core/rtw_debug.c
index d34747b29309..51cef55d3f76 100644
--- a/drivers/staging/rtl8723bs/core/rtw_debug.c
+++ b/drivers/staging/rtl8723bs/core/rtw_debug.c
@@ -451,7 +451,6 @@ int proc_get_survey_info(struct seq_file *m, void *v)
         spin_lock_bh(&(pmlmepriv->scanned_queue.lock));
         phead = get_list_head(queue);
         plist = phead ? get_next(phead) : NULL;
-       plist = get_next(phead);
         if ((!phead) || (!plist)) {
                 spin_unlock_bh(&(pmlmepriv->scanned_queue.lock));
                 return 0;

Pointer plist is set in the 3rd line, thus the second set of it can be removed.

Larry

Re: [PATCH] staging: Add rtl8723bs sdio wifi driver

[Hans de Goede]

Hi,

On 05-04-17 01:41, Larry Finger wrote:
> On 04/04/2017 04:53 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 04/04/2017 11:38 PM, Arend Van Spriel wrote:
>>>
>>>
>>> On 4-4-2017 20:53, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 04/04/2017 08:31 PM, Larry Finger wrote:
>>>>> On 03/29/2017 12:47 PM, Hans de Goede wrote:
>>>>>> The rtl8723bs is found on quite a few systems used by Linux users,
>>>>>> such as on Atom systems (Intel Computestick and various other
>>>>>> Atom based devices) and on many (budget) ARM boards such as
>>>>>> the CHIP.
>>>>>>
>>>>>> The plan moving forward with this is for the new clean,
>>>>>> written from scratch, rtl8xxxu driver to eventually gain
>>>>>> support for sdio devices. But there is no clear timeline
>>>>>> for that, so lets add this driver included in staging for now.
>>>>>
>>>>> Hans,
>>>>>
>>>>> I started looking at the Smatch errors. This one may be the result of
>>>>> a serious problem:
>>>>>
>>>>>   CHECK   drivers/staging/rtl8723bs/core/rtw_debug.c
>>>>> drivers/staging/rtl8723bs/core/rtw_debug.c:454 proc_get_survey_info()
>>>>> error: we previously assumed 'phead' could be null (see line 453)
>>>>> drivers/staging/rtl8723bs/core/rtw_debug.c:455 proc_get_survey_info()
>>>>> warn: variable dereferenced before check 'phead' (see line 454)
>>>>>
>>>>> A snippet of the code in question is as follows:
>>>>>
>>>>>         spin_lock_bh(&(pmlmepriv->scanned_queue.lock));
>>>>>         phead = get_list_head(queue);
>>>>>         plist = phead ? get_next(phead) : NULL;
>>>>>         plist = get_next(phead);
>>>>>         if ((!phead) || (!plist)) {
>>>>>                 spin_unlock_bh(&(pmlmepriv->scanned_queue.lock));
>>>>>                 return 0;
>>>>>         }
>>>>>
>>>>> This code comes directly from the hadess repo, but I am suspicious of
>>>>> the double call to get_next(phead). I cannot imagine any valid reason
>>>>> to skip every other entry on that list.
>>>>
>>>> If you look closer and simplify the first getnext line what is written is:
>>>>
>>>>          plist = get_next(phead);
>>>>          plist = get_next(phead);
>>>>
>>>> Which indeed looks like it could use improvement, but I don't
>>>> think it is seriously broken.
>>>
>>> So get_list_head(queue) can never return NULL?
>>
>> I don't know.
>>
>>> Otherwise I don't know
>>> how close I need to get for a simplified look ;-)
>>
>> I simplified things so as to make clear that Larry's worry was
>> not really a problem, I do agree the smatch complaint is valid.
>
> I think the following fixes the problem:
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_debug.c b/drivers/staging/rtl8723bs/core/rtw_debug.c
> index d34747b29309..51cef55d3f76 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_debug.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_debug.c
> @@ -451,7 +451,6 @@ int proc_get_survey_info(struct seq_file *m, void *v)
>         spin_lock_bh(&(pmlmepriv->scanned_queue.lock));
>         phead = get_list_head(queue);
>         plist = phead ? get_next(phead) : NULL;
> -       plist = get_next(phead);
>         if ((!phead) || (!plist)) {
>                 spin_unlock_bh(&(pmlmepriv->scanned_queue.lock));
>                 return 0;
>
> Pointer plist is set in the 3rd line, thus the second set of it can be removed.

Agreed, when I've some time I plan to do a follow-up patch
to my initial submission fixing all the Smatch errors. But feel
free to beat me to it.

Greg, if I understood you correctly you plan to merge my initial
submission as is and then we can fix this (and other things) with
follow up patches, right ?

Regards,

Hans

Re: [PATCH] staging: Add rtl8723bs sdio wifi driver

[Larry Finger]

On 04/05/2017 04:36 AM, Hans de Goede wrote:
> Hi,
>
> On 05-04-17 01:41, Larry Finger wrote:
>> On 04/04/2017 04:53 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 04/04/2017 11:38 PM, Arend Van Spriel wrote:
>>>>
>>>>
>>>> On 4-4-2017 20:53, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 04/04/2017 08:31 PM, Larry Finger wrote:
>>>>>> On 03/29/2017 12:47 PM, Hans de Goede wrote:
>>>>>>> The rtl8723bs is found on quite a few systems used by Linux users,
>>>>>>> such as on Atom systems (Intel Computestick and various other
>>>>>>> Atom based devices) and on many (budget) ARM boards such as
>>>>>>> the CHIP.
>>>>>>>
>>>>>>> The plan moving forward with this is for the new clean,
>>>>>>> written from scratch, rtl8xxxu driver to eventually gain
>>>>>>> support for sdio devices. But there is no clear timeline
>>>>>>> for that, so lets add this driver included in staging for now.
>>>>>>
>>>>>> Hans,
>>>>>>
>>>>>> I started looking at the Smatch errors. This one may be the result of
>>>>>> a serious problem:
>>>>>>
>>>>>>   CHECK   drivers/staging/rtl8723bs/core/rtw_debug.c
>>>>>> drivers/staging/rtl8723bs/core/rtw_debug.c:454 proc_get_survey_info()
>>>>>> error: we previously assumed 'phead' could be null (see line 453)
>>>>>> drivers/staging/rtl8723bs/core/rtw_debug.c:455 proc_get_survey_info()
>>>>>> warn: variable dereferenced before check 'phead' (see line 454)
>>>>>>
>>>>>> A snippet of the code in question is as follows:
>>>>>>
>>>>>>         spin_lock_bh(&(pmlmepriv->scanned_queue.lock));
>>>>>>         phead = get_list_head(queue);
>>>>>>         plist = phead ? get_next(phead) : NULL;
>>>>>>         plist = get_next(phead);
>>>>>>         if ((!phead) || (!plist)) {
>>>>>>                 spin_unlock_bh(&(pmlmepriv->scanned_queue.lock));
>>>>>>                 return 0;
>>>>>>         }
>>>>>>
>>>>>> This code comes directly from the hadess repo, but I am suspicious of
>>>>>> the double call to get_next(phead). I cannot imagine any valid reason
>>>>>> to skip every other entry on that list.
>>>>>
>>>>> If you look closer and simplify the first getnext line what is written is:
>>>>>
>>>>>          plist = get_next(phead);
>>>>>          plist = get_next(phead);
>>>>>
>>>>> Which indeed looks like it could use improvement, but I don't
>>>>> think it is seriously broken.
>>>>
>>>> So get_list_head(queue) can never return NULL?
>>>
>>> I don't know.
>>>
>>>> Otherwise I don't know
>>>> how close I need to get for a simplified look ;-)
>>>
>>> I simplified things so as to make clear that Larry's worry was
>>> not really a problem, I do agree the smatch complaint is valid.
>>
>> I think the following fixes the problem:
>>
>> diff --git a/drivers/staging/rtl8723bs/core/rtw_debug.c
>> b/drivers/staging/rtl8723bs/core/rtw_debug.c
>> index d34747b29309..51cef55d3f76 100644
>> --- a/drivers/staging/rtl8723bs/core/rtw_debug.c
>> +++ b/drivers/staging/rtl8723bs/core/rtw_debug.c
>> @@ -451,7 +451,6 @@ int proc_get_survey_info(struct seq_file *m, void *v)
>>         spin_lock_bh(&(pmlmepriv->scanned_queue.lock));
>>         phead = get_list_head(queue);
>>         plist = phead ? get_next(phead) : NULL;
>> -       plist = get_next(phead);
>>         if ((!phead) || (!plist)) {
>>                 spin_unlock_bh(&(pmlmepriv->scanned_queue.lock));
>>                 return 0;
>>
>> Pointer plist is set in the 3rd line, thus the second set of it can be removed.
>
> Agreed, when I've some time I plan to do a follow-up patch
> to my initial submission fixing all the Smatch errors. But feel
> free to beat me to it.
>
> Greg, if I understood you correctly you plan to merge my initial
> submission as is and then we can fix this (and other things) with
> follow up patches, right ?

Hans,

I took GregKH out of the Cc list when I started the discussion of the Smatch 
errors/warnings, and he probably has not seen the recent E-mails in this thread. 
It it was my understanding that he planned to apply your submission in the form 
you sent.

I have several patches nearly ready to fix most of the Smatch warnings and 
errors. I will send them as soon as the original submission is applied.

Larry

Re: [PATCH] staging: Add rtl8723bs sdio wifi driver

[Hans de Goede]

Hi,

On 05-04-17 18:32, Larry Finger wrote:
> On 04/05/2017 04:36 AM, Hans de Goede wrote:
>> Hi,
>>
>> On 05-04-17 01:41, Larry Finger wrote:
>>> On 04/04/2017 04:53 PM, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 04/04/2017 11:38 PM, Arend Van Spriel wrote:
>>>>>
>>>>>
>>>>> On 4-4-2017 20:53, Hans de Goede wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 04/04/2017 08:31 PM, Larry Finger wrote:
>>>>>>> On 03/29/2017 12:47 PM, Hans de Goede wrote:
>>>>>>>> The rtl8723bs is found on quite a few systems used by Linux users,
>>>>>>>> such as on Atom systems (Intel Computestick and various other
>>>>>>>> Atom based devices) and on many (budget) ARM boards such as
>>>>>>>> the CHIP.
>>>>>>>>
>>>>>>>> The plan moving forward with this is for the new clean,
>>>>>>>> written from scratch, rtl8xxxu driver to eventually gain
>>>>>>>> support for sdio devices. But there is no clear timeline
>>>>>>>> for that, so lets add this driver included in staging for now.
>>>>>>>
>>>>>>> Hans,
>>>>>>>
>>>>>>> I started looking at the Smatch errors. This one may be the result of
>>>>>>> a serious problem:
>>>>>>>
>>>>>>>   CHECK   drivers/staging/rtl8723bs/core/rtw_debug.c
>>>>>>> drivers/staging/rtl8723bs/core/rtw_debug.c:454 proc_get_survey_info()
>>>>>>> error: we previously assumed 'phead' could be null (see line 453)
>>>>>>> drivers/staging/rtl8723bs/core/rtw_debug.c:455 proc_get_survey_info()
>>>>>>> warn: variable dereferenced before check 'phead' (see line 454)
>>>>>>>
>>>>>>> A snippet of the code in question is as follows:
>>>>>>>
>>>>>>>         spin_lock_bh(&(pmlmepriv->scanned_queue.lock));
>>>>>>>         phead = get_list_head(queue);
>>>>>>>         plist = phead ? get_next(phead) : NULL;
>>>>>>>         plist = get_next(phead);
>>>>>>>         if ((!phead) || (!plist)) {
>>>>>>>                 spin_unlock_bh(&(pmlmepriv->scanned_queue.lock));
>>>>>>>                 return 0;
>>>>>>>         }
>>>>>>>
>>>>>>> This code comes directly from the hadess repo, but I am suspicious of
>>>>>>> the double call to get_next(phead). I cannot imagine any valid reason
>>>>>>> to skip every other entry on that list.
>>>>>>
>>>>>> If you look closer and simplify the first getnext line what is written is:
>>>>>>
>>>>>>          plist = get_next(phead);
>>>>>>          plist = get_next(phead);
>>>>>>
>>>>>> Which indeed looks like it could use improvement, but I don't
>>>>>> think it is seriously broken.
>>>>>
>>>>> So get_list_head(queue) can never return NULL?
>>>>
>>>> I don't know.
>>>>
>>>>> Otherwise I don't know
>>>>> how close I need to get for a simplified look ;-)
>>>>
>>>> I simplified things so as to make clear that Larry's worry was
>>>> not really a problem, I do agree the smatch complaint is valid.
>>>
>>> I think the following fixes the problem:
>>>
>>> diff --git a/drivers/staging/rtl8723bs/core/rtw_debug.c
>>> b/drivers/staging/rtl8723bs/core/rtw_debug.c
>>> index d34747b29309..51cef55d3f76 100644
>>> --- a/drivers/staging/rtl8723bs/core/rtw_debug.c
>>> +++ b/drivers/staging/rtl8723bs/core/rtw_debug.c
>>> @@ -451,7 +451,6 @@ int proc_get_survey_info(struct seq_file *m, void *v)
>>>         spin_lock_bh(&(pmlmepriv->scanned_queue.lock));
>>>         phead = get_list_head(queue);
>>>         plist = phead ? get_next(phead) : NULL;
>>> -       plist = get_next(phead);
>>>         if ((!phead) || (!plist)) {
>>>                 spin_unlock_bh(&(pmlmepriv->scanned_queue.lock));
>>>                 return 0;
>>>
>>> Pointer plist is set in the 3rd line, thus the second set of it can be removed.
>>
>> Agreed, when I've some time I plan to do a follow-up patch
>> to my initial submission fixing all the Smatch errors. But feel
>> free to beat me to it.
>>
>> Greg, if I understood you correctly you plan to merge my initial
>> submission as is and then we can fix this (and other things) with
>> follow up patches, right ?
>
> Hans,
>
> I took GregKH out of the Cc list when I started the discussion of the Smatch errors/warnings, and he probably has not seen the recent E-mails in this thread.

Ah I missed that (clearly I did).

> It it was my understanding that he planned to apply your submission in the form you sent.

That is my understanding too.

> I have several patches nearly ready to fix most of the Smatch warnings and errors. I will send them as soon as the original submission is applied.

Good thank you. So what is the plan with the github version ?

Note that my submission contains a few small fixes on top of
the github version, for which I intended to submit a pull-req
but I've not gotten around to that yet, I've done so now:

https://github.com/hadess/rtl8723bs/pull/125

But do we want to keep maintaining the github version (for a while
at least) I wonder, as that does mean double work?

Regards,

Hans

Re: [PATCH] staging: Add rtl8723bs sdio wifi driver

[Bastien Nocera]

On Thu, 2017-04-06 at 08:55 +0200, Hans de Goede wrote:
> 
<snip>
> Good thank you. So what is the plan with the github version ?
> 
> Note that my submission contains a few small fixes on top of
> the github version, for which I intended to submit a pull-req
> but I've not gotten around to that yet, I've done so now:
> 
> https://github.com/hadess/rtl8723bs/pull/125
> 
> But do we want to keep maintaining the github version (for a while
> at least) I wonder, as that does mean double work?

My plan is to remove everything and point to the upstream tree as soon
as the support has landed in Linus' tree.

While there is some use in keeping it around for older kernels, we keep
getting asked to support even older kernels than reasonable, or have
users complaining about non-working machines once they use the driver,
which simply uncovers kernel bugs that were fixed upstream.

I don't think it's a good use of our time to carry on supporting this
out-of-tree. I'll however make sure to try and document the migration
in a way that's helpful to users.

Cheers

Re: [PATCH] staging: Add rtl8723bs sdio wifi driver

[Hans de Goede]

Hi,

On 06-04-17 11:04, Bastien Nocera wrote:
> On Thu, 2017-04-06 at 08:55 +0200, Hans de Goede wrote:
>>
> <snip>
>> Good thank you. So what is the plan with the github version ?
>>
>> Note that my submission contains a few small fixes on top of
>> the github version, for which I intended to submit a pull-req
>> but I've not gotten around to that yet, I've done so now:
>>
>> https://github.com/hadess/rtl8723bs/pull/125
>>
>> But do we want to keep maintaining the github version (for a while
>> at least) I wonder, as that does mean double work?
>
> My plan is to remove everything and point to the upstream tree as soon
> as the support has landed in Linus' tree.
>
> While there is some use in keeping it around for older kernels, we keep
> getting asked to support even older kernels than reasonable, or have
> users complaining about non-working machines once they use the driver,
> which simply uncovers kernel bugs that were fixed upstream.
>
> I don't think it's a good use of our time to carry on supporting this
> out-of-tree. I'll however make sure to try and document the migration
> in a way that's helpful to users.

Ok, that works for me.

Regards,

Hans

Re: [PATCH] staging: Add rtl8723bs sdio wifi driver

[Larry Finger]

On 04/06/2017 04:49 AM, Hans de Goede wrote:
> Hi,
>
> On 06-04-17 11:04, Bastien Nocera wrote:
>> On Thu, 2017-04-06 at 08:55 +0200, Hans de Goede wrote:
>>>
>> <snip>
>>> Good thank you. So what is the plan with the github version ?
>>>
>>> Note that my submission contains a few small fixes on top of
>>> the github version, for which I intended to submit a pull-req
>>> but I've not gotten around to that yet, I've done so now:
>>>
>>> https://github.com/hadess/rtl8723bs/pull/125
>>>
>>> But do we want to keep maintaining the github version (for a while
>>> at least) I wonder, as that does mean double work?
>>
>> My plan is to remove everything and point to the upstream tree as soon
>> as the support has landed in Linus' tree.
>>
>> While there is some use in keeping it around for older kernels, we keep
>> getting asked to support even older kernels than reasonable, or have
>> users complaining about non-working machines once they use the driver,
>> which simply uncovers kernel bugs that were fixed upstream.
>>
>> I don't think it's a good use of our time to carry on supporting this
>> out-of-tree. I'll however make sure to try and document the migration
>> in a way that's helpful to users.
>
> Ok, that works for me.

I agree. I have enough to do.

Larry

Re: [PATCH] staging: Add rtl8723bs sdio wifi driver

[Jes Sorensen]

On 04/06/2017 02:32 PM, Larry Finger wrote:
> On 04/06/2017 04:49 AM, Hans de Goede wrote:
>> Hi,
>>
>> On 06-04-17 11:04, Bastien Nocera wrote:
>>> On Thu, 2017-04-06 at 08:55 +0200, Hans de Goede wrote:
>>>>
>>> <snip>
>>>> Good thank you. So what is the plan with the github version ?
>>>>
>>>> Note that my submission contains a few small fixes on top of
>>>> the github version, for which I intended to submit a pull-req
>>>> but I've not gotten around to that yet, I've done so now:
>>>>
>>>> https://github.com/hadess/rtl8723bs/pull/125
>>>>
>>>> But do we want to keep maintaining the github version (for a while
>>>> at least) I wonder, as that does mean double work?
>>>
>>> My plan is to remove everything and point to the upstream tree as soon
>>> as the support has landed in Linus' tree.
>>>
>>> While there is some use in keeping it around for older kernels, we keep
>>> getting asked to support even older kernels than reasonable, or have
>>> users complaining about non-working machines once they use the driver,
>>> which simply uncovers kernel bugs that were fixed upstream.
>>>
>>> I don't think it's a good use of our time to carry on supporting this
>>> out-of-tree. I'll however make sure to try and document the migration
>>> in a way that's helpful to users.
>>
>> Ok, that works for me.
>
> I agree. I have enough to do.

You could simply break the build, and have it spit out a warning saying 
the git repo is kept there to track the old source.

It might be useful to have a repository of the original code to go look at.

Cheers,
Jes