[Outreachy kernel] [PATCH 0/4] staging: rtl8723bs: Change several files of the driver

[Outreachy kernel] [PATCH 0/4] staging: rtl8723bs: Change several files of the driver

[Fabio M. De Francesco]

Remove camelcase, correct misspelled words in comments, remove an unused
variable, change type and use of another variable. These changes affect 
files in different subdirectories of this driver.

Fabio M. De Francesco (4):
  staging: rtl8723bs: Remove camelcase in several files
  staging: rtl8723bs: include: Fix misspelled words in comments
  staging: rtl8723bs: core: Remove an unused variable
  staging: rtl8723bs: Change the type and use of a variable

 drivers/staging/rtl8723bs/core/rtw_cmd.c      |  2 +-
 .../staging/rtl8723bs/core/rtw_ieee80211.c    |  4 +--
 drivers/staging/rtl8723bs/core/rtw_mlme.c     |  2 +-
 drivers/staging/rtl8723bs/core/rtw_pwrctrl.c  | 18 +++++-----
 drivers/staging/rtl8723bs/hal/hal_intf.c      |  2 +-
 drivers/staging/rtl8723bs/hal/rtl8723b_dm.c   |  6 ++--
 .../staging/rtl8723bs/hal/rtl8723b_hal_init.c |  2 +-
 drivers/staging/rtl8723bs/hal/sdio_ops.c      | 14 ++++----
 .../rtl8723bs/include/Hal8192CPhyReg.h        |  8 ++---
 .../staging/rtl8723bs/include/basic_types.h   |  2 +-
 drivers/staging/rtl8723bs/include/drv_types.h |  2 +-
 drivers/staging/rtl8723bs/include/hal_com.h   |  2 +-
 .../staging/rtl8723bs/include/hal_com_reg.h   | 34 +++++++++----------
 drivers/staging/rtl8723bs/include/hal_data.h  |  2 +-
 .../staging/rtl8723bs/include/hal_pwr_seq.h   |  2 +-
 drivers/staging/rtl8723bs/include/rtw_cmd.h   |  6 ++--
 drivers/staging/rtl8723bs/include/rtw_mlme.h  | 18 +++++-----
 .../staging/rtl8723bs/include/rtw_mlme_ext.h  |  2 +-
 drivers/staging/rtl8723bs/include/rtw_mp.h    |  2 +-
 .../staging/rtl8723bs/include/rtw_pwrctrl.h   |  4 +--
 drivers/staging/rtl8723bs/include/rtw_recv.h  |  4 +--
 drivers/staging/rtl8723bs/include/rtw_xmit.h  |  2 +-
 drivers/staging/rtl8723bs/include/sta_info.h  |  2 +-
 drivers/staging/rtl8723bs/include/wifi.h      |  2 +-
 24 files changed, 71 insertions(+), 73 deletions(-)

-- 
2.31.1

[Outreachy kernel] [PATCH v7 1/3] staging: rtl8723bs: Remove camelcase in several files

[Fabio M. De Francesco]

Remove camelcase in bFwCurrentInPSMode, a variable used by code
of several subdirectories/files of the driver. Issue detected by
checkpatch.pl. Delete the unnecessary "b" (that stands for "byte") from
the beginning of the name.

Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---

Changes from v6: Edit against the wrong patch (again).
Changes from v5: Edit against the wrong patch.
Changes from v4: Mention the removal of the initial "b" in log message.
Changes from v3: Fix errors in the format of the patch.
Changes from v2: Remove unnecessary comment. Shortened a function name.
Changes from v1: No changes to the code but only to the subject for the
purpose to differentiate this patch because other removes of camelcase
have been made in other files of the same directory.

 drivers/staging/rtl8723bs/core/rtw_cmd.c       |  2 +-
 drivers/staging/rtl8723bs/core/rtw_mlme.c      |  2 +-
 drivers/staging/rtl8723bs/core/rtw_pwrctrl.c   | 18 +++++++++---------
 drivers/staging/rtl8723bs/hal/hal_intf.c       |  2 +-
 drivers/staging/rtl8723bs/hal/rtl8723b_dm.c    |  6 +++---
 .../staging/rtl8723bs/hal/rtl8723b_hal_init.c  |  2 +-
 drivers/staging/rtl8723bs/hal/sdio_ops.c       | 14 +++++++-------
 .../staging/rtl8723bs/include/rtw_pwrctrl.h    |  2 +-
 8 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c b/drivers/staging/rtl8723bs/core/rtw_cmd.c
index e94eb1138cf1..32079e0f71d5 100644
--- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
+++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
@@ -1507,7 +1507,7 @@ static void rtw_lps_change_dtim_hdl(struct adapter *padapter, u8 dtim)
 	if (pwrpriv->dtim != dtim)
 		pwrpriv->dtim = dtim;
 
-	if ((pwrpriv->bFwCurrentInPSMode == true) && (pwrpriv->pwr_mode > PS_MODE_ACTIVE)) {
+	if ((pwrpriv->fw_current_in_ps_mode == true) && (pwrpriv->pwr_mode > PS_MODE_ACTIVE)) {
 		u8 ps_mode = pwrpriv->pwr_mode;
 
 		rtw_hal_set_hwreg(padapter, HW_VAR_H2C_FW_PWRMODE, (u8 *)(&ps_mode));
diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme.c b/drivers/staging/rtl8723bs/core/rtw_mlme.c
index a7e40aaae2d9..895997868c81 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme.c
@@ -1684,7 +1684,7 @@ void rtw_dynamic_check_timer_handler(struct adapter *adapter)
 	if (adapter->net_closed)
 		return;
 
-	if ((adapter_to_pwrctl(adapter)->bFwCurrentInPSMode)
+	if ((adapter_to_pwrctl(adapter)->fw_current_in_ps_mode)
 		&& !(hal_btcoex_IsBtControlLps(adapter))
 		) {
 		u8 bEnterPS;
diff --git a/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c b/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c
index f7465cf90c46..481e2ad60853 100644
--- a/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c
+++ b/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c
@@ -365,7 +365,7 @@ void rtw_set_ps_mode(struct adapter *padapter, u8 ps_mode, u8 smart_ps, u8 bcn_a
 			rtw_set_rpwm(padapter, PS_STATE_S4);
 
 			rtw_hal_set_hwreg(padapter, HW_VAR_H2C_FW_PWRMODE, (u8 *)(&ps_mode));
-			pwrpriv->bFwCurrentInPSMode = false;
+			pwrpriv->fw_current_in_ps_mode = false;
 
 			hal_btcoex_LpsNotify(padapter, ps_mode);
 		}
@@ -377,7 +377,7 @@ void rtw_set_ps_mode(struct adapter *padapter, u8 ps_mode, u8 smart_ps, u8 bcn_a
 
 			hal_btcoex_LpsNotify(padapter, ps_mode);
 
-			pwrpriv->bFwCurrentInPSMode = true;
+			pwrpriv->fw_current_in_ps_mode = true;
 			pwrpriv->pwr_mode = ps_mode;
 			pwrpriv->smart_ps = smart_ps;
 			pwrpriv->bcn_ant_mode = bcn_ant_mode;
@@ -734,7 +734,7 @@ s32 rtw_register_task_alive(struct adapter *padapter, u32 task)
 
 	register_task_alive(pwrctrl, task);
 
-	if (pwrctrl->bFwCurrentInPSMode) {
+	if (pwrctrl->fw_current_in_ps_mode) {
 		if (pwrctrl->cpwm < pslv) {
 			if (pwrctrl->cpwm < PS_STATE_S2)
 				res = _FAIL;
@@ -782,7 +782,7 @@ void rtw_unregister_task_alive(struct adapter *padapter, u32 task)
 
 	unregister_task_alive(pwrctrl, task);
 
-	if ((pwrctrl->pwr_mode != PS_MODE_ACTIVE) && pwrctrl->bFwCurrentInPSMode) {
+	if ((pwrctrl->pwr_mode != PS_MODE_ACTIVE) && pwrctrl->fw_current_in_ps_mode) {
 		if (pwrctrl->cpwm > pslv)
 			if ((pslv >= PS_STATE_S2) || (pwrctrl->alives == 0))
 				rtw_set_rpwm(padapter, pslv);
@@ -819,7 +819,7 @@ s32 rtw_register_tx_alive(struct adapter *padapter)
 
 	register_task_alive(pwrctrl, XMIT_ALIVE);
 
-	if (pwrctrl->bFwCurrentInPSMode) {
+	if (pwrctrl->fw_current_in_ps_mode) {
 		if (pwrctrl->cpwm < pslv) {
 			if (pwrctrl->cpwm < PS_STATE_S2)
 				res = _FAIL;
@@ -864,7 +864,7 @@ s32 rtw_register_cmd_alive(struct adapter *padapter)
 
 	register_task_alive(pwrctrl, CMD_ALIVE);
 
-	if (pwrctrl->bFwCurrentInPSMode) {
+	if (pwrctrl->fw_current_in_ps_mode) {
 		if (pwrctrl->cpwm < pslv) {
 			if (pwrctrl->cpwm < PS_STATE_S2)
 				res = _FAIL;
@@ -909,7 +909,7 @@ void rtw_unregister_tx_alive(struct adapter *padapter)
 
 	unregister_task_alive(pwrctrl, XMIT_ALIVE);
 
-	if ((pwrctrl->pwr_mode != PS_MODE_ACTIVE) && pwrctrl->bFwCurrentInPSMode) {
+	if ((pwrctrl->pwr_mode != PS_MODE_ACTIVE) && pwrctrl->fw_current_in_ps_mode) {
 		if (pwrctrl->cpwm > pslv)
 			if ((pslv >= PS_STATE_S2) || (pwrctrl->alives == 0))
 				rtw_set_rpwm(padapter, pslv);
@@ -945,7 +945,7 @@ void rtw_unregister_cmd_alive(struct adapter *padapter)
 
 	unregister_task_alive(pwrctrl, CMD_ALIVE);
 
-	if ((pwrctrl->pwr_mode != PS_MODE_ACTIVE) && pwrctrl->bFwCurrentInPSMode) {
+	if ((pwrctrl->pwr_mode != PS_MODE_ACTIVE) && pwrctrl->fw_current_in_ps_mode) {
 		if (pwrctrl->cpwm > pslv) {
 			if ((pslv >= PS_STATE_S2) || (pwrctrl->alives == 0))
 				rtw_set_rpwm(padapter, pslv);
@@ -978,7 +978,7 @@ void rtw_init_pwrctrl_priv(struct adapter *padapter)
 	pwrctrlpriv->power_mgnt = padapter->registrypriv.power_mgnt;/*  PS_MODE_MIN; */
 	pwrctrlpriv->bLeisurePs = pwrctrlpriv->power_mgnt != PS_MODE_ACTIVE;
 
-	pwrctrlpriv->bFwCurrentInPSMode = false;
+	pwrctrlpriv->fw_current_in_ps_mode = false;
 
 	pwrctrlpriv->rpwm = 0;
 	pwrctrlpriv->cpwm = PS_STATE_S4;
diff --git a/drivers/staging/rtl8723bs/hal/hal_intf.c b/drivers/staging/rtl8723bs/hal/hal_intf.c
index bc9ae2088754..96fe172ced8d 100644
--- a/drivers/staging/rtl8723bs/hal/hal_intf.c
+++ b/drivers/staging/rtl8723bs/hal/hal_intf.c
@@ -348,7 +348,7 @@ void rtw_hal_dm_watchdog(struct adapter *padapter)
 
 void rtw_hal_dm_watchdog_in_lps(struct adapter *padapter)
 {
-	if (adapter_to_pwrctl(padapter)->bFwCurrentInPSMode == true) {
+	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode == true) {
 		if (padapter->HalFunc.hal_dm_watchdog_in_lps)
 			padapter->HalFunc.hal_dm_watchdog_in_lps(padapter); /* this function caller is in interrupt context */
 	}
diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_dm.c b/drivers/staging/rtl8723bs/hal/rtl8723b_dm.c
index c2e9e4a0be22..23be025ceb5b 100644
--- a/drivers/staging/rtl8723bs/hal/rtl8723b_dm.c
+++ b/drivers/staging/rtl8723bs/hal/rtl8723b_dm.c
@@ -141,7 +141,7 @@ void rtl8723b_InitHalDm(struct adapter *Adapter)
 
 void rtl8723b_HalDmWatchDog(struct adapter *Adapter)
 {
-	bool bFwCurrentInPSMode = false;
+	bool fw_current_in_ps_mode = false;
 	bool bFwPSAwake = true;
 	u8 hw_init_completed = false;
 	struct hal_com_data *pHalData = GET_HAL_DATA(Adapter);
@@ -151,12 +151,12 @@ void rtl8723b_HalDmWatchDog(struct adapter *Adapter)
 	if (hw_init_completed == false)
 		goto skip_dm;
 
-	bFwCurrentInPSMode = adapter_to_pwrctl(Adapter)->bFwCurrentInPSMode;
+	fw_current_in_ps_mode = adapter_to_pwrctl(Adapter)->fw_current_in_ps_mode;
 	rtw_hal_get_hwreg(Adapter, HW_VAR_FWLPS_RF_ON, (u8 *)(&bFwPSAwake));
 
 	if (
 		(hw_init_completed == true) &&
-		((!bFwCurrentInPSMode) && bFwPSAwake)
+		((!fw_current_in_ps_mode) && bFwPSAwake)
 	) {
 		/*  */
 		/*  Calculate Tx/Rx statistics. */
diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
index dcb7cb131432..c9f17a742ff0 100644
--- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
+++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
@@ -430,7 +430,7 @@ void rtl8723b_InitializeFirmwareVars(struct adapter *padapter)
 	struct hal_com_data *pHalData = GET_HAL_DATA(padapter);
 
 	/*  Init Fw LPS related. */
-	adapter_to_pwrctl(padapter)->bFwCurrentInPSMode = false;
+	adapter_to_pwrctl(padapter)->fw_current_in_ps_mode = false;
 
 	/* Init H2C cmd. */
 	rtw_write8(padapter, REG_HMETFR, 0x0f);
diff --git a/drivers/staging/rtl8723bs/hal/sdio_ops.c b/drivers/staging/rtl8723bs/hal/sdio_ops.c
index af7f846f90fe..abe8f2f8f452 100644
--- a/drivers/staging/rtl8723bs/hal/sdio_ops.c
+++ b/drivers/staging/rtl8723bs/hal/sdio_ops.c
@@ -173,7 +173,7 @@ static u32 sdio_read32(struct intf_hdl *intfhdl, u32 addr)
 	if (
 		((device_id == WLAN_IOREG_DEVICE_ID) && (offset < 0x100)) ||
 		(!mac_pwr_ctrl_on) ||
-		(adapter_to_pwrctl(adapter)->bFwCurrentInPSMode)
+		(adapter_to_pwrctl(adapter)->fw_current_in_ps_mode)
 	) {
 		err = sd_cmd52_read(intfhdl, ftaddr, 4, (u8 *)&le_tmp);
 #ifdef SDIO_DEBUG_IO
@@ -230,7 +230,7 @@ static s32 sdio_readN(struct intf_hdl *intfhdl, u32 addr, u32 cnt, u8 *buf)
 	if (
 		((device_id == WLAN_IOREG_DEVICE_ID) && (offset < 0x100)) ||
 		(!mac_pwr_ctrl_on) ||
-		(adapter_to_pwrctl(adapter)->bFwCurrentInPSMode)
+		(adapter_to_pwrctl(adapter)->fw_current_in_ps_mode)
 	)
 		return sd_cmd52_read(intfhdl, ftaddr, cnt, buf);
 
@@ -297,7 +297,7 @@ static s32 sdio_write32(struct intf_hdl *intfhdl, u32 addr, u32 val)
 	if (
 		((device_id == WLAN_IOREG_DEVICE_ID) && (offset < 0x100)) ||
 		(!mac_pwr_ctrl_on) ||
-		(adapter_to_pwrctl(adapter)->bFwCurrentInPSMode)
+		(adapter_to_pwrctl(adapter)->fw_current_in_ps_mode)
 	) {
 		le_tmp = cpu_to_le32(val);
 
@@ -334,7 +334,7 @@ static s32 sdio_writeN(struct intf_hdl *intfhdl, u32 addr, u32 cnt, u8 *buf)
 	if (
 		((device_id == WLAN_IOREG_DEVICE_ID) && (offset < 0x100)) ||
 		(!mac_pwr_ctrl_on) ||
-		(adapter_to_pwrctl(adapter)->bFwCurrentInPSMode)
+		(adapter_to_pwrctl(adapter)->fw_current_in_ps_mode)
 	)
 		return sd_cmd52_write(intfhdl, ftaddr, cnt, buf);
 
@@ -565,7 +565,7 @@ s32 sdio_local_read(
 	rtw_hal_get_hwreg(adapter, HW_VAR_APFM_ON_MAC, &mac_pwr_ctrl_on);
 	if (
 		(!mac_pwr_ctrl_on) ||
-		(adapter_to_pwrctl(adapter)->bFwCurrentInPSMode)
+		(adapter_to_pwrctl(adapter)->fw_current_in_ps_mode)
 	)
 		return sd_cmd52_read(intfhdl, addr, cnt, buf);
 
@@ -611,7 +611,7 @@ s32 sdio_local_write(
 	rtw_hal_get_hwreg(adapter, HW_VAR_APFM_ON_MAC, &mac_pwr_ctrl_on);
 	if (
 		(!mac_pwr_ctrl_on) ||
-		(adapter_to_pwrctl(adapter)->bFwCurrentInPSMode)
+		(adapter_to_pwrctl(adapter)->fw_current_in_ps_mode)
 	)
 		return sd_cmd52_write(intfhdl, addr, cnt, buf);
 
@@ -660,7 +660,7 @@ static u32 sdio_local_cmd53_read4byte(struct adapter *adapter, u32 addr)
 
 	hal_sdio_get_cmd_addr_8723b(adapter, SDIO_LOCAL_DEVICE_ID, addr, &addr);
 	rtw_hal_get_hwreg(adapter, HW_VAR_APFM_ON_MAC, &mac_pwr_ctrl_on);
-	if (!mac_pwr_ctrl_on || adapter_to_pwrctl(adapter)->bFwCurrentInPSMode) {
+	if (!mac_pwr_ctrl_on || adapter_to_pwrctl(adapter)->fw_current_in_ps_mode) {
 		sd_cmd52_read(intfhdl, addr, 4, (u8 *)&le_tmp);
 		val = le32_to_cpu(le_tmp);
 	} else {
diff --git a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
index 2e739a17dd95..5450d20b44a6 100644
--- a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
+++ b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
@@ -203,7 +203,7 @@ struct pwrctrl_priv {
 	u8 LpsIdleCount;
 	u8 power_mgnt;
 	u8 org_power_mgnt;
-	u8 bFwCurrentInPSMode;
+	u8 fw_current_in_ps_mode;
 	unsigned long	DelayLPSLastTimeStamp;
 	s32		pnp_current_pwr_state;
 	u8 pnp_bstop_trx;
-- 
2.31.1

Re: [Outreachy kernel] [PATCH v7 1/3] staging: rtl8723bs: Remove camelcase in several files

[Greg KH]

On Sat, Apr 10, 2021 at 11:22:29AM +0200, Fabio M. De Francesco wrote:
> Remove camelcase in bFwCurrentInPSMode, a variable used by code
> of several subdirectories/files of the driver. Issue detected by
> checkpatch.pl. Delete the unnecessary "b" (that stands for "byte") from
> the beginning of the name.
> 
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
> 

Why is there a "v7" in this subject line, but not all the other lines?

It should be in all of them, including the 0/X email, git format-patch
will create it automatically if you tell it to.

thanks,

greg k-h

Re: [Outreachy kernel] [PATCH v7 1/3] staging: rtl8723bs: Remove camelcase in several files

[Fabio M. De Francesco]

On Saturday, April 10, 2021 11:32:00 AM CEST Greg KH wrote:
> On Sat, Apr 10, 2021 at 11:22:29AM +0200, Fabio M. De Francesco wrote:
> > Remove camelcase in bFwCurrentInPSMode, a variable used by code
> > of several subdirectories/files of the driver. Issue detected by
> > checkpatch.pl. Delete the unnecessary "b" (that stands for "byte") from
> > the beginning of the name.
> > 
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> 
> Why is there a "v7" in this subject line, but not all the other lines?
> 

It's v7 because only this file (and not the others in the series had six 
previous versions that you dropped. Please remind that it was already sent 
alone (not in a series) several times. The changelog is in the body.

Should I drop that "v7" and the changelog, and the send the patch series 
anew?

Please, I'm waiting your instructions on what to do.

Thanks,

Fabio
>
> It should be in all of them, including the 0/X email, git format-patch
> will create it automatically if you tell it to.
> 
> thanks,
> 
> greg k-h

Re: [Outreachy kernel] [PATCH v7 1/3] staging: rtl8723bs: Remove camelcase in several files

[Greg KH]

On Sat, Apr 10, 2021 at 11:45:18AM +0200, Fabio M. De Francesco wrote:
> On Saturday, April 10, 2021 11:32:00 AM CEST Greg KH wrote:
> > On Sat, Apr 10, 2021 at 11:22:29AM +0200, Fabio M. De Francesco wrote:
> > > Remove camelcase in bFwCurrentInPSMode, a variable used by code
> > > of several subdirectories/files of the driver. Issue detected by
> > > checkpatch.pl. Delete the unnecessary "b" (that stands for "byte") from
> > > the beginning of the name.
> > > 
> > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > ---
> > 
> > Why is there a "v7" in this subject line, but not all the other lines?
> > 
> 
> It's v7 because only this file (and not the others in the series had six 
> previous versions that you dropped. Please remind that it was already sent 
> alone (not in a series) several times. The changelog is in the body.
> 
> Should I drop that "v7" and the changelog, and the send the patch series 
> anew?

It's a v2 series now, right?  :)

> Please, I'm waiting your instructions on what to do.

Don't "wait" for anyone, least of all me.  Do what you think is needed,
after waiting a bit of time, there's no rush here...

thanks,

greg k-h

[Outreachy kernel] [PATCH 2/4] staging: rtl8723bs: include: Fix misspelled words in comments

[Fabio M. De Francesco]

Correct misspelled words in comments of several files. Issue (largely)
detected by checkpatch.pl.

Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---

Changes from v1: Substitute "mispelled" with "misspelled" in the Subject.

 .../rtl8723bs/include/Hal8192CPhyReg.h        |  8 ++---
 .../staging/rtl8723bs/include/basic_types.h   |  2 +-
 drivers/staging/rtl8723bs/include/drv_types.h |  2 +-
 drivers/staging/rtl8723bs/include/hal_com.h   |  2 +-
 .../staging/rtl8723bs/include/hal_com_reg.h   | 34 +++++++++----------
 drivers/staging/rtl8723bs/include/hal_data.h  |  2 +-
 .../staging/rtl8723bs/include/hal_pwr_seq.h   |  2 +-
 drivers/staging/rtl8723bs/include/rtw_cmd.h   |  6 ++--
 drivers/staging/rtl8723bs/include/rtw_mlme.h  | 18 +++++-----
 .../staging/rtl8723bs/include/rtw_mlme_ext.h  |  2 +-
 drivers/staging/rtl8723bs/include/rtw_mp.h    |  2 +-
 .../staging/rtl8723bs/include/rtw_pwrctrl.h   |  2 +-
 drivers/staging/rtl8723bs/include/rtw_recv.h  |  4 +--
 drivers/staging/rtl8723bs/include/rtw_xmit.h  |  2 +-
 drivers/staging/rtl8723bs/include/sta_info.h  |  2 +-
 drivers/staging/rtl8723bs/include/wifi.h      |  2 +-
 16 files changed, 46 insertions(+), 46 deletions(-)

diff --git a/drivers/staging/rtl8723bs/include/Hal8192CPhyReg.h b/drivers/staging/rtl8723bs/include/Hal8192CPhyReg.h
index fb80901f0788..4b3a7c051630 100644
--- a/drivers/staging/rtl8723bs/include/Hal8192CPhyReg.h
+++ b/drivers/staging/rtl8723bs/include/Hal8192CPhyReg.h
@@ -34,7 +34,7 @@
 /*--------------------------Define Parameters-------------------------------*/
 
 /*  */
-/*        8192S Regsiter offset definition */
+/*        8192S Register offset definition */
 /*  */
 
 /*  */
@@ -43,7 +43,7 @@
 /*  2. 0x800/0x900/0xA00/0xC00/0xD00/0xE00 */
 /*  3. RF register 0x00-2E */
 /*  4. Bit Mask for BB/RF register */
-/*  5. Other defintion for BB/RF R/W */
+/*  5. Other definition for BB/RF R/W */
 /*  */
 
 
@@ -137,7 +137,7 @@
 #define		rFPGA0_AnalogParameter3		0x888	/*  Useless now */
 #define		rFPGA0_AnalogParameter4		0x88c
 
-#define		rFPGA0_XA_LSSIReadBack		0x8a0	/*  Tranceiver LSSI Readback */
+#define		rFPGA0_XA_LSSIReadBack		0x8a0	/*  Transceiver LSSI Readback */
 #define		rFPGA0_XB_LSSIReadBack		0x8a4
 #define		rFPGA0_XC_LSSIReadBack		0x8a8
 #define		rFPGA0_XD_LSSIReadBack		0x8ac
@@ -206,7 +206,7 @@
 #define		rOFDM0_TRSWIsolation		0xc0c
 
 #define		rOFDM0_XARxAFE			0xc10  /* RxIQ DC offset, Rx digital filter, DC notch filter */
-#define		rOFDM0_XARxIQImbalance		0xc14  /* RxIQ imblance matrix */
+#define		rOFDM0_XARxIQImbalance		0xc14  /* RxIQ imbalance matrix */
 #define		rOFDM0_XBRxAFE				0xc18
 #define		rOFDM0_XBRxIQImbalance		0xc1c
 #define		rOFDM0_XCRxAFE				0xc20
diff --git a/drivers/staging/rtl8723bs/include/basic_types.h b/drivers/staging/rtl8723bs/include/basic_types.h
index 76304086107a..57bb717327ce 100644
--- a/drivers/staging/rtl8723bs/include/basic_types.h
+++ b/drivers/staging/rtl8723bs/include/basic_types.h
@@ -187,7 +187,7 @@
 		); \
 }
 
-/*  Get the N-bytes aligment offset from the current length */
+/*  Get the N-bytes alignent offset from the current length */
 #define N_BYTE_ALIGMENT(__Value, __Aligment) ((__Aligment == 1) ? (__Value) : (((__Value + __Aligment - 1) / __Aligment) * __Aligment))
 
 #define TEST_FLAG(__Flag, __testFlag)		(((__Flag) & (__testFlag)) != 0)
diff --git a/drivers/staging/rtl8723bs/include/drv_types.h b/drivers/staging/rtl8723bs/include/drv_types.h
index 19da27fb5ddf..f1f588d38a60 100644
--- a/drivers/staging/rtl8723bs/include/drv_types.h
+++ b/drivers/staging/rtl8723bs/include/drv_types.h
@@ -424,7 +424,7 @@ struct adapter {
 	/* 	The driver will show up the desired channel number when this flag is 1. */
 	u8 bNotifyChannelChange;
 
-	/* pbuddystruct adapter is used only in  two inteface case, (iface_nums =2 in struct dvobj_priv) */
+	/* pbuddystruct adapter is used only in two interface case, (iface_nums =2 in struct dvobj_priv) */
 	/* PRIMARY ADAPTER's buddy is SECONDARY_ADAPTER */
 	/* SECONDARY_ADAPTER's buddy is PRIMARY_ADAPTER */
 	/* for iface_id > SECONDARY_ADAPTER(IFACE_ID1), refer to padapters[iface_id]  in struct dvobj_priv */
diff --git a/drivers/staging/rtl8723bs/include/hal_com.h b/drivers/staging/rtl8723bs/include/hal_com.h
index a1e1b76b5d8a..6bcc443d59fb 100644
--- a/drivers/staging/rtl8723bs/include/hal_com.h
+++ b/drivers/staging/rtl8723bs/include/hal_com.h
@@ -158,7 +158,7 @@
 (rate == DESC_RATEVHTSS2MCS6) ? "VHTSS2MCS6" : \
 (rate == DESC_RATEVHTSS2MCS7) ? "VHTSS2MCS7" : \
 (rate == DESC_RATEVHTSS2MCS8) ? "VHTSS2MCS8" : \
-(rate == DESC_RATEVHTSS2MCS9) ? "VHTSS2MCS9" : "UNKNOW"
+(rate == DESC_RATEVHTSS2MCS9) ? "VHTSS2MCS9" : "UNKNOWN"
 
 
 enum{
diff --git a/drivers/staging/rtl8723bs/include/hal_com_reg.h b/drivers/staging/rtl8723bs/include/hal_com_reg.h
index 2cd18eb57244..b14585cb0233 100644
--- a/drivers/staging/rtl8723bs/include/hal_com_reg.h
+++ b/drivers/staging/rtl8723bs/include/hal_com_reg.h
@@ -768,14 +768,14 @@ Default: 00b.
 #define IMR_BCNDMAINT3			BIT28		/*  Beacon DMA Interrupt 3 */
 #define IMR_BCNDMAINT2			BIT27		/*  Beacon DMA Interrupt 2 */
 #define IMR_BCNDMAINT1			BIT26		/*  Beacon DMA Interrupt 1 */
-#define IMR_BCNDOK8				BIT25		/*  Beacon Queue DMA OK Interrup 8 */
-#define IMR_BCNDOK7				BIT24		/*  Beacon Queue DMA OK Interrup 7 */
-#define IMR_BCNDOK6				BIT23		/*  Beacon Queue DMA OK Interrup 6 */
-#define IMR_BCNDOK5				BIT22		/*  Beacon Queue DMA OK Interrup 5 */
-#define IMR_BCNDOK4				BIT21		/*  Beacon Queue DMA OK Interrup 4 */
-#define IMR_BCNDOK3				BIT20		/*  Beacon Queue DMA OK Interrup 3 */
-#define IMR_BCNDOK2				BIT19		/*  Beacon Queue DMA OK Interrup 2 */
-#define IMR_BCNDOK1				BIT18		/*  Beacon Queue DMA OK Interrup 1 */
+#define IMR_BCNDOK8				BIT25		/*  Beacon Queue DMA OK Interrupt 8 */
+#define IMR_BCNDOK7				BIT24		/*  Beacon Queue DMA OK Interrupt 7 */
+#define IMR_BCNDOK6				BIT23		/*  Beacon Queue DMA OK Interrupt 6 */
+#define IMR_BCNDOK5				BIT22		/*  Beacon Queue DMA OK Interrupt 5 */
+#define IMR_BCNDOK4				BIT21		/*  Beacon Queue DMA OK Interrupt 4 */
+#define IMR_BCNDOK3				BIT20		/*  Beacon Queue DMA OK Interrupt 3 */
+#define IMR_BCNDOK2				BIT19		/*  Beacon Queue DMA OK Interrupt 2 */
+#define IMR_BCNDOK1				BIT18		/*  Beacon Queue DMA OK Interrupt 1 */
 #define IMR_TIMEOUT2			BIT17		/*  Timeout interrupt 2 */
 #define IMR_TIMEOUT1			BIT16		/*  Timeout interrupt 1 */
 #define IMR_TXFOVW				BIT15		/*  Transmit FIFO Overflow */
@@ -784,9 +784,9 @@ Default: 00b.
 #define IMR_RXFOVW				BIT12		/*  Receive FIFO Overflow */
 #define IMR_RDU					BIT11		/*  Receive Descriptor Unavailable */
 #define IMR_ATIMEND				BIT10		/*  For 92C, ATIM Window End Interrupt. For 8723 and later ICs, it also means P2P CTWin End interrupt. */
-#define IMR_BDOK				BIT9		/*  Beacon Queue DMA OK Interrup */
+#define IMR_BDOK				BIT9		/*  Beacon Queue DMA OK Interrupt */
 #define IMR_HIGHDOK				BIT8		/*  High Queue DMA OK Interrupt */
-#define IMR_TBDOK				BIT7		/*  Transmit Beacon OK interrup */
+#define IMR_TBDOK				BIT7		/*  Transmit Beacon OK interrupt */
 #define IMR_MGNTDOK			BIT6		/*  Management Queue DMA OK Interrupt */
 #define IMR_TBDER				BIT5		/*  For 92C, Transmit Beacon Error Interrupt */
 #define IMR_BKDOK				BIT4		/*  AC_BK DMA OK Interrupt */
@@ -956,13 +956,13 @@ Default: 00b.
 #define IMR_BCNDMAINT3_88E		BIT23		/*  Beacon DMA Interrupt 3 */
 #define IMR_BCNDMAINT2_88E		BIT22		/*  Beacon DMA Interrupt 2 */
 #define IMR_BCNDMAINT1_88E		BIT21		/*  Beacon DMA Interrupt 1 */
-#define IMR_BCNDOK7_88E			BIT20		/*  Beacon Queue DMA OK Interrup 7 */
-#define IMR_BCNDOK6_88E			BIT19		/*  Beacon Queue DMA OK Interrup 6 */
-#define IMR_BCNDOK5_88E			BIT18		/*  Beacon Queue DMA OK Interrup 5 */
-#define IMR_BCNDOK4_88E			BIT17		/*  Beacon Queue DMA OK Interrup 4 */
-#define IMR_BCNDOK3_88E			BIT16		/*  Beacon Queue DMA OK Interrup 3 */
-#define IMR_BCNDOK2_88E			BIT15		/*  Beacon Queue DMA OK Interrup 2 */
-#define IMR_BCNDOK1_88E			BIT14		/*  Beacon Queue DMA OK Interrup 1 */
+#define IMR_BCNDOK7_88E			BIT20		/*  Beacon Queue DMA OK Interrupt 7 */
+#define IMR_BCNDOK6_88E			BIT19		/*  Beacon Queue DMA OK Interrupt 6 */
+#define IMR_BCNDOK5_88E			BIT18		/*  Beacon Queue DMA OK Interrupt 5 */
+#define IMR_BCNDOK4_88E			BIT17		/*  Beacon Queue DMA OK Interrupt 4 */
+#define IMR_BCNDOK3_88E			BIT16		/*  Beacon Queue DMA OK Interrupt 3 */
+#define IMR_BCNDOK2_88E			BIT15		/*  Beacon Queue DMA OK Interrupt 2 */
+#define IMR_BCNDOK1_88E			BIT14		/*  Beacon Queue DMA OK Interrupt 1 */
 #define IMR_ATIMEND_E_88E			BIT13		/*  ATIM Window End Extension for Win7 */
 #define IMR_TXERR_88E				BIT11		/*  Tx Error Flag Interrupt Status, write 1 clear. */
 #define IMR_RXERR_88E				BIT10		/*  Rx Error Flag INT Status, Write 1 clear */
diff --git a/drivers/staging/rtl8723bs/include/hal_data.h b/drivers/staging/rtl8723bs/include/hal_data.h
index df5c7b747498..babcb03a7c23 100644
--- a/drivers/staging/rtl8723bs/include/hal_data.h
+++ b/drivers/staging/rtl8723bs/include/hal_data.h
@@ -389,7 +389,7 @@ struct hal_com_data {
 	u8 OutEpQueueSel;
 	u8 OutEpNumber;
 
-	/*  2010/12/10 MH Add for USB aggreation mode dynamic shceme. */
+	/*  2010/12/10 MH Add for USB aggregation mode dynamic scheme. */
 	bool		UsbRxHighSpeedMode;
 
 	/*  2010/11/22 MH Add for slim combo debug mode selective. */
diff --git a/drivers/staging/rtl8723bs/include/hal_pwr_seq.h b/drivers/staging/rtl8723bs/include/hal_pwr_seq.h
index 0837506b6be8..0a2e60770668 100644
--- a/drivers/staging/rtl8723bs/include/hal_pwr_seq.h
+++ b/drivers/staging/rtl8723bs/include/hal_pwr_seq.h
@@ -14,7 +14,7 @@
 	4: LPS--Low Power State
 	5: SUS--Suspend
 
-	The transision from different states are defined below
+	The transition from different states are defined below
 	TRANS_CARDEMU_TO_ACT
 	TRANS_ACT_TO_CARDEMU
 	TRANS_CARDEMU_TO_SUS
diff --git a/drivers/staging/rtl8723bs/include/rtw_cmd.h b/drivers/staging/rtl8723bs/include/rtw_cmd.h
index 87cbad525393..517ae3b51386 100644
--- a/drivers/staging/rtl8723bs/include/rtw_cmd.h
+++ b/drivers/staging/rtl8723bs/include/rtw_cmd.h
@@ -136,7 +136,7 @@ enum {
 	RESET_SECURITYPRIV, /*  add for CONFIG_IEEE80211W, none 11w also can use */
 	FREE_ASSOC_RESOURCES, /*  add for CONFIG_IEEE80211W, none 11w also can use */
 	DM_IN_LPS_WK_CID,
-	DM_RA_MSK_WK_CID, /* add for STA update RAMask when bandwith change. */
+	DM_RA_MSK_WK_CID, /* add for STA update RAMask when bandwidth change. */
 	BEAMFORMING_WK_CID,
 	LPS_CHANGE_DTIM_CID,
 	BTINFO_WK_CID,
@@ -514,7 +514,7 @@ struct drvextra_cmd_parm {
 	unsigned char *pbuf;
 };
 
-/*------------------- Below are used for RF/BB tunning ---------------------*/
+/*------------------- Below are used for RF/BB tuning ---------------------*/
 
 struct	getcountjudge_rsp {
 	u8 count_judge[MAX_RATES_LENGTH];
@@ -567,7 +567,7 @@ struct RunInThread_param {
 
 Result:
 0x00: success
-0x01: sucess, and check Response.
+0x01: success, and check Response.
 0x02: cmd ignored due to duplicated sequcne number
 0x03: cmd dropped due to invalid cmd code
 0x04: reserved.
diff --git a/drivers/staging/rtl8723bs/include/rtw_mlme.h b/drivers/staging/rtl8723bs/include/rtw_mlme.h
index 87a1fa8f347e..5deb73fe3885 100644
--- a/drivers/staging/rtl8723bs/include/rtw_mlme.h
+++ b/drivers/staging/rtl8723bs/include/rtw_mlme.h
@@ -57,11 +57,11 @@
 
 /* ifdef UNDER_MPTEST */
 #define	WIFI_MP_STATE							0x00010000
-#define	WIFI_MP_CTX_BACKGROUND				0x00020000	/*  in continous tx background */
-#define	WIFI_MP_CTX_ST						0x00040000	/*  in continous tx with single-tone */
-#define	WIFI_MP_CTX_BACKGROUND_PENDING	0x00080000	/*  pending in continous tx background due to out of skb */
-#define	WIFI_MP_CTX_CCK_HW					0x00100000	/*  in continous tx */
-#define	WIFI_MP_CTX_CCK_CS					0x00200000	/*  in continous tx with carrier suppression */
+#define	WIFI_MP_CTX_BACKGROUND				0x00020000	/*  in continuous tx background */
+#define	WIFI_MP_CTX_ST						0x00040000	/*  in continuous tx with single-tone */
+#define	WIFI_MP_CTX_BACKGROUND_PENDING	0x00080000	/*  pending in continuous tx background due to out of skb */
+#define	WIFI_MP_CTX_CCK_HW					0x00100000	/*  in continuous tx */
+#define	WIFI_MP_CTX_CCK_CS					0x00200000	/*  in continuous tx with carrier suppression */
 #define   WIFI_MP_LPBK_STATE					0x00400000
 /* endif */
 
@@ -168,7 +168,7 @@ struct tx_provdisc_req_info {
 	u8 			benable;					/* 	This provision discovery request frame is trigger to send or not */
 };
 
-struct rx_provdisc_req_info {	/* When peer device issue prov_disc_req first, we should store the following informations */
+struct rx_provdisc_req_info {	/* When peer device issue prov_disc_req first, we should store the following information */
 	u8			peerDevAddr[ETH_ALEN];		/*	Peer device address */
 	u8 			strconfig_method_desc_of_prov_disc_req[4];	/* 	description for the config method located in the provisioning discovery request frame. */
 																	/* 	The UI must know this information to know which config method the remote p2p device is requiring. */
@@ -177,7 +177,7 @@ struct rx_provdisc_req_info {	/* When peer device issue prov_disc_req first, we
 struct tx_nego_req_info {
 	u16 				peer_channel_num[2];		/* 	The channel number which the receiver stands. */
 	u8			peerDevAddr[ETH_ALEN];		/*	Peer device address */
-	u8 			benable;					/* 	This negoitation request frame is trigger to send or not */
+	u8 			benable;					/* 	This negotiation request frame is trigger to send or not */
 };
 
 struct group_id_info {
@@ -228,9 +228,9 @@ struct wifidirect_info {
 	u8 				profileindex;	/* 	Used to point to the index of profileinfo array */
 	u8 				peer_operating_ch;
 	u8 				find_phase_state_exchange_cnt;
-	u16 					device_password_id_for_nego;	/* 	The device password ID for group negotation */
+	u16 					device_password_id_for_nego;	/* 	The device password ID for group negotiation */
 	u8 				negotiation_dialog_token;
-	u8				nego_ssid[WLAN_SSID_MAXLEN];	/*	SSID information for group negotitation */
+	u8				nego_ssid[WLAN_SSID_MAXLEN];	/*	SSID information for group negotiation */
 	u8 				nego_ssidlen;
 	u8 				p2p_group_ssid[WLAN_SSID_MAXLEN];
 	u8 				p2p_group_ssid_len;
diff --git a/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h b/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
index 393eeecaf3a0..5e6cf63956b8 100644
--- a/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
+++ b/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
@@ -112,7 +112,7 @@ extern unsigned char WMM_PARA_OUI[];
 /*  Note: */
 /* 	We just add new channel plan when the new channel plan is different from any of the following */
 /* 	channel plan. */
-/* 	If you just wnat to customize the acitions(scan period or join actions) about one of the channel plan, */
+/* 	If you just want to customize the actions(scan period or join actions) about one of the channel plan, */
 /* 	customize them in rt_channel_info in the RT_CHANNEL_LIST. */
 /*  */
 enum {
diff --git a/drivers/staging/rtl8723bs/include/rtw_mp.h b/drivers/staging/rtl8723bs/include/rtw_mp.h
index 26dec21bf0f1..2788ad80b114 100644
--- a/drivers/staging/rtl8723bs/include/rtw_mp.h
+++ b/drivers/staging/rtl8723bs/include/rtw_mp.h
@@ -101,7 +101,7 @@ struct mpt_context {
 	/*  For MP Tx Power index */
 	u8 	TxPwrLevel[2];	/*  rf-A, rf-B */
 	u32 		RegTxPwrLimit;
-	/*  Content of RCR Regsiter for Mass Production Test. */
+	/*  Content of RCR Register for Mass Production Test. */
 	u32 		MptRCR;
 	/*  true if we only receive packets with specific pattern. */
 	bool			bMptFilterPattern;
diff --git a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
index 5450d20b44a6..0a48f1653be5 100644
--- a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
+++ b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
@@ -112,7 +112,7 @@ enum rt_rf_power_state {
 #define	RT_RF_OFF_LEVL_FREE_FW		BIT(4)	/*  FW free, re-download the FW */
 #define	RT_RF_OFF_LEVL_FW_32K		BIT(5)	/*  FW in 32k */
 #define	RT_RF_PS_LEVEL_ALWAYS_ASPM	BIT(6)	/*  Always enable ASPM and Clock Req in initialization. */
-#define	RT_RF_LPS_DISALBE_2R			BIT(30)	/*  When LPS is on, disable 2R if no packet is received or transmittd. */
+#define	RT_RF_LPS_DISALBE_2R			BIT(30)	/*  When LPS is on, disable 2R if no packet is received or transmitted. */
 #define	RT_RF_LPS_LEVEL_ASPM			BIT(31)	/*  LPS with ASPM */
 
 #define	RT_IN_PS_LEVEL(ppsc, _PS_FLAG)		((ppsc->cur_ps_level & _PS_FLAG) ? true : false)
diff --git a/drivers/staging/rtl8723bs/include/rtw_recv.h b/drivers/staging/rtl8723bs/include/rtw_recv.h
index d007f90d02c3..9c3cdcc990fa 100644
--- a/drivers/staging/rtl8723bs/include/rtw_recv.h
+++ b/drivers/staging/rtl8723bs/include/rtw_recv.h
@@ -132,7 +132,7 @@ struct rx_pkt_attrib	{
 	u8 order;
 	u8 privacy; /* in frame_ctrl field */
 	u8 bdecrypted;
-	u8 encrypt; /* when 0 indicate no encrypt. when non-zero, indicate the encrypt algorith */
+	u8 encrypt; /* when 0 indicates no encryption; when non-zero, indicates the encryption algorithm */
 	u8 iv_len;
 	u8 icv_len;
 	u8 crc_err;
@@ -227,7 +227,7 @@ struct recv_priv {
 
 	struct __queue	recv_buf_pending_queue;
 
-	/* For display the phy informatiom */
+	/* For display the phy information */
 	u8 is_signal_dbg;	/*  for debug */
 	u8 signal_strength_dbg;	/*  for debug */
 
diff --git a/drivers/staging/rtl8723bs/include/rtw_xmit.h b/drivers/staging/rtl8723bs/include/rtw_xmit.h
index 73d020cfd0d1..e45753d17313 100644
--- a/drivers/staging/rtl8723bs/include/rtw_xmit.h
+++ b/drivers/staging/rtl8723bs/include/rtw_xmit.h
@@ -142,7 +142,7 @@ struct pkt_attrib {
 	u32 pktlen;		/* the original 802.3 pkt raw_data len (not include ether_hdr data) */
 	u32 last_txcmdsz;
 	u8 nr_frags;
-	u8 encrypt;	/* when 0 indicate no encrypt. when non-zero, indicate the encrypt algorith */
+	u8 encrypt;	/* when 0 indicates no encryption; when non-zero, indicates the encryption algorithm */
 	u8 iv_len;
 	u8 icv_len;
 	u8 iv[18];
diff --git a/drivers/staging/rtl8723bs/include/sta_info.h b/drivers/staging/rtl8723bs/include/sta_info.h
index abde3e3df988..69c377eeeaf0 100644
--- a/drivers/staging/rtl8723bs/include/sta_info.h
+++ b/drivers/staging/rtl8723bs/include/sta_info.h
@@ -329,7 +329,7 @@ struct	sta_priv {
 	 */
 	struct sta_info *sta_aid[NUM_STA];
 
-	u16 sta_dz_bitmap;/* only support 15 stations, staion aid bitmap for sleeping sta. */
+	u16 sta_dz_bitmap;/* only support for 15 stations, aid bitmap for sleeping stations. */
 	u16 tim_bitmap;/* only support 15 stations, aid = 0~15 mapping bit0~bit15 */
 
 	u16 max_num_sta;
diff --git a/drivers/staging/rtl8723bs/include/wifi.h b/drivers/staging/rtl8723bs/include/wifi.h
index 69e714a6d87c..036cf57c65a9 100644
--- a/drivers/staging/rtl8723bs/include/wifi.h
+++ b/drivers/staging/rtl8723bs/include/wifi.h
@@ -678,7 +678,7 @@ struct ADDBA_request {
 
 #define	P2P_PROVISION_TIMEOUT				5000	/* 	5 seconds timeout for sending the provision discovery request */
 #define	P2P_CONCURRENT_PROVISION_TIMEOUT	3000	/* 	3 seconds timeout for sending the provision discovery request under concurrent mode */
-#define	P2P_GO_NEGO_TIMEOUT					5000	/* 	5 seconds timeout for receiving the group negotation response */
+#define	P2P_GO_NEGO_TIMEOUT					5000	/* 	5 seconds timeout for receiving the group negotiation response */
 #define	P2P_CONCURRENT_GO_NEGO_TIMEOUT		3000	/* 	3 seconds timeout for sending the negotiation request under concurrent mode */
 #define	P2P_TX_PRESCAN_TIMEOUT				100		/* 	100ms */
 #define	P2P_INVITE_TIMEOUT					5000	/* 	5 seconds timeout for sending the invitation request */
-- 
2.31.1

[Outreachy kernel] [PATCH 3/4] staging: rtl8723bs: core: Remove an unused variable

[Fabio M. De Francesco]

Delete local variable "u8 sec_idx" because it is declared and
initialised, but never used.

Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 drivers/staging/rtl8723bs/core/rtw_ieee80211.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
index 2af66a18200d..3fd8a4261ea2 100644
--- a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
+++ b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
@@ -628,7 +628,7 @@ int rtw_get_wapi_ie(u8 *in_ie, uint in_len, u8 *wapi_ie, u16 *wapi_len)
 
 void rtw_get_sec_ie(u8 *in_ie, uint in_len, u8 *rsn_ie, u16 *rsn_len, u8 *wpa_ie, u16 *wpa_len)
 {
-	u8 authmode, sec_idx;
+	u8 authmode;
 	u8 wpa_oui[4] = {0x0, 0x50, 0xf2, 0x01};
 	uint	cnt;
 
@@ -636,8 +636,6 @@ void rtw_get_sec_ie(u8 *in_ie, uint in_len, u8 *rsn_ie, u16 *rsn_len, u8 *wpa_ie
 
 	cnt = (_TIMESTAMP_ + _BEACON_ITERVAL_ + _CAPABILITY_);
 
-	sec_idx = 0;
-
 	while (cnt < in_len) {
 		authmode = in_ie[cnt];
 
-- 
2.31.1

[Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Fabio M. De Francesco]

Change the type of fw_current_in_ps_mode from u8 to bool, because
it is used everywhere as a bool and, accordingly, it should be
declared as a bool. Shorten the controlling
expression of an 'if' statement.

Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 drivers/staging/rtl8723bs/hal/hal_intf.c        | 2 +-
 drivers/staging/rtl8723bs/include/rtw_pwrctrl.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8723bs/hal/hal_intf.c b/drivers/staging/rtl8723bs/hal/hal_intf.c
index 96fe172ced8d..8dc4dd8c6d4c 100644
--- a/drivers/staging/rtl8723bs/hal/hal_intf.c
+++ b/drivers/staging/rtl8723bs/hal/hal_intf.c
@@ -348,7 +348,7 @@ void rtw_hal_dm_watchdog(struct adapter *padapter)
 
 void rtw_hal_dm_watchdog_in_lps(struct adapter *padapter)
 {
-	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode == true) {
+	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode) {
 		if (padapter->HalFunc.hal_dm_watchdog_in_lps)
 			padapter->HalFunc.hal_dm_watchdog_in_lps(padapter); /* this function caller is in interrupt context */
 	}
diff --git a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
index 0a48f1653be5..0767dbb84199 100644
--- a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
+++ b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
@@ -203,7 +203,7 @@ struct pwrctrl_priv {
 	u8 LpsIdleCount;
 	u8 power_mgnt;
 	u8 org_power_mgnt;
-	u8 fw_current_in_ps_mode;
+	bool fw_current_in_ps_mode;
 	unsigned long	DelayLPSLastTimeStamp;
 	s32		pnp_current_pwr_state;
 	u8 pnp_bstop_trx;
-- 
2.31.1

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Greg KH]

On Sat, Apr 10, 2021 at 11:22:32AM +0200, Fabio M. De Francesco wrote:
> Change the type of fw_current_in_ps_mode from u8 to bool, because
> it is used everywhere as a bool and, accordingly, it should be
> declared as a bool. Shorten the controlling
> expression of an 'if' statement.
> 
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>  drivers/staging/rtl8723bs/hal/hal_intf.c        | 2 +-
>  drivers/staging/rtl8723bs/include/rtw_pwrctrl.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/hal/hal_intf.c b/drivers/staging/rtl8723bs/hal/hal_intf.c
> index 96fe172ced8d..8dc4dd8c6d4c 100644
> --- a/drivers/staging/rtl8723bs/hal/hal_intf.c
> +++ b/drivers/staging/rtl8723bs/hal/hal_intf.c
> @@ -348,7 +348,7 @@ void rtw_hal_dm_watchdog(struct adapter *padapter)
>  
>  void rtw_hal_dm_watchdog_in_lps(struct adapter *padapter)
>  {
> -	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode == true) {
> +	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode) {
>  		if (padapter->HalFunc.hal_dm_watchdog_in_lps)
>  			padapter->HalFunc.hal_dm_watchdog_in_lps(padapter); /* this function caller is in interrupt context */
>  	}
> diff --git a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> index 0a48f1653be5..0767dbb84199 100644
> --- a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> +++ b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> @@ -203,7 +203,7 @@ struct pwrctrl_priv {
>  	u8 LpsIdleCount;
>  	u8 power_mgnt;
>  	u8 org_power_mgnt;
> -	u8 fw_current_in_ps_mode;
> +	bool fw_current_in_ps_mode;
>  	unsigned long	DelayLPSLastTimeStamp;
>  	s32		pnp_current_pwr_state;
>  	u8 pnp_bstop_trx;

If this is only checked, how can it ever be true?  Who ever sets this
value?

thanks,

greg k-h

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Fabio M. De Francesco]

On Saturday, April 10, 2021 11:31:16 AM CEST Greg KH wrote:
> On Sat, Apr 10, 2021 at 11:22:32AM +0200, Fabio M. De Francesco wrote:
> > Change the type of fw_current_in_ps_mode from u8 to bool, because
> > it is used everywhere as a bool and, accordingly, it should be
> > declared as a bool. Shorten the controlling
> > expression of an 'if' statement.
> > 
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> > 
> >  drivers/staging/rtl8723bs/hal/hal_intf.c        | 2 +-
> >  drivers/staging/rtl8723bs/include/rtw_pwrctrl.h | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/rtl8723bs/hal/hal_intf.c
> > b/drivers/staging/rtl8723bs/hal/hal_intf.c index
> > 96fe172ced8d..8dc4dd8c6d4c 100644
> > --- a/drivers/staging/rtl8723bs/hal/hal_intf.c
> > +++ b/drivers/staging/rtl8723bs/hal/hal_intf.c
> > @@ -348,7 +348,7 @@ void rtw_hal_dm_watchdog(struct adapter *padapter)
> > 
> >  void rtw_hal_dm_watchdog_in_lps(struct adapter *padapter)
> >  {
> > 
> > -	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode == true) 
{
> > +	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode) {
> > 
> >  		if (padapter->HalFunc.hal_dm_watchdog_in_lps)
> >  		
> >  			padapter-
>HalFunc.hal_dm_watchdog_in_lps(padapter); /* this
> >  			function caller is in interrupt context 
*/>  	
> >  	}
> > 
> > diff --git a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h index
> > 0a48f1653be5..0767dbb84199 100644
> > --- a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > +++ b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > @@ -203,7 +203,7 @@ struct pwrctrl_priv {
> > 
> >  	u8 LpsIdleCount;
> >  	u8 power_mgnt;
> >  	u8 org_power_mgnt;
> > 
> > -	u8 fw_current_in_ps_mode;
> > +	bool fw_current_in_ps_mode;
> > 
> >  	unsigned long	DelayLPSLastTimeStamp;
> >  	s32		pnp_current_pwr_state;
> >  	u8 pnp_bstop_trx;
> 
> If this is only checked, how can it ever be true?  Who ever sets this
> value?
>
You're right. It is not set, therefore the "if" control expression cannot 
ever be "true".

Can I delete this statement in a new patch? Or you prefer I send the whole 
series again with this change in patch 4/4?

Thanks,

Fabio
>
> thanks,
> 
> greg k-h

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Greg KH]

On Sat, Apr 10, 2021 at 11:56:48AM +0200, Fabio M. De Francesco wrote:
> On Saturday, April 10, 2021 11:31:16 AM CEST Greg KH wrote:
> > On Sat, Apr 10, 2021 at 11:22:32AM +0200, Fabio M. De Francesco wrote:
> > > Change the type of fw_current_in_ps_mode from u8 to bool, because
> > > it is used everywhere as a bool and, accordingly, it should be
> > > declared as a bool. Shorten the controlling
> > > expression of an 'if' statement.
> > > 
> > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > ---
> > > 
> > >  drivers/staging/rtl8723bs/hal/hal_intf.c        | 2 +-
> > >  drivers/staging/rtl8723bs/include/rtw_pwrctrl.h | 2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > b/drivers/staging/rtl8723bs/hal/hal_intf.c index
> > > 96fe172ced8d..8dc4dd8c6d4c 100644
> > > --- a/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > +++ b/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > @@ -348,7 +348,7 @@ void rtw_hal_dm_watchdog(struct adapter *padapter)
> > > 
> > >  void rtw_hal_dm_watchdog_in_lps(struct adapter *padapter)
> > >  {
> > > 
> > > -	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode == true) 
> {
> > > +	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode) {
> > > 
> > >  		if (padapter->HalFunc.hal_dm_watchdog_in_lps)
> > >  		
> > >  			padapter-
> >HalFunc.hal_dm_watchdog_in_lps(padapter); /* this
> > >  			function caller is in interrupt context 
> */>  	
> > >  	}
> > > 
> > > diff --git a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h index
> > > 0a48f1653be5..0767dbb84199 100644
> > > --- a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > +++ b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > @@ -203,7 +203,7 @@ struct pwrctrl_priv {
> > > 
> > >  	u8 LpsIdleCount;
> > >  	u8 power_mgnt;
> > >  	u8 org_power_mgnt;
> > > 
> > > -	u8 fw_current_in_ps_mode;
> > > +	bool fw_current_in_ps_mode;
> > > 
> > >  	unsigned long	DelayLPSLastTimeStamp;
> > >  	s32		pnp_current_pwr_state;
> > >  	u8 pnp_bstop_trx;
> > 
> > If this is only checked, how can it ever be true?  Who ever sets this
> > value?
> >
> You're right. It is not set, therefore the "if" control expression cannot 
> ever be "true".
> 
> Can I delete this statement in a new patch? Or you prefer I send the whole 
> series again with this change in patch 4/4?

Just delete the variable from the structure entirely and when it is
used.

thanks,

greg k-h

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Fabio M. De Francesco]

On Saturday, April 10, 2021 12:31:27 PM CEST Greg KH wrote:
> On Sat, Apr 10, 2021 at 11:56:48AM +0200, Fabio M. De Francesco wrote:
> > On Saturday, April 10, 2021 11:31:16 AM CEST Greg KH wrote:
> > > On Sat, Apr 10, 2021 at 11:22:32AM +0200, Fabio M. De Francesco 
wrote:
> > > > Change the type of fw_current_in_ps_mode from u8 to bool, because
> > > > it is used everywhere as a bool and, accordingly, it should be
> > > > declared as a bool. Shorten the controlling
> > > > expression of an 'if' statement.
> > > > 
> > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > > ---
> > > > 
> > > >  drivers/staging/rtl8723bs/hal/hal_intf.c        | 2 +-
> > > >  drivers/staging/rtl8723bs/include/rtw_pwrctrl.h | 2 +-
> > > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > > b/drivers/staging/rtl8723bs/hal/hal_intf.c index
> > > > 96fe172ced8d..8dc4dd8c6d4c 100644
> > > > --- a/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > > +++ b/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > > @@ -348,7 +348,7 @@ void rtw_hal_dm_watchdog(struct adapter
> > > > *padapter)
> > > > 
> > > >  void rtw_hal_dm_watchdog_in_lps(struct adapter *padapter)
> > > >  {
> > > > 
> > > > -	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode == 
true)
> > 
> > {
> > 
> > > > +	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode) {
> > > > 
> > > >  		if (padapter->HalFunc.hal_dm_watchdog_in_lps)
> > > >  		
> > > >  			padapter-
> > >
> > >HalFunc.hal_dm_watchdog_in_lps(padapter); /* this
> > >
> > > >  			function caller is in interrupt context
> > 
> > */>
> > 
> > > >  	}
> > > > 
> > > > diff --git a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > > b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h index
> > > > 0a48f1653be5..0767dbb84199 100644
> > > > --- a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > > +++ b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > > @@ -203,7 +203,7 @@ struct pwrctrl_priv {
> > > > 
> > > >  	u8 LpsIdleCount;
> > > >  	u8 power_mgnt;
> > > >  	u8 org_power_mgnt;
> > > > 
> > > > -	u8 fw_current_in_ps_mode;
> > > > +	bool fw_current_in_ps_mode;
> > > > 
> > > >  	unsigned long	DelayLPSLastTimeStamp;
> > > >  	s32		pnp_current_pwr_state;
> > > >  	u8 pnp_bstop_trx;
> > > 
> > > If this is only checked, how can it ever be true?  Who ever sets this
> > > value?
> > 
> > You're right. It is not set, therefore the "if" control expression
> > cannot ever be "true".
> > 
> > Can I delete this statement in a new patch? Or you prefer I send the
> > whole series again with this change in patch 4/4?
> 
> Just delete the variable from the structure entirely and when it is
> used.
>
I've read the code of the function whom that 'if' statement belongs to. 
This function takes a pointer whose name is 'padapter' and this is has 
global scope. I think that since fw_current_in_ps_mode is dereferenced by 
the function adapter_to_pwrctl(padapter) it can and is indeed initialized 
and modified in some other files of the driver.

That's why I'll leave the if statement as is now. If I am wrong there's 
time to change it later in a future patch.

Thanks for your kind review,

Fabio
>
> 
> thanks,
> 
> greg k-h

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Greg KH]

On Sat, Apr 10, 2021 at 12:58:06PM +0200, Fabio M. De Francesco wrote:
> On Saturday, April 10, 2021 12:31:27 PM CEST Greg KH wrote:
> > On Sat, Apr 10, 2021 at 11:56:48AM +0200, Fabio M. De Francesco wrote:
> > > On Saturday, April 10, 2021 11:31:16 AM CEST Greg KH wrote:
> > > > On Sat, Apr 10, 2021 at 11:22:32AM +0200, Fabio M. De Francesco 
> wrote:
> > > > > Change the type of fw_current_in_ps_mode from u8 to bool, because
> > > > > it is used everywhere as a bool and, accordingly, it should be
> > > > > declared as a bool. Shorten the controlling
> > > > > expression of an 'if' statement.
> > > > > 
> > > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > > > ---
> > > > > 
> > > > >  drivers/staging/rtl8723bs/hal/hal_intf.c        | 2 +-
> > > > >  drivers/staging/rtl8723bs/include/rtw_pwrctrl.h | 2 +-
> > > > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > > > b/drivers/staging/rtl8723bs/hal/hal_intf.c index
> > > > > 96fe172ced8d..8dc4dd8c6d4c 100644
> > > > > --- a/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > > > +++ b/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > > > @@ -348,7 +348,7 @@ void rtw_hal_dm_watchdog(struct adapter
> > > > > *padapter)
> > > > > 
> > > > >  void rtw_hal_dm_watchdog_in_lps(struct adapter *padapter)
> > > > >  {
> > > > > 
> > > > > -	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode == 
> true)
> > > 
> > > {
> > > 
> > > > > +	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode) {
> > > > > 
> > > > >  		if (padapter->HalFunc.hal_dm_watchdog_in_lps)
> > > > >  		
> > > > >  			padapter-
> > > >
> > > >HalFunc.hal_dm_watchdog_in_lps(padapter); /* this
> > > >
> > > > >  			function caller is in interrupt context
> > > 
> > > */>
> > > 
> > > > >  	}
> > > > > 
> > > > > diff --git a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > > > b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h index
> > > > > 0a48f1653be5..0767dbb84199 100644
> > > > > --- a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > > > +++ b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > > > @@ -203,7 +203,7 @@ struct pwrctrl_priv {
> > > > > 
> > > > >  	u8 LpsIdleCount;
> > > > >  	u8 power_mgnt;
> > > > >  	u8 org_power_mgnt;
> > > > > 
> > > > > -	u8 fw_current_in_ps_mode;
> > > > > +	bool fw_current_in_ps_mode;
> > > > > 
> > > > >  	unsigned long	DelayLPSLastTimeStamp;
> > > > >  	s32		pnp_current_pwr_state;
> > > > >  	u8 pnp_bstop_trx;
> > > > 
> > > > If this is only checked, how can it ever be true?  Who ever sets this
> > > > value?
> > > 
> > > You're right. It is not set, therefore the "if" control expression
> > > cannot ever be "true".
> > > 
> > > Can I delete this statement in a new patch? Or you prefer I send the
> > > whole series again with this change in patch 4/4?
> > 
> > Just delete the variable from the structure entirely and when it is
> > used.
> >
> I've read the code of the function whom that 'if' statement belongs to. 
> This function takes a pointer whose name is 'padapter' and this is has 
> global scope. I think that since fw_current_in_ps_mode is dereferenced by 
> the function adapter_to_pwrctl(padapter) it can and is indeed initialized 
> and modified in some other files of the driver.

Where does that happen, and why did the build not break when you changed
the variable name?  Is the whole variable assigned to a specific
location in memory in the device?  Where is it initialized?

> That's why I'll leave the if statement as is now. If I am wrong there's 
> time to change it later in a future patch.

Don't change obviously wrong code, we can clean it up properly :)

thanks,

greg k-h

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Fabio M. De Francesco]

On Saturday, April 10, 2021 1:03:34 PM CEST Greg KH wrote:
> On Sat, Apr 10, 2021 at 12:58:06PM +0200, Fabio M. De Francesco wrote:
> > On Saturday, April 10, 2021 12:31:27 PM CEST Greg KH wrote:
> > > On Sat, Apr 10, 2021 at 11:56:48AM +0200, Fabio M. De Francesco 
wrote:
> > > > On Saturday, April 10, 2021 11:31:16 AM CEST Greg KH wrote:
> > > > > On Sat, Apr 10, 2021 at 11:22:32AM +0200, Fabio M. De Francesco
> > 
> > wrote:
> > > > > > Change the type of fw_current_in_ps_mode from u8 to bool,
> > > > > > because
> > > > > > it is used everywhere as a bool and, accordingly, it should be
> > > > > > declared as a bool. Shorten the controlling
> > > > > > expression of an 'if' statement.
> > > > > > 
> > > > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > > > > ---
> > > > > > 
> > > > > >  drivers/staging/rtl8723bs/hal/hal_intf.c        | 2 +-
> > > > > >  drivers/staging/rtl8723bs/include/rtw_pwrctrl.h | 2 +-
> > > > > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > > > > b/drivers/staging/rtl8723bs/hal/hal_intf.c index
> > > > > > 96fe172ced8d..8dc4dd8c6d4c 100644
> > > > > > --- a/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > > > > +++ b/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > > > > @@ -348,7 +348,7 @@ void rtw_hal_dm_watchdog(struct adapter
> > > > > > *padapter)
> > > > > > 
> > > > > >  void rtw_hal_dm_watchdog_in_lps(struct adapter *padapter)
> > > > > >  {
> > > > > > 
> > > > > > -	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode ==
> > 
> > true)
> > 
> > > > {
> > > > 
> > > > > > +	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode) {
> > > > > > 
> > > > > >  		if (padapter->HalFunc.hal_dm_watchdog_in_lps)
> > > > > >  		
> > > > > >  			padapter-
> > > > >
> > > > >HalFunc.hal_dm_watchdog_in_lps(padapter); /* this
> > > > >
> > > > > >  			function caller is in interrupt 
context
> > > > 
> > > > */>
> > > > 
> > > > > >  	}
> > > > > > 
> > > > > > diff --git a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > > > > b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h index
> > > > > > 0a48f1653be5..0767dbb84199 100644
> > > > > > --- a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > > > > +++ b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > > > > @@ -203,7 +203,7 @@ struct pwrctrl_priv {
> > > > > > 
> > > > > >  	u8 LpsIdleCount;
> > > > > >  	u8 power_mgnt;
> > > > > >  	u8 org_power_mgnt;
> > > > > > 
> > > > > > -	u8 fw_current_in_ps_mode;
> > > > > > +	bool fw_current_in_ps_mode;
> > > > > > 
> > > > > >  	unsigned long	DelayLPSLastTimeStamp;
> > > > > >  	s32		pnp_current_pwr_state;
> > > > > >  	u8 pnp_bstop_trx;
> > > > > 
> > > > > If this is only checked, how can it ever be true?  Who ever sets
> > > > > this
> > > > > value?
> > > > 
> > > > You're right. It is not set, therefore the "if" control expression
> > > > cannot ever be "true".
> > > > 
> > > > Can I delete this statement in a new patch? Or you prefer I send
> > > > the
> > > > whole series again with this change in patch 4/4?
> > > 
> > > Just delete the variable from the structure entirely and when it is
> > > used.
> > 
> > I've read the code of the function whom that 'if' statement belongs to.
> > This function takes a pointer whose name is 'padapter' and this is has
> > global scope. I think that since fw_current_in_ps_mode is dereferenced
> > by the function adapter_to_pwrctl(padapter) it can and is indeed
> > initialized and modified in some other files of the driver.
> 
> Where does that happen, and why did the build not break when you changed
> the variable name?  
>
The build didn't break because I changed the name of that variable 
everywhere it is used in the driver. This patch is against all the affected 
files of each subdirectory of staging/rtl8723bs.
>
> Is the whole variable assigned to a specific
> location in memory in the device?  Where is it initialized?
>
That variable has global scope and is assigned at least in:

drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:368:                       
pwrpriv->fw_current_in_ps_mode = false;

drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:380:                       
pwrpriv->fw_current_in_ps_mode = true;

drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c:433:  
adapter_to_pwrctl(padapter)->fw_current_in_ps_mode = false;

drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:981:       
pwrctrlpriv->fw_current_in_ps_mode = false;

Thanks,

Fabio
> 
> > That's why I'll leave the if statement as is now. If I am wrong there's
> > time to change it later in a future patch.
> 
> Don't change obviously wrong code, we can clean it up properly :)
> 
> thanks,
> 
> greg k-h

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Greg KH]

On Sat, Apr 10, 2021 at 01:23:21PM +0200, Fabio M. De Francesco wrote:
> On Saturday, April 10, 2021 1:03:34 PM CEST Greg KH wrote:
> > On Sat, Apr 10, 2021 at 12:58:06PM +0200, Fabio M. De Francesco wrote:
> > > On Saturday, April 10, 2021 12:31:27 PM CEST Greg KH wrote:
> > > > On Sat, Apr 10, 2021 at 11:56:48AM +0200, Fabio M. De Francesco 
> wrote:
> > > > > On Saturday, April 10, 2021 11:31:16 AM CEST Greg KH wrote:
> > > > > > On Sat, Apr 10, 2021 at 11:22:32AM +0200, Fabio M. De Francesco
> > > 
> > > wrote:
> > > > > > > Change the type of fw_current_in_ps_mode from u8 to bool,
> > > > > > > because
> > > > > > > it is used everywhere as a bool and, accordingly, it should be
> > > > > > > declared as a bool. Shorten the controlling
> > > > > > > expression of an 'if' statement.
> > > > > > > 
> > > > > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > > > > > ---
> > > > > > > 
> > > > > > >  drivers/staging/rtl8723bs/hal/hal_intf.c        | 2 +-
> > > > > > >  drivers/staging/rtl8723bs/include/rtw_pwrctrl.h | 2 +-
> > > > > > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > > > > > b/drivers/staging/rtl8723bs/hal/hal_intf.c index
> > > > > > > 96fe172ced8d..8dc4dd8c6d4c 100644
> > > > > > > --- a/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > > > > > +++ b/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > > > > > @@ -348,7 +348,7 @@ void rtw_hal_dm_watchdog(struct adapter
> > > > > > > *padapter)
> > > > > > > 
> > > > > > >  void rtw_hal_dm_watchdog_in_lps(struct adapter *padapter)
> > > > > > >  {
> > > > > > > 
> > > > > > > -	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode ==
> > > 
> > > true)
> > > 
> > > > > {
> > > > > 
> > > > > > > +	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode) {
> > > > > > > 
> > > > > > >  		if (padapter->HalFunc.hal_dm_watchdog_in_lps)
> > > > > > >  		
> > > > > > >  			padapter-
> > > > > >
> > > > > >HalFunc.hal_dm_watchdog_in_lps(padapter); /* this
> > > > > >
> > > > > > >  			function caller is in interrupt 
> context
> > > > > 
> > > > > */>
> > > > > 
> > > > > > >  	}
> > > > > > > 
> > > > > > > diff --git a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > > > > > b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h index
> > > > > > > 0a48f1653be5..0767dbb84199 100644
> > > > > > > --- a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > > > > > +++ b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > > > > > @@ -203,7 +203,7 @@ struct pwrctrl_priv {
> > > > > > > 
> > > > > > >  	u8 LpsIdleCount;
> > > > > > >  	u8 power_mgnt;
> > > > > > >  	u8 org_power_mgnt;
> > > > > > > 
> > > > > > > -	u8 fw_current_in_ps_mode;
> > > > > > > +	bool fw_current_in_ps_mode;
> > > > > > > 
> > > > > > >  	unsigned long	DelayLPSLastTimeStamp;
> > > > > > >  	s32		pnp_current_pwr_state;
> > > > > > >  	u8 pnp_bstop_trx;
> > > > > > 
> > > > > > If this is only checked, how can it ever be true?  Who ever sets
> > > > > > this
> > > > > > value?
> > > > > 
> > > > > You're right. It is not set, therefore the "if" control expression
> > > > > cannot ever be "true".
> > > > > 
> > > > > Can I delete this statement in a new patch? Or you prefer I send
> > > > > the
> > > > > whole series again with this change in patch 4/4?
> > > > 
> > > > Just delete the variable from the structure entirely and when it is
> > > > used.
> > > 
> > > I've read the code of the function whom that 'if' statement belongs to.
> > > This function takes a pointer whose name is 'padapter' and this is has
> > > global scope. I think that since fw_current_in_ps_mode is dereferenced
> > > by the function adapter_to_pwrctl(padapter) it can and is indeed
> > > initialized and modified in some other files of the driver.
> > 
> > Where does that happen, and why did the build not break when you changed
> > the variable name?  
> >
> The build didn't break because I changed the name of that variable 
> everywhere it is used in the driver. This patch is against all the affected 
> files of each subdirectory of staging/rtl8723bs.

Right, and as you only had to change it in one place, that tested if
that variable was set to a specific value, and there is no code that
actually sets the variable, it seems like that variable is not ever used
and can be removed.

> > Is the whole variable assigned to a specific
> > location in memory in the device?  Where is it initialized?
> >
> That variable has global scope and is assigned at least in:
> 
> drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:368:                       
> pwrpriv->fw_current_in_ps_mode = false;
> 
> drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:380:                       
> pwrpriv->fw_current_in_ps_mode = true;
> 
> drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c:433:  
> adapter_to_pwrctl(padapter)->fw_current_in_ps_mode = false;
> 
> drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:981:       
> pwrctrlpriv->fw_current_in_ps_mode = false;

But you are only changing 2 files in this patch:

> > > > > > >  drivers/staging/rtl8723bs/hal/hal_intf.c        | 2 +-
> > > > > > >  drivers/staging/rtl8723bs/include/rtw_pwrctrl.h | 2 +-

Ah, you are only changing the type, not the name.  That's the confusion
on my part.

Was the code setting a u8 to "true/false"?  Ugh, what a mess...

thanks,

greg k-h

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Julia Lawall]

> That variable has global scope and is assigned at least in:

What do you mean by global scope?  None of the following look like
references to global variables.

julia

>
> drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:368:
> pwrpriv->fw_current_in_ps_mode = false;
>
> drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:380:
> pwrpriv->fw_current_in_ps_mode = true;
>
> drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c:433:
> adapter_to_pwrctl(padapter)->fw_current_in_ps_mode = false;
>
> drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:981:
> pwrctrlpriv->fw_current_in_ps_mode = false;

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Julia Lawall]

> That variable has global scope and is assigned at least in:

What do you mean by global scope?  None of the following look like
references to global variables.

julia

>
> drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:368:
> pwrpriv->fw_current_in_ps_mode = false;
>
> drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:380:
> pwrpriv->fw_current_in_ps_mode = true;
>
> drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c:433:
> adapter_to_pwrctl(padapter)->fw_current_in_ps_mode = false;
>
> drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:981:
> pwrctrlpriv->fw_current_in_ps_mode = false;

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Fabio M. De Francesco]

On Saturday, April 10, 2021 1:37:30 PM CEST Julia Lawall wrote:
> > That variable has global scope and is assigned at least in:
> What do you mean by global scope?  None of the following look like
> references to global variables.
> 
> julia
I just mean that fw_current_in_ps_mode is a field of a struct in a .h file 
included everywhere in this driver and that the functions whom the 
following assignments belong to have not the "static" type modifier.

Thanks,

Fabio
> 
> > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:368:
> > pwrpriv->fw_current_in_ps_mode = false;
> > 
> > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:380:
> > pwrpriv->fw_current_in_ps_mode = true;
> > 
> > drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c:433:
> > adapter_to_pwrctl(padapter)->fw_current_in_ps_mode = false;
> > 
> > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:981:
> > pwrctrlpriv->fw_current_in_ps_mode = false;

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Julia Lawall]

On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:

> On Saturday, April 10, 2021 1:37:30 PM CEST Julia Lawall wrote:
> > > That variable has global scope and is assigned at least in:
> > What do you mean by global scope?  None of the following look like
> > references to global variables.
> >
> > julia
> I just mean that fw_current_in_ps_mode is a field of a struct in a .h file
> included everywhere in this driver and that the functions whom the
> following assignments belong to have not the "static" type modifier.

OK, but a field in a structure is not a variable, and this is not what
scope means.

int x;

outside of anything is a global variable (global scope).

int foo() {
  int x;
  ...
}

Here x is a local variable.  Its scope is the body of the function.

int foo() {
  if (abc) {
    int x;
    ...
  }
}

Here x is a local variable, but its scope is only in the if branch.

julia

>
> Thanks,
>
> Fabio
> >
> > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:368:
> > > pwrpriv->fw_current_in_ps_mode = false;
> > >
> > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:380:
> > > pwrpriv->fw_current_in_ps_mode = true;
> > >
> > > drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c:433:
> > > adapter_to_pwrctl(padapter)->fw_current_in_ps_mode = false;
> > >
> > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:981:
> > > pwrctrlpriv->fw_current_in_ps_mode = false;
>
>
>
>
>

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Julia Lawall]

On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:

> On Saturday, April 10, 2021 1:37:30 PM CEST Julia Lawall wrote:
> > > That variable has global scope and is assigned at least in:
> > What do you mean by global scope?  None of the following look like
> > references to global variables.
> >
> > julia
> I just mean that fw_current_in_ps_mode is a field of a struct in a .h file
> included everywhere in this driver and that the functions whom the
> following assignments belong to have not the "static" type modifier.

OK, but a field in a structure is not a variable, and this is not what
scope means.

int x;

outside of anything is a global variable (global scope).

int foo() {
  int x;
  ...
}

Here x is a local variable.  Its scope is the body of the function.

int foo() {
  if (abc) {
    int x;
    ...
  }
}

Here x is a local variable, but its scope is only in the if branch.

julia

>
> Thanks,
>
> Fabio
> >
> > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:368:
> > > pwrpriv->fw_current_in_ps_mode = false;
> > >
> > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:380:
> > > pwrpriv->fw_current_in_ps_mode = true;
> > >
> > > drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c:433:
> > > adapter_to_pwrctl(padapter)->fw_current_in_ps_mode = false;
> > >
> > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:981:
> > > pwrctrlpriv->fw_current_in_ps_mode = false;
>
>
>
>
>

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Fabio M. De Francesco]

On Saturday, April 10, 2021 2:12:28 PM CEST Julia Lawall wrote:
> On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:
> > On Saturday, April 10, 2021 1:37:30 PM CEST Julia Lawall wrote:
> > > > That variable has global scope and is assigned at least in:
> > > What do you mean by global scope?  None of the following look like
> > > references to global variables.
> > > 
> > > julia
> > 
> > I just mean that fw_current_in_ps_mode is a field of a struct in a .h
> > file included everywhere in this driver and that the functions whom
> > the following assignments belong to have not the "static" type
> > modifier.
> OK, but a field in a structure is not a variable, and this is not what
> scope means.
>
You're right, a field in a structure is not a variable.
> 
> int x;
> 
> outside of anything is a global variable (global scope).
> 
> int foo() {
>   int x;
>   ...
> }
> 
> Here x is a local variable.  Its scope is the body of the function.
> 
> int foo() {
>   if (abc) {
>     int x;
>     ...
>   }
> }
> 
> Here x is a local variable, but its scope is only in the if branch.
>
And you're right again: I needed a little refresh of my knowledge of C.

I've searched again in the code for the purpose of finding out if that 
struct is initialized with global scope but I didn't find anything. I 
didn't even find any dynamic allocation within functions that returns 
pointers to that struct.

Therefore, according to Greg's request, I'll delete that stupid 'if' 
statement in the patch series v2 that I'm about to submit.

I've really appreciated your help.

Thanks,

Fabio
> 
> julia
> 
> > Thanks,
> > 
> > Fabio
> > 
> > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:368:
> > > > pwrpriv->fw_current_in_ps_mode = false;
> > > > 
> > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:380:
> > > > pwrpriv->fw_current_in_ps_mode = true;
> > > > 
> > > > drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c:433:
> > > > adapter_to_pwrctl(padapter)->fw_current_in_ps_mode = false;
> > > > 
> > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:981:
> > > > pwrctrlpriv->fw_current_in_ps_mode = false;

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Julia Lawall]

On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:

> On Saturday, April 10, 2021 2:12:28 PM CEST Julia Lawall wrote:
> > On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:
> > > On Saturday, April 10, 2021 1:37:30 PM CEST Julia Lawall wrote:
> > > > > That variable has global scope and is assigned at least in:
> > > > What do you mean by global scope?  None of the following look like
> > > > references to global variables.
> > > >
> > > > julia
> > >
> > > I just mean that fw_current_in_ps_mode is a field of a struct in a .h
> > > file included everywhere in this driver and that the functions whom
> > > the following assignments belong to have not the "static" type
> > > modifier.
> > OK, but a field in a structure is not a variable, and this is not what
> > scope means.
> >
> You're right, a field in a structure is not a variable.
> >
> > int x;
> >
> > outside of anything is a global variable (global scope).
> >
> > int foo() {
> >   int x;
> >   ...
> > }
> >
> > Here x is a local variable.  Its scope is the body of the function.
> >
> > int foo() {
> >   if (abc) {
> >     int x;
> >     ...
> >   }
> > }
> >
> > Here x is a local variable, but its scope is only in the if branch.
> >
> And you're right again: I needed a little refresh of my knowledge of C.
>
> I've searched again in the code for the purpose of finding out if that
> struct is initialized with global scope but I didn't find anything. I
> didn't even find any dynamic allocation within functions that returns
> pointers to that struct.
>
> Therefore, according to Greg's request, I'll delete that stupid 'if'
> statement in the patch series v2 that I'm about to submit.

I'm really not clear on why the if should be deleted.

julia


>
> I've really appreciated your help.
>
> Thanks,
>
> Fabio
> >
> > julia
> >
> > > Thanks,
> > >
> > > Fabio
> > >
> > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:368:
> > > > > pwrpriv->fw_current_in_ps_mode = false;
> > > > >
> > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:380:
> > > > > pwrpriv->fw_current_in_ps_mode = true;
> > > > >
> > > > > drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c:433:
> > > > > adapter_to_pwrctl(padapter)->fw_current_in_ps_mode = false;
> > > > >
> > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:981:
> > > > > pwrctrlpriv->fw_current_in_ps_mode = false;
>
>
>
>
>

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Julia Lawall]

On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:

> On Saturday, April 10, 2021 2:12:28 PM CEST Julia Lawall wrote:
> > On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:
> > > On Saturday, April 10, 2021 1:37:30 PM CEST Julia Lawall wrote:
> > > > > That variable has global scope and is assigned at least in:
> > > > What do you mean by global scope?  None of the following look like
> > > > references to global variables.
> > > >
> > > > julia
> > >
> > > I just mean that fw_current_in_ps_mode is a field of a struct in a .h
> > > file included everywhere in this driver and that the functions whom
> > > the following assignments belong to have not the "static" type
> > > modifier.
> > OK, but a field in a structure is not a variable, and this is not what
> > scope means.
> >
> You're right, a field in a structure is not a variable.
> >
> > int x;
> >
> > outside of anything is a global variable (global scope).
> >
> > int foo() {
> >   int x;
> >   ...
> > }
> >
> > Here x is a local variable.  Its scope is the body of the function.
> >
> > int foo() {
> >   if (abc) {
> >     int x;
> >     ...
> >   }
> > }
> >
> > Here x is a local variable, but its scope is only in the if branch.
> >
> And you're right again: I needed a little refresh of my knowledge of C.
>
> I've searched again in the code for the purpose of finding out if that
> struct is initialized with global scope but I didn't find anything. I
> didn't even find any dynamic allocation within functions that returns
> pointers to that struct.
>
> Therefore, according to Greg's request, I'll delete that stupid 'if'
> statement in the patch series v2 that I'm about to submit.

I'm really not clear on why the if should be deleted.

julia


>
> I've really appreciated your help.
>
> Thanks,
>
> Fabio
> >
> > julia
> >
> > > Thanks,
> > >
> > > Fabio
> > >
> > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:368:
> > > > > pwrpriv->fw_current_in_ps_mode = false;
> > > > >
> > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:380:
> > > > > pwrpriv->fw_current_in_ps_mode = true;
> > > > >
> > > > > drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c:433:
> > > > > adapter_to_pwrctl(padapter)->fw_current_in_ps_mode = false;
> > > > >
> > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:981:
> > > > > pwrctrlpriv->fw_current_in_ps_mode = false;
>
>
>
>
>

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Fabio M. De Francesco]

On Saturday, April 10, 2021 3:24:43 PM CEST Julia Lawall wrote:
> On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:
> > On Saturday, April 10, 2021 2:12:28 PM CEST Julia Lawall wrote:
> > > On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:
> > > > On Saturday, April 10, 2021 1:37:30 PM CEST Julia Lawall wrote:
> > > > > > That variable has global scope and is assigned at least in:
> > > > > What do you mean by global scope?  None of the following look
> > > > > like
> > > > > references to global variables.
> > > > > 
> > > > > julia
> > > > 
> > > > I just mean that fw_current_in_ps_mode is a field of a struct in a
> > > > .h
> > > > file included everywhere in this driver and that the functions whom
> > > > the following assignments belong to have not the "static" type
> > > > modifier.
> > > 
> > > OK, but a field in a structure is not a variable, and this is not
> > > what
> > > scope means.
> > 
> > You're right, a field in a structure is not a variable.
> > 
> > > int x;
> > > 
> > > outside of anything is a global variable (global scope).
> > > 
> > > int foo() {
> > > 
> > >   int x;
> > >   ...
> > > 
> > > }
> > > 
> > > Here x is a local variable.  Its scope is the body of the function.
> > > 
> > > int foo() {
> > > 
> > >   if (abc) {
> > >   
> > >     int x;
> > >     ...
> > >   
> > >   }
> > > 
> > > }
> > > 
> > > Here x is a local variable, but its scope is only in the if branch.
> > 
> > And you're right again: I needed a little refresh of my knowledge of C.
> > 
> > I've searched again in the code for the purpose of finding out if that
> > struct is initialized with global scope but I didn't find anything. I
> > didn't even find any dynamic allocation within functions that returns
> > pointers to that struct.
> > 
> > Therefore, according to Greg's request, I'll delete that stupid 'if'
> > statement in the patch series v2 that I'm about to submit.
> 
> I'm really not clear on why the if should be deleted.
> 
> julia
>
I'm supposed to delete it because of the review made by Greg. In a couple 
of previous messages he wrote:

"If this is only checked, how can it ever be true?  Who ever sets this 
value?"

and then:

"Just delete the variable from the structure entirely and when it is
used.".

However, like you, I'm not sure yet.

Thanks,

Fabio
> 
> > I've really appreciated your help.
> > 
> > Thanks,
> > 
> > Fabio
> > 
> > > julia
> > > 
> > > > Thanks,
> > > > 
> > > > Fabio
> > > > 
> > > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:368:
> > > > > > pwrpriv->fw_current_in_ps_mode = false;
> > > > > > 
> > > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:380:
> > > > > > pwrpriv->fw_current_in_ps_mode = true;
> > > > > > 
> > > > > > drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c:433:
> > > > > > adapter_to_pwrctl(padapter)->fw_current_in_ps_mode = false;
> > > > > > 
> > > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:981:
> > > > > > pwrctrlpriv->fw_current_in_ps_mode = false;

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Greg KH]

On Sat, Apr 10, 2021 at 03:53:38PM +0200, Fabio M. De Francesco wrote:
> On Saturday, April 10, 2021 3:24:43 PM CEST Julia Lawall wrote:
> > On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:
> > > On Saturday, April 10, 2021 2:12:28 PM CEST Julia Lawall wrote:
> > > > On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:
> > > > > On Saturday, April 10, 2021 1:37:30 PM CEST Julia Lawall wrote:
> > > > > > > That variable has global scope and is assigned at least in:
> > > > > > What do you mean by global scope?  None of the following look
> > > > > > like
> > > > > > references to global variables.
> > > > > > 
> > > > > > julia
> > > > > 
> > > > > I just mean that fw_current_in_ps_mode is a field of a struct in a
> > > > > .h
> > > > > file included everywhere in this driver and that the functions whom
> > > > > the following assignments belong to have not the "static" type
> > > > > modifier.
> > > > 
> > > > OK, but a field in a structure is not a variable, and this is not
> > > > what
> > > > scope means.
> > > 
> > > You're right, a field in a structure is not a variable.
> > > 
> > > > int x;
> > > > 
> > > > outside of anything is a global variable (global scope).
> > > > 
> > > > int foo() {
> > > > 
> > > >   int x;
> > > >   ...
> > > > 
> > > > }
> > > > 
> > > > Here x is a local variable.  Its scope is the body of the function.
> > > > 
> > > > int foo() {
> > > > 
> > > >   if (abc) {
> > > >   
> > > >     int x;
> > > >     ...
> > > >   
> > > >   }
> > > > 
> > > > }
> > > > 
> > > > Here x is a local variable, but its scope is only in the if branch.
> > > 
> > > And you're right again: I needed a little refresh of my knowledge of C.
> > > 
> > > I've searched again in the code for the purpose of finding out if that
> > > struct is initialized with global scope but I didn't find anything. I
> > > didn't even find any dynamic allocation within functions that returns
> > > pointers to that struct.
> > > 
> > > Therefore, according to Greg's request, I'll delete that stupid 'if'
> > > statement in the patch series v2 that I'm about to submit.
> > 
> > I'm really not clear on why the if should be deleted.
> > 
> > julia
> >
> I'm supposed to delete it because of the review made by Greg. In a couple 
> of previous messages he wrote:
> 
> "If this is only checked, how can it ever be true?  Who ever sets this 
> value?"
> 
> and then:
> 
> "Just delete the variable from the structure entirely and when it is
> used.".
> 
> However, like you, I'm not sure yet.

I don't think any of us are, try fixing up what you think is right and
resend and we can go from there :)

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Julia Lawall]

On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:

> On Saturday, April 10, 2021 3:24:43 PM CEST Julia Lawall wrote:
> > On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:
> > > On Saturday, April 10, 2021 2:12:28 PM CEST Julia Lawall wrote:
> > > > On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:
> > > > > On Saturday, April 10, 2021 1:37:30 PM CEST Julia Lawall wrote:
> > > > > > > That variable has global scope and is assigned at least in:
> > > > > > What do you mean by global scope?  None of the following look
> > > > > > like
> > > > > > references to global variables.
> > > > > >
> > > > > > julia
> > > > >
> > > > > I just mean that fw_current_in_ps_mode is a field of a struct in a
> > > > > .h
> > > > > file included everywhere in this driver and that the functions whom
> > > > > the following assignments belong to have not the "static" type
> > > > > modifier.
> > > >
> > > > OK, but a field in a structure is not a variable, and this is not
> > > > what
> > > > scope means.
> > >
> > > You're right, a field in a structure is not a variable.
> > >
> > > > int x;
> > > >
> > > > outside of anything is a global variable (global scope).
> > > >
> > > > int foo() {
> > > >
> > > >   int x;
> > > >   ...
> > > >
> > > > }
> > > >
> > > > Here x is a local variable.  Its scope is the body of the function.
> > > >
> > > > int foo() {
> > > >
> > > >   if (abc) {
> > > >
> > > >     int x;
> > > >     ...
> > > >
> > > >   }
> > > >
> > > > }
> > > >
> > > > Here x is a local variable, but its scope is only in the if branch.
> > >
> > > And you're right again: I needed a little refresh of my knowledge of C.
> > >
> > > I've searched again in the code for the purpose of finding out if that
> > > struct is initialized with global scope but I didn't find anything. I
> > > didn't even find any dynamic allocation within functions that returns
> > > pointers to that struct.
> > >
> > > Therefore, according to Greg's request, I'll delete that stupid 'if'
> > > statement in the patch series v2 that I'm about to submit.
> >
> > I'm really not clear on why the if should be deleted.
> >
> > julia
> >
> I'm supposed to delete it because of the review made by Greg. In a couple
> of previous messages he wrote:
>
> "If this is only checked, how can it ever be true?  Who ever sets this
> value?"
>
> and then:
>
> "Just delete the variable from the structure entirely and when it is
> used.".
>
> However, like you, I'm not sure yet.

Be sure.  Greg already said that he misinterpreted the patch, because he
thought that the name also changed.

julia


>
> Thanks,
>
> Fabio
> >
> > > I've really appreciated your help.
> > >
> > > Thanks,
> > >
> > > Fabio
> > >
> > > > julia
> > > >
> > > > > Thanks,
> > > > >
> > > > > Fabio
> > > > >
> > > > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:368:
> > > > > > > pwrpriv->fw_current_in_ps_mode = false;
> > > > > > >
> > > > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:380:
> > > > > > > pwrpriv->fw_current_in_ps_mode = true;
> > > > > > >
> > > > > > > drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c:433:
> > > > > > > adapter_to_pwrctl(padapter)->fw_current_in_ps_mode = false;
> > > > > > >
> > > > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:981:
> > > > > > > pwrctrlpriv->fw_current_in_ps_mode = false;
>
>
>
>
>

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Julia Lawall]

On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:

> On Saturday, April 10, 2021 3:24:43 PM CEST Julia Lawall wrote:
> > On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:
> > > On Saturday, April 10, 2021 2:12:28 PM CEST Julia Lawall wrote:
> > > > On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:
> > > > > On Saturday, April 10, 2021 1:37:30 PM CEST Julia Lawall wrote:
> > > > > > > That variable has global scope and is assigned at least in:
> > > > > > What do you mean by global scope?  None of the following look
> > > > > > like
> > > > > > references to global variables.
> > > > > >
> > > > > > julia
> > > > >
> > > > > I just mean that fw_current_in_ps_mode is a field of a struct in a
> > > > > .h
> > > > > file included everywhere in this driver and that the functions whom
> > > > > the following assignments belong to have not the "static" type
> > > > > modifier.
> > > >
> > > > OK, but a field in a structure is not a variable, and this is not
> > > > what
> > > > scope means.
> > >
> > > You're right, a field in a structure is not a variable.
> > >
> > > > int x;
> > > >
> > > > outside of anything is a global variable (global scope).
> > > >
> > > > int foo() {
> > > >
> > > >   int x;
> > > >   ...
> > > >
> > > > }
> > > >
> > > > Here x is a local variable.  Its scope is the body of the function.
> > > >
> > > > int foo() {
> > > >
> > > >   if (abc) {
> > > >
> > > >     int x;
> > > >     ...
> > > >
> > > >   }
> > > >
> > > > }
> > > >
> > > > Here x is a local variable, but its scope is only in the if branch.
> > >
> > > And you're right again: I needed a little refresh of my knowledge of C.
> > >
> > > I've searched again in the code for the purpose of finding out if that
> > > struct is initialized with global scope but I didn't find anything. I
> > > didn't even find any dynamic allocation within functions that returns
> > > pointers to that struct.
> > >
> > > Therefore, according to Greg's request, I'll delete that stupid 'if'
> > > statement in the patch series v2 that I'm about to submit.
> >
> > I'm really not clear on why the if should be deleted.
> >
> > julia
> >
> I'm supposed to delete it because of the review made by Greg. In a couple
> of previous messages he wrote:
>
> "If this is only checked, how can it ever be true?  Who ever sets this
> value?"
>
> and then:
>
> "Just delete the variable from the structure entirely and when it is
> used.".
>
> However, like you, I'm not sure yet.

Be sure.  Greg already said that he misinterpreted the patch, because he
thought that the name also changed.

julia


>
> Thanks,
>
> Fabio
> >
> > > I've really appreciated your help.
> > >
> > > Thanks,
> > >
> > > Fabio
> > >
> > > > julia
> > > >
> > > > > Thanks,
> > > > >
> > > > > Fabio
> > > > >
> > > > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:368:
> > > > > > > pwrpriv->fw_current_in_ps_mode = false;
> > > > > > >
> > > > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:380:
> > > > > > > pwrpriv->fw_current_in_ps_mode = true;
> > > > > > >
> > > > > > > drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c:433:
> > > > > > > adapter_to_pwrctl(padapter)->fw_current_in_ps_mode = false;
> > > > > > >
> > > > > > > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c:981:
> > > > > > > pwrctrlpriv->fw_current_in_ps_mode = false;
>
>
>
>
>

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Julia Lawall]

On Sat, 10 Apr 2021, Greg KH wrote:

> On Sat, Apr 10, 2021 at 11:22:32AM +0200, Fabio M. De Francesco wrote:
> > Change the type of fw_current_in_ps_mode from u8 to bool, because
> > it is used everywhere as a bool and, accordingly, it should be
> > declared as a bool. Shorten the controlling
> > expression of an 'if' statement.
> >
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> >  drivers/staging/rtl8723bs/hal/hal_intf.c        | 2 +-
> >  drivers/staging/rtl8723bs/include/rtw_pwrctrl.h | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8723bs/hal/hal_intf.c b/drivers/staging/rtl8723bs/hal/hal_intf.c
> > index 96fe172ced8d..8dc4dd8c6d4c 100644
> > --- a/drivers/staging/rtl8723bs/hal/hal_intf.c
> > +++ b/drivers/staging/rtl8723bs/hal/hal_intf.c
> > @@ -348,7 +348,7 @@ void rtw_hal_dm_watchdog(struct adapter *padapter)
> >
> >  void rtw_hal_dm_watchdog_in_lps(struct adapter *padapter)
> >  {
> > -	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode == true) {
> > +	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode) {
> >  		if (padapter->HalFunc.hal_dm_watchdog_in_lps)
> >  			padapter->HalFunc.hal_dm_watchdog_in_lps(padapter); /* this function caller is in interrupt context */
> >  	}
> > diff --git a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > index 0a48f1653be5..0767dbb84199 100644
> > --- a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > +++ b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > @@ -203,7 +203,7 @@ struct pwrctrl_priv {
> >  	u8 LpsIdleCount;
> >  	u8 power_mgnt;
> >  	u8 org_power_mgnt;
> > -	u8 fw_current_in_ps_mode;
> > +	bool fw_current_in_ps_mode;
> >  	unsigned long	DelayLPSLastTimeStamp;
> >  	s32		pnp_current_pwr_state;
> >  	u8 pnp_bstop_trx;
>
> If this is only checked, how can it ever be true?  Who ever sets this
> value?

I think it's already updated everywhere with true and false, so there is
nothing to change.  But it would be good to make that clear in the log
message.

julia

>
> thanks,
>
> greg k-h
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/YHFwZCh%2Bs7ymrsQN%40kroah.com.
>

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Julia Lawall]

On Sat, 10 Apr 2021, Greg KH wrote:

> On Sat, Apr 10, 2021 at 11:22:32AM +0200, Fabio M. De Francesco wrote:
> > Change the type of fw_current_in_ps_mode from u8 to bool, because
> > it is used everywhere as a bool and, accordingly, it should be
> > declared as a bool. Shorten the controlling
> > expression of an 'if' statement.
> >
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> >  drivers/staging/rtl8723bs/hal/hal_intf.c        | 2 +-
> >  drivers/staging/rtl8723bs/include/rtw_pwrctrl.h | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8723bs/hal/hal_intf.c b/drivers/staging/rtl8723bs/hal/hal_intf.c
> > index 96fe172ced8d..8dc4dd8c6d4c 100644
> > --- a/drivers/staging/rtl8723bs/hal/hal_intf.c
> > +++ b/drivers/staging/rtl8723bs/hal/hal_intf.c
> > @@ -348,7 +348,7 @@ void rtw_hal_dm_watchdog(struct adapter *padapter)
> >
> >  void rtw_hal_dm_watchdog_in_lps(struct adapter *padapter)
> >  {
> > -	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode == true) {
> > +	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode) {
> >  		if (padapter->HalFunc.hal_dm_watchdog_in_lps)
> >  			padapter->HalFunc.hal_dm_watchdog_in_lps(padapter); /* this function caller is in interrupt context */
> >  	}
> > diff --git a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > index 0a48f1653be5..0767dbb84199 100644
> > --- a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > +++ b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > @@ -203,7 +203,7 @@ struct pwrctrl_priv {
> >  	u8 LpsIdleCount;
> >  	u8 power_mgnt;
> >  	u8 org_power_mgnt;
> > -	u8 fw_current_in_ps_mode;
> > +	bool fw_current_in_ps_mode;
> >  	unsigned long	DelayLPSLastTimeStamp;
> >  	s32		pnp_current_pwr_state;
> >  	u8 pnp_bstop_trx;
>
> If this is only checked, how can it ever be true?  Who ever sets this
> value?

I think it's already updated everywhere with true and false, so there is
nothing to change.  But it would be good to make that clear in the log
message.

julia

>
> thanks,
>
> greg k-h
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/YHFwZCh%2Bs7ymrsQN%40kroah.com.
>

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Julia Lawall]

On Sat, 10 Apr 2021, Julia Lawall wrote:

>
>
> On Sat, 10 Apr 2021, Greg KH wrote:
>
> > On Sat, Apr 10, 2021 at 11:22:32AM +0200, Fabio M. De Francesco wrote:
> > > Change the type of fw_current_in_ps_mode from u8 to bool, because
> > > it is used everywhere as a bool and, accordingly, it should be
> > > declared as a bool. Shorten the controlling
> > > expression of an 'if' statement.
> > >
> > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > ---
> > >  drivers/staging/rtl8723bs/hal/hal_intf.c        | 2 +-
> > >  drivers/staging/rtl8723bs/include/rtw_pwrctrl.h | 2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/staging/rtl8723bs/hal/hal_intf.c b/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > index 96fe172ced8d..8dc4dd8c6d4c 100644
> > > --- a/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > +++ b/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > @@ -348,7 +348,7 @@ void rtw_hal_dm_watchdog(struct adapter *padapter)
> > >
> > >  void rtw_hal_dm_watchdog_in_lps(struct adapter *padapter)
> > >  {
> > > -	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode == true) {
> > > +	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode) {
> > >  		if (padapter->HalFunc.hal_dm_watchdog_in_lps)
> > >  			padapter->HalFunc.hal_dm_watchdog_in_lps(padapter); /* this function caller is in interrupt context */
> > >  	}
> > > diff --git a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > index 0a48f1653be5..0767dbb84199 100644
> > > --- a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > +++ b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > @@ -203,7 +203,7 @@ struct pwrctrl_priv {
> > >  	u8 LpsIdleCount;
> > >  	u8 power_mgnt;
> > >  	u8 org_power_mgnt;
> > > -	u8 fw_current_in_ps_mode;
> > > +	bool fw_current_in_ps_mode;
> > >  	unsigned long	DelayLPSLastTimeStamp;
> > >  	s32		pnp_current_pwr_state;
> > >  	u8 pnp_bstop_trx;
> >
> > If this is only checked, how can it ever be true?  Who ever sets this
> > value?
>
> I think it's already updated everywhere with true and false, so there is
> nothing to change.  But it would be good to make that clear in the log
> message.

Oops, I was thinking of the field, not the local variable.
If the field is never set, that seems like a big problem...

julia

>
> julia
>
> >
> > thanks,
> >
> > greg k-h
> >
> > --
> > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/YHFwZCh%2Bs7ymrsQN%40kroah.com.
> >
>

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Julia Lawall]

On Sat, 10 Apr 2021, Julia Lawall wrote:

>
>
> On Sat, 10 Apr 2021, Greg KH wrote:
>
> > On Sat, Apr 10, 2021 at 11:22:32AM +0200, Fabio M. De Francesco wrote:
> > > Change the type of fw_current_in_ps_mode from u8 to bool, because
> > > it is used everywhere as a bool and, accordingly, it should be
> > > declared as a bool. Shorten the controlling
> > > expression of an 'if' statement.
> > >
> > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > ---
> > >  drivers/staging/rtl8723bs/hal/hal_intf.c        | 2 +-
> > >  drivers/staging/rtl8723bs/include/rtw_pwrctrl.h | 2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/staging/rtl8723bs/hal/hal_intf.c b/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > index 96fe172ced8d..8dc4dd8c6d4c 100644
> > > --- a/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > +++ b/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > @@ -348,7 +348,7 @@ void rtw_hal_dm_watchdog(struct adapter *padapter)
> > >
> > >  void rtw_hal_dm_watchdog_in_lps(struct adapter *padapter)
> > >  {
> > > -	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode == true) {
> > > +	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode) {
> > >  		if (padapter->HalFunc.hal_dm_watchdog_in_lps)
> > >  			padapter->HalFunc.hal_dm_watchdog_in_lps(padapter); /* this function caller is in interrupt context */
> > >  	}
> > > diff --git a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > index 0a48f1653be5..0767dbb84199 100644
> > > --- a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > +++ b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > @@ -203,7 +203,7 @@ struct pwrctrl_priv {
> > >  	u8 LpsIdleCount;
> > >  	u8 power_mgnt;
> > >  	u8 org_power_mgnt;
> > > -	u8 fw_current_in_ps_mode;
> > > +	bool fw_current_in_ps_mode;
> > >  	unsigned long	DelayLPSLastTimeStamp;
> > >  	s32		pnp_current_pwr_state;
> > >  	u8 pnp_bstop_trx;
> >
> > If this is only checked, how can it ever be true?  Who ever sets this
> > value?
>
> I think it's already updated everywhere with true and false, so there is
> nothing to change.  But it would be good to make that clear in the log
> message.

Oops, I was thinking of the field, not the local variable.
If the field is never set, that seems like a big problem...

julia

>
> julia
>
> >
> > thanks,
> >
> > greg k-h
> >
> > --
> > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/YHFwZCh%2Bs7ymrsQN%40kroah.com.
> >
>

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Fabio M. De Francesco]

On Saturday, April 10, 2021 12:03:14 PM CEST you wrote:
> On Sat, 10 Apr 2021, Julia Lawall wrote:
> > On Sat, 10 Apr 2021, Greg KH wrote:
> > > On Sat, Apr 10, 2021 at 11:22:32AM +0200, Fabio M. De Francesco 
wrote:
> > > > Change the type of fw_current_in_ps_mode from u8 to bool, because
> > > > it is used everywhere as a bool and, accordingly, it should be
> > > > declared as a bool. Shorten the controlling
> > > > expression of an 'if' statement.
> > > > 
> > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > > ---
> > > > 
> > > >  drivers/staging/rtl8723bs/hal/hal_intf.c        | 2 +-
> > > >  drivers/staging/rtl8723bs/include/rtw_pwrctrl.h | 2 +-
> > > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > > b/drivers/staging/rtl8723bs/hal/hal_intf.c index
> > > > 96fe172ced8d..8dc4dd8c6d4c 100644
> > > > --- a/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > > +++ b/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > > @@ -348,7 +348,7 @@ void rtw_hal_dm_watchdog(struct adapter
> > > > *padapter)
> > > > 
> > > >  void rtw_hal_dm_watchdog_in_lps(struct adapter *padapter)
> > > >  {
> > > > 
> > > > -	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode == 
true) {
> > > > +	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode) {
> > > > 
> > > >  		if (padapter->HalFunc.hal_dm_watchdog_in_lps)
> > > >  		
> > > >  			padapter-
>HalFunc.hal_dm_watchdog_in_lps(padapter); /* this
> > > >  			function caller is in interrupt context 
*/> > >  	
> > > >  	}
> > > > 
> > > > diff --git a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > > b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h index
> > > > 0a48f1653be5..0767dbb84199 100644
> > > > --- a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > > +++ b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > > @@ -203,7 +203,7 @@ struct pwrctrl_priv {
> > > > 
> > > >  	u8 LpsIdleCount;
> > > >  	u8 power_mgnt;
> > > >  	u8 org_power_mgnt;
> > > > 
> > > > -	u8 fw_current_in_ps_mode;
> > > > +	bool fw_current_in_ps_mode;
> > > > 
> > > >  	unsigned long	DelayLPSLastTimeStamp;
> > > >  	s32		pnp_current_pwr_state;
> > > >  	u8 pnp_bstop_trx;
> > > 
> > > If this is only checked, how can it ever be true?  Who ever sets this
> > > value?
> > 
> > I think it's already updated everywhere with true and false, so there
> > is
> > nothing to change.  But it would be good to make that clear in the log
> > message.
> 
> Oops, I was thinking of the field, not the local variable.
> If the field is never set, that seems like a big problem...
> 
> julia
>
>
Julia, I'm a bit confused...

I've anew examined that line and noticed that fw_current_in_ps_mode is 
referenced by through a pointer returned by adapter_to_pwrctl(padapter). 
padapter is a pointer set in other files of the driver.

To me it seems that is perfectly reasonable that, as a result, 
fw_current_in_ps_mode could be set (externally) either with true or false.

I'm sure I'm missing something, since both you and Greg have a larger 
experience than I could ever have. Can you please elaborate this topic a 
bit more in order to make me understand whatever I can't see?

Thanks for your kind help,

Fabio
>
> 
> > julia
> > 
> > > thanks,
> > > 
> > > greg k-h
> > > 
> > > --
> > > You received this message because you are subscribed to the Google
> > > Groups "outreachy-kernel" group. To unsubscribe from this group and
> > > stop receiving emails from it, send an email to
> > > outreachy-kernel+unsubscribe@googlegroups.com. To view this
> > > discussion on the web visit
> > > https://groups.google.com/d/msgid/outreachy-kernel/YHFwZCh%2Bs7ymrsQ
> > > N%40kroah.com.

Re: [Outreachy kernel] [PATCH 4/4] staging: rtl8723bs: Change the type and use of a variable

[Julia Lawall]

On Sat, 10 Apr 2021, Fabio M. De Francesco wrote:

> On Saturday, April 10, 2021 12:03:14 PM CEST you wrote:
> > On Sat, 10 Apr 2021, Julia Lawall wrote:
> > > On Sat, 10 Apr 2021, Greg KH wrote:
> > > > On Sat, Apr 10, 2021 at 11:22:32AM +0200, Fabio M. De Francesco
> wrote:
> > > > > Change the type of fw_current_in_ps_mode from u8 to bool, because
> > > > > it is used everywhere as a bool and, accordingly, it should be
> > > > > declared as a bool. Shorten the controlling
> > > > > expression of an 'if' statement.
> > > > >
> > > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > > > ---
> > > > >
> > > > >  drivers/staging/rtl8723bs/hal/hal_intf.c        | 2 +-
> > > > >  drivers/staging/rtl8723bs/include/rtw_pwrctrl.h | 2 +-
> > > > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > > > b/drivers/staging/rtl8723bs/hal/hal_intf.c index
> > > > > 96fe172ced8d..8dc4dd8c6d4c 100644
> > > > > --- a/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > > > +++ b/drivers/staging/rtl8723bs/hal/hal_intf.c
> > > > > @@ -348,7 +348,7 @@ void rtw_hal_dm_watchdog(struct adapter
> > > > > *padapter)
> > > > >
> > > > >  void rtw_hal_dm_watchdog_in_lps(struct adapter *padapter)
> > > > >  {
> > > > >
> > > > > -	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode ==
> true) {
> > > > > +	if (adapter_to_pwrctl(padapter)->fw_current_in_ps_mode) {
> > > > >
> > > > >  		if (padapter->HalFunc.hal_dm_watchdog_in_lps)
> > > > >
> > > > >  			padapter-
> >HalFunc.hal_dm_watchdog_in_lps(padapter); /* this
> > > > >  			function caller is in interrupt context
> */> > >
> > > > >  	}
> > > > >
> > > > > diff --git a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > > > b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h index
> > > > > 0a48f1653be5..0767dbb84199 100644
> > > > > --- a/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > > > +++ b/drivers/staging/rtl8723bs/include/rtw_pwrctrl.h
> > > > > @@ -203,7 +203,7 @@ struct pwrctrl_priv {
> > > > >
> > > > >  	u8 LpsIdleCount;
> > > > >  	u8 power_mgnt;
> > > > >  	u8 org_power_mgnt;
> > > > >
> > > > > -	u8 fw_current_in_ps_mode;
> > > > > +	bool fw_current_in_ps_mode;
> > > > >
> > > > >  	unsigned long	DelayLPSLastTimeStamp;
> > > > >  	s32		pnp_current_pwr_state;
> > > > >  	u8 pnp_bstop_trx;
> > > >
> > > > If this is only checked, how can it ever be true?  Who ever sets this
> > > > value?
> > >
> > > I think it's already updated everywhere with true and false, so there
> > > is
> > > nothing to change.  But it would be good to make that clear in the log
> > > message.
> >
> > Oops, I was thinking of the field, not the local variable.
> > If the field is never set, that seems like a big problem...
> >
> > julia
> >
> >
> Julia, I'm a bit confused...
>
> I've anew examined that line and noticed that fw_current_in_ps_mode is
> referenced by through a pointer returned by adapter_to_pwrctl(padapter).
> padapter is a pointer set in other files of the driver.
>
> To me it seems that is perfectly reasonable that, as a result,
> fw_current_in_ps_mode could be set (externally) either with true or false.
>
> I'm sure I'm missing something, since both you and Greg have a larger
> experience than I could ever have. Can you please elaborate this topic a
> bit more in order to make me understand whatever I can't see?

OK, I was just following my recollection of the code.  It seems that the
only thing that is needed is to explain in the log message how the
variable is initialized and why changing the type doesn't mean that you
have to change the initializations.  A weakness of the patch notation is
that you only see what changed, and not all of the code that is actually
relevant.  So if there is something that is not visible and that is
important to convince the reader that the change is correct, then it
should be mentioned in the log message.

Again, not having looked at the code, but does this pointer already have a
bool type?  Or does it have a u8 type?  Perhaps the pointer has to be
changed too?

julia