Skip to contentSkip to conversation

staging: r8188eu: add error handling of usb read errors

PATCH Superseded Pavel Skripkin 3 participants 13 messages May 18, 2022 – May 19, 2022 Staging

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Pavel Skripkin

author
#0 lore raw

cover staging: r8188eu: add error handling of usb read errors

Hi,

it's reincarnation of my old series for adding sane error handling in r8818eu.

*Problem*

Old code was returning just stack variable in case of read error. It's not the best approach, since passing around stack data might cause device misconfiguration or even kernel data leakage

To solve this I've changed rtw_read{8,16,32} prototypes to return an error via return value and data via passed pointer. Some work should be done to propogate an error down to calltrace, but it's good way to at least start doing sane I/O error handling

Tested locally on qemu with TP-Link TL-WN722N v2/v3 [Realtek RTL8188EUS] device. More testing is welcomed, of course :)

Series is based on top of staging-testing branch

Pavel Skripkin (4):
staging: r8188eu: add error handling of rtw_read8
staging: r8188eu: add error handling of rtw_read16
staging: r8188eu: add error handling of rtw_read32
MAINTAINERS: add myself as r8188eu reviewer

 MAINTAINERS                                   |   1 +
 drivers/staging/r8188eu/core/rtw_cmd.c        |  15 +-
 drivers/staging/r8188eu/core/rtw_efuse.c      |  32 ++-
 drivers/staging/r8188eu/core/rtw_fw.c         |  62 ++++-
 drivers/staging/r8188eu/core/rtw_led.c        |  20 +-
 drivers/staging/r8188eu/core/rtw_mlme_ext.c   |  62 ++++-
 drivers/staging/r8188eu/core/rtw_pwrctrl.c    |   9 +-
 drivers/staging/r8188eu/core/rtw_wlan_util.c  |  20 +-
 .../r8188eu/hal/Hal8188ERateAdaptive.c        |  21 +-
 drivers/staging/r8188eu/hal/HalPhyRf_8188e.c  |  20 +-
 drivers/staging/r8188eu/hal/HalPwrSeqCmd.c    |   9 +-
 drivers/staging/r8188eu/hal/hal_com.c         |  24 +-
 drivers/staging/r8188eu/hal/rtl8188e_cmd.c    |  37 ++-
 drivers/staging/r8188eu/hal/rtl8188e_dm.c     |   6 +-
 .../staging/r8188eu/hal/rtl8188e_hal_init.c   | 117 +++++++--
 drivers/staging/r8188eu/hal/rtl8188e_phycfg.c |  30 ++-
 drivers/staging/r8188eu/hal/usb_halinit.c     | 239 +++++++++++++++---
 drivers/staging/r8188eu/hal/usb_ops_linux.c   |  33 ++-
 drivers/staging/r8188eu/include/rtw_io.h      |   6 +-
 drivers/staging/r8188eu/os_dep/ioctl_linux.c  |  61 ++++-
 drivers/staging/r8188eu/os_dep/os_intfs.c     |  19 +-
 21 files changed, 681 insertions(+), 162 deletions(-)
Signature · 1 line
2.36.1
author
#1 lore raw

PATCH 1/4 staging: r8188eu: add error handling of rtw_read8

rtw_read8() reads data from device via USB API which may fail. In case of any failure previous code returned stack data to callers, which is wrong.

Fix it by changing rtw_read8() prototype and prevent caller from touching random stack data

Signed-off-by Pavel Skripkin <[email protected]>
---
 drivers/staging/r8188eu/core/rtw_efuse.c      |  13 +-
 drivers/staging/r8188eu/core/rtw_fw.c         |  46 ++++-
 drivers/staging/r8188eu/core/rtw_led.c        |  20 ++-
 drivers/staging/r8188eu/core/rtw_mlme_ext.c   |  49 +++++-
 drivers/staging/r8188eu/core/rtw_wlan_util.c  |  20 ++-
 drivers/staging/r8188eu/hal/HalPhyRf_8188e.c  |  17 +-
 drivers/staging/r8188eu/hal/HalPwrSeqCmd.c    |   9 +-
 drivers/staging/r8188eu/hal/hal_com.c         |  24 ++-
 drivers/staging/r8188eu/hal/rtl8188e_cmd.c    |  37 +++-
 drivers/staging/r8188eu/hal/rtl8188e_dm.c     |   6 +-
 .../staging/r8188eu/hal/rtl8188e_hal_init.c   |  69 ++++++--
 drivers/staging/r8188eu/hal/rtl8188e_phycfg.c |  10 +-
 drivers/staging/r8188eu/hal/usb_halinit.c     | 159 ++++++++++++++----
 drivers/staging/r8188eu/hal/usb_ops_linux.c   |   7 +-
 drivers/staging/r8188eu/include/rtw_io.h      |   2 +-
 drivers/staging/r8188eu/os_dep/ioctl_linux.c  |  18 +-
 16 files changed, 407 insertions(+), 99 deletions(-)
drivers/staging/r8188eu/core/rtw_efuse.c +11 -2
index 0e0e60638880..a2691c7f96f6 100644
@@ -28,14 +28,21 @@ ReadEFuseByte(
 	u32 value32;
 	u8 readbyte;
 	u16 retry;
+	int res;
 
 	/* Write Address */
 	rtw_write8(Adapter, EFUSE_CTRL + 1, (_offset & 0xff));
-	readbyte = rtw_read8(Adapter, EFUSE_CTRL + 2);
+	res = rtw_read8(Adapter, EFUSE_CTRL + 2, &readbyte);
+	if (res)
+		return;
+
 	rtw_write8(Adapter, EFUSE_CTRL + 2, ((_offset >> 8) & 0x03) | (readbyte & 0xfc));
 
 	/* Write bit 32 0 */
-	readbyte = rtw_read8(Adapter, EFUSE_CTRL + 3);
+	res = rtw_read8(Adapter, EFUSE_CTRL + 3, &readbyte);
+	if (res)
+		return;
+
 	rtw_write8(Adapter, EFUSE_CTRL + 3, (readbyte & 0x7f));
 
 	/* Check bit 32 read-ready */

@@ -54,6 +61,8 @@ ReadEFuseByte(
 	value32 = rtw_read32(Adapter, EFUSE_CTRL);
 
 	*pbuf = (u8)(value32 & 0xff);
+
+	/* FIXME: return an error to caller */
 }
 
 /*-----------------------------------------------------------------------------
drivers/staging/r8188eu/core/rtw_fw.c +39 -7

99 lines not on this page · raw message

drivers/staging/r8188eu/core/rtw_led.c +15 -5
index 2f3000428af7..b532e614c5b6 100644
@@ -34,28 +34,38 @@ static void ResetLedStatus(struct LED_871x *pLed)
 
 static void SwLedOn(struct adapter *padapter, struct LED_871x *pLed)
 {
-	u8	LedCfg;
+	u8 LedCfg;
+	int res;
 
 	if (padapter->bSurpriseRemoved || padapter->bDriverStopped)
 		return;
 
-	LedCfg = rtw_read8(padapter, REG_LEDCFG2);
+	res = rtw_read8(padapter, REG_LEDCFG2, &LedCfg);
+	if (res)
+		return;
+
 	rtw_write8(padapter, REG_LEDCFG2, (LedCfg & 0xf0) | BIT(5) | BIT(6)); /*  SW control led0 on. */
 	pLed->bLedOn = true;
 }
 
 static void SwLedOff(struct adapter *padapter, struct LED_871x *pLed)
 {
-	u8	LedCfg;
+	u8 LedCfg;
+	int res;
 
 	if (padapter->bSurpriseRemoved || padapter->bDriverStopped)
 		goto exit;
 
-	LedCfg = rtw_read8(padapter, REG_LEDCFG2);/* 0x4E */
+	res = rtw_read8(padapter, REG_LEDCFG2, &LedCfg);/* 0x4E */
+	if (res)
+		goto exit;
 
 	LedCfg &= 0x90; /*  Set to software control. */
 	rtw_write8(padapter, REG_LEDCFG2, (LedCfg | BIT(3)));
-	LedCfg = rtw_read8(padapter, REG_MAC_PINMUX_CFG);
+	res = rtw_read8(padapter, REG_MAC_PINMUX_CFG, &LedCfg);
+	if (res)
+		goto exit;
+
 	LedCfg &= 0xFE;
 	rtw_write8(padapter, REG_MAC_PINMUX_CFG, LedCfg);
 exit:
drivers/staging/r8188eu/core/rtw_mlme_ext.c +42 -7

107 lines not on this page · raw message

drivers/staging/r8188eu/core/rtw_wlan_util.c +17 -3
index 27035eac6e61..f5002c88a5ac 100644
@@ -279,8 +279,13 @@ void Restore_DM_Func_Flag(struct adapter *padapter)
 void Set_MSR(struct adapter *padapter, u8 type)
 {
 	u8 val8;
+	int res;
 
-	val8 = rtw_read8(padapter, MSR) & 0x0c;
+	res = rtw_read8(padapter, MSR, &val8);
+	if (res)
+		return;
+
+	val8 &= 0x0c;
 	val8 |= type;
 	rtw_write8(padapter, MSR, val8);
 }

@@ -505,7 +510,11 @@ int WMM_param_handler(struct adapter *padapter, struct ndis_802_11_var_ie *pIE)
 
 static void set_acm_ctrl(struct adapter *adapter, u8 acm_mask)
 {
-	u8 acmctrl = rtw_read8(adapter, REG_ACMHWCTRL);
+	u8 acmctrl;
+	int res = rtw_read8(adapter, REG_ACMHWCTRL, &acmctrl);
+
+	if (res)
+		return;
 
 	if (acm_mask > 1)
 		acmctrl = acmctrl | 0x1;

@@ -763,6 +772,7 @@ void HT_info_handler(struct adapter *padapter, struct ndis_802_11_var_ie *pIE)
 static void set_min_ampdu_spacing(struct adapter *adapter, u8 spacing)
 {
 	u8 sec_spacing;
+	int res;
 
 	if (spacing <= 7) {
 		switch (adapter->securitypriv.dot11PrivacyAlgrthm) {

@@ -784,8 +794,12 @@ static void set_min_ampdu_spacing(struct adapter *adapter, u8 spacing)
 		if (spacing < sec_spacing)
 			spacing = sec_spacing;
 
+		res = rtw_read8(adapter, REG_AMPDU_MIN_SPACE, &sec_spacing);
+		if (res)
+			return;
+
 		rtw_write8(adapter, REG_AMPDU_MIN_SPACE,
-			   (rtw_read8(adapter, REG_AMPDU_MIN_SPACE) & 0xf8) | spacing);
+			   (sec_spacing & 0xf8) | spacing);
 	}
 }
drivers/staging/r8188eu/hal/HalPhyRf_8188e.c +14 -3
index b944c8071a3b..aa4b4459329e 100644
@@ -470,9 +470,17 @@ static void _PHY_SaveMACRegisters(
 	)
 {
 	u32 i;
+	int res;
 
-	for (i = 0; i < (IQK_MAC_REG_NUM - 1); i++)
-		MACBackup[i] = rtw_read8(adapt, MACReg[i]);
+	for (i = 0; i < (IQK_MAC_REG_NUM - 1); i++) {
+		u8 reg;
+
+		res = rtw_read8(adapt, MACReg[i], &reg);
+		if (res)
+			return;
+
+		MACBackup[i] = reg;
+	}
 
 	MACBackup[i] = rtw_read32(adapt, MACReg[i]);
 }

@@ -739,9 +747,12 @@ static void phy_LCCalibrate_8188E(struct adapter *adapt)
 {
 	u8 tmpreg;
 	u32 RF_Amode = 0, LC_Cal;
+	int res;
 
 	/* Check continuous TX and Packet TX */
-	tmpreg = rtw_read8(adapt, 0xd03);
+	res = rtw_read8(adapt, 0xd03, &tmpreg);
+	if (res)
+		return;
 
 	if ((tmpreg & 0x70) != 0)			/* Deal with contisuous TX case */
 		rtw_write8(adapt, 0xd03, tmpreg & 0x8F);	/* disable all continuous TX */
drivers/staging/r8188eu/hal/HalPwrSeqCmd.c +7 -2
index 5b91aec6a7e3..fe2fe63dbc18 100644
@@ -34,6 +34,7 @@ u8 HalPwrSeqCmdParsing(struct adapter *padapter, struct wl_pwr_cfg pwrseqcmd[])
 	u32 offset = 0;
 	u32 poll_count = 0; /*  polling autoload done. */
 	u32 max_poll_count = 5000;
+	int res;
 
 	do {
 		pwrcfgcmd = pwrseqcmd[aryidx];

@@ -43,7 +44,9 @@ u8 HalPwrSeqCmdParsing(struct adapter *padapter, struct wl_pwr_cfg pwrseqcmd[])
 			offset = GET_PWR_CFG_OFFSET(pwrcfgcmd);
 
 			/*  Read the value from system register */
-			value = rtw_read8(padapter, offset);
+			res = rtw_read8(padapter, offset, &value);
+			if (res)
+				return false;
 
 			value &= ~(GET_PWR_CFG_MASK(pwrcfgcmd));
 			value |= (GET_PWR_CFG_VALUE(pwrcfgcmd) & GET_PWR_CFG_MASK(pwrcfgcmd));

@@ -55,7 +58,9 @@ u8 HalPwrSeqCmdParsing(struct adapter *padapter, struct wl_pwr_cfg pwrseqcmd[])
 			poll_bit = false;
 			offset = GET_PWR_CFG_OFFSET(pwrcfgcmd);
 			do {
-				value = rtw_read8(padapter, offset);
+				res = rtw_read8(padapter, offset, &value);
+				if (res)
+					return false;
 
 				value &= GET_PWR_CFG_MASK(pwrcfgcmd);
 				if (value == (GET_PWR_CFG_VALUE(pwrcfgcmd) & GET_PWR_CFG_MASK(pwrcfgcmd)))
drivers/staging/r8188eu/hal/hal_com.c +17 -7
index 910cc07f656c..7717ee722cce 100644
@@ -297,13 +297,15 @@ s32 c2h_evt_read(struct adapter *adapter, u8 *buf)
 {
 	s32 ret = _FAIL;
 	struct c2h_evt_hdr *c2h_evt;
-	int i;
+	int i, res;
 	u8 trigger;
 
 	if (!buf)
 		goto exit;
 
-	trigger = rtw_read8(adapter, REG_C2HEVT_CLEAR);
+	res = rtw_read8(adapter, REG_C2HEVT_CLEAR, &trigger);
+	if (res)
+		return _FAIL;
 
 	if (trigger == C2H_EVT_HOST_CLOSE)
 		goto exit; /* Not ready */

@@ -314,13 +316,21 @@ s32 c2h_evt_read(struct adapter *adapter, u8 *buf)
 
 	memset(c2h_evt, 0, 16);
 
-	*buf = rtw_read8(adapter, REG_C2HEVT_MSG_NORMAL);
-	*(buf + 1) = rtw_read8(adapter, REG_C2HEVT_MSG_NORMAL + 1);
+	res = rtw_read8(adapter, REG_C2HEVT_MSG_NORMAL, buf);
+	if (res)
+		return _FAIL;
+
+	res = rtw_read8(adapter, REG_C2HEVT_MSG_NORMAL + 1, buf + 1);
+	if (res)
+		return _FAIL;
 
 	/* Read the content */
-	for (i = 0; i < c2h_evt->plen; i++)
-		c2h_evt->payload[i] = rtw_read8(adapter, REG_C2HEVT_MSG_NORMAL +
-						sizeof(*c2h_evt) + i);
+	for (i = 0; i < c2h_evt->plen; i++) {
+		res = rtw_read8(adapter, REG_C2HEVT_MSG_NORMAL +
+						sizeof(*c2h_evt) + i, c2h_evt->payload + i);
+		if (res)
+			return _FAIL;
+	}
 
 	ret = _SUCCESS;
drivers/staging/r8188eu/hal/rtl8188e_cmd.c +31 -6
index 475650dc7301..b01ee1695fee 100644
@@ -18,13 +18,18 @@
 
 static u8 _is_fw_read_cmd_down(struct adapter *adapt, u8 msgbox_num)
 {
-	u8 read_down = false;
+	u8 read_down = false, reg;
 	int	retry_cnts = 100;
+	int res;
 
 	u8 valid;
 
 	do {
-		valid = rtw_read8(adapt, REG_HMETFR) & BIT(msgbox_num);
+		res = rtw_read8(adapt, REG_HMETFR, &reg);
+		if (res)
+			continue;
+
+		valid = reg & BIT(msgbox_num);
 		if (0 == valid)
 			read_down = true;
 	} while ((!read_down) && (retry_cnts--));

@@ -533,6 +538,8 @@ void rtl8188e_set_FwJoinBssReport_cmd(struct adapter *adapt, u8 mstatus)
 	bool	bcn_valid = false;
 	u8 DLBcnCount = 0;
 	u32 poll = 0;
+	u8 reg;
+	int res;
 
 	if (mstatus == 1) {
 		/*  We should set AID, correct TSF, HW seq enable before set JoinBssReport to Fw in 88/92C. */

@@ -547,8 +554,17 @@ void rtl8188e_set_FwJoinBssReport_cmd(struct adapter *adapt, u8 mstatus)
 		/*  Disable Hw protection for a time which revserd for Hw sending beacon. */
 		/*  Fix download reserved page packet fail that access collision with the protection time. */
 		/*  2010.05.11. Added by tynli. */
-		rtw_write8(adapt, REG_BCN_CTRL, rtw_read8(adapt, REG_BCN_CTRL) & (~BIT(3)));
-		rtw_write8(adapt, REG_BCN_CTRL, rtw_read8(adapt, REG_BCN_CTRL) | BIT(4));
+		res = rtw_read8(adapt, REG_BCN_CTRL, &reg);
+		if (res)
+			return;
+
+		rtw_write8(adapt, REG_BCN_CTRL, reg & (~BIT(3)));
+
+		res = rtw_read8(adapt, REG_BCN_CTRL, &reg);
+		if (res)
+			return;
+
+		rtw_write8(adapt, REG_BCN_CTRL, reg | BIT(4));
 
 		if (haldata->RegFwHwTxQCtrl & BIT(6))
 			bSendBeacon = true;

@@ -581,8 +597,17 @@ void rtl8188e_set_FwJoinBssReport_cmd(struct adapter *adapt, u8 mstatus)
 		/*  */
 
 		/*  Enable Bcn */
-		rtw_write8(adapt, REG_BCN_CTRL, rtw_read8(adapt, REG_BCN_CTRL) | BIT(3));
-		rtw_write8(adapt, REG_BCN_CTRL, rtw_read8(adapt, REG_BCN_CTRL) & (~BIT(4)));
+		res = rtw_read8(adapt, REG_BCN_CTRL, &reg);
+		if (res)
+			return;
+
+		rtw_write8(adapt, REG_BCN_CTRL, reg | BIT(3));
+
+		res = rtw_read8(adapt, REG_BCN_CTRL, &reg);
+		if (res)
+			return;
+
+		rtw_write8(adapt, REG_BCN_CTRL, reg & (~BIT(4)));
 
 		/*  To make sure that if there exists an adapter which would like to send beacon. */
 		/*  If exists, the origianl value of 0x422[6] will be 1, we should check this to */
drivers/staging/r8188eu/hal/rtl8188e_dm.c +5 -1
index 6d28e3dc0d26..0399872c4546 100644
@@ -12,8 +12,12 @@
 static void dm_InitGPIOSetting(struct adapter *Adapter)
 {
 	u8	tmp1byte;
+	int res;
+
+	res = rtw_read8(Adapter, REG_GPIO_MUXCFG, &tmp1byte);
+	if (res)
+		return;
 
-	tmp1byte = rtw_read8(Adapter, REG_GPIO_MUXCFG);
 	tmp1byte &= (GPIOSEL_GPIO | ~GPIOSEL_ENBT);
 
 	rtw_write8(Adapter, REG_GPIO_MUXCFG, tmp1byte);
drivers/staging/r8188eu/hal/rtl8188e_hal_init.c +55 -14

120 lines not on this page · raw message

drivers/staging/r8188eu/hal/rtl8188e_phycfg.c +8 -2
index 4864dafd887b..985339a974fc 100644
@@ -594,6 +594,7 @@ _PHY_SetBWMode92C(
 	struct hal_data_8188e *pHalData = &Adapter->haldata;
 	u8 regBwOpMode;
 	u8 regRRSR_RSC;
+	int res;
 
 	if (Adapter->bDriverStopped)
 		return;

@@ -602,8 +603,13 @@ _PHY_SetBWMode92C(
 	/* 3<1>Set MAC register */
 	/* 3 */
 
-	regBwOpMode = rtw_read8(Adapter, REG_BWOPMODE);
-	regRRSR_RSC = rtw_read8(Adapter, REG_RRSR + 2);
+	res = rtw_read8(Adapter, REG_BWOPMODE, &regBwOpMode);
+	if (res)
+		return;
+
+	res = rtw_read8(Adapter, REG_RRSR + 2, &regRRSR_RSC);
+	if (res)
+		return;
 
 	switch (pHalData->CurrentChannelBW) {
 	case HT_CHANNEL_WIDTH_20:
drivers/staging/r8188eu/hal/usb_halinit.c +130 -29

325 lines not on this page · raw message

drivers/staging/r8188eu/hal/usb_ops_linux.c +2 -5
index d5e674542a78..f399a7fd8b97 100644
@@ -94,16 +94,13 @@ static int usb_write(struct intf_hdl *intf, u16 value, void *data, u8 size)
 	return status;
 }
 
-u8 rtw_read8(struct adapter *adapter, u32 addr)
+int __must_check rtw_read8(struct adapter *adapter, u32 addr, u8 *data)
 {
 	struct io_priv *io_priv = &adapter->iopriv;
 	struct intf_hdl *intf = &io_priv->intf;
 	u16 value = addr & 0xffff;
-	u8 data;
 
-	usb_read(intf, value, &data, 1);
-
-	return data;
+	return usb_read(intf, value, data, 1);
 }
 
 u16 rtw_read16(struct adapter *adapter, u32 addr)
drivers/staging/r8188eu/include/rtw_io.h +1 -1
index 6910e2b430e2..1198d3850a6d 100644
@@ -220,7 +220,7 @@ void unregister_intf_hdl(struct intf_hdl *pintfhdl);
 void _rtw_attrib_read(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 void _rtw_attrib_write(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 
-u8 rtw_read8(struct adapter *adapter, u32 addr);
+int __must_check rtw_read8(struct adapter *adapter, u32 addr, u8 *data);
 u16 rtw_read16(struct adapter *adapter, u32 addr);
 u32 rtw_read32(struct adapter *adapter, u32 addr);
 void _rtw_read_mem(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
drivers/staging/r8188eu/os_dep/ioctl_linux.c +13 -5
index 42cb79cee2ae..66aac2cbe3a9 100644
@@ -1912,8 +1912,11 @@ static int rtw_wx_read32(struct net_device *dev,
 
 	switch (bytes) {
 	case 1:
-		data32 = rtw_read8(padapter, addr);
-		sprintf(extra, "0x%02X", data32);
+		ret = rtw_read8(padapter, addr, (u8 *) &data32);
+		if (ret)
+			goto err_free_ptmp;
+
+		sprintf(extra, "0x%02X", data32 & 0xff);
 		break;
 	case 2:
 		data32 = rtw_read16(padapter, addr);

@@ -3259,6 +3262,7 @@ static void rtw_set_dynamic_functions(struct adapter *adapter, u8 dm_func)
 {
 	struct hal_data_8188e *haldata = &adapter->haldata;
 	struct odm_dm_struct *odmpriv = &haldata->odmpriv;
+	int res;
 
 	switch (dm_func) {
 	case 0:

@@ -3274,7 +3278,9 @@ static void rtw_set_dynamic_functions(struct adapter *adapter, u8 dm_func)
 		if (!(odmpriv->SupportAbility & DYNAMIC_BB_DIG)) {
 			struct rtw_dig *digtable = &odmpriv->DM_DigTable;
 
-			digtable->CurIGValue = rtw_read8(adapter, 0xc50);
+			res = rtw_read8(adapter, 0xc50, &digtable->CurIGValue);
+			(void) res;
+			/* FIXME: return an error to caller */
 		}
 		odmpriv->SupportAbility = DYNAMIC_ALL_FUNC_ENABLE;
 		break;

@@ -3410,8 +3416,9 @@ static int rtw_dbg_port(struct net_device *dev,
 			u16 reg = arg;
 			u16 start_value = 0;
 			u32 write_num = extra_arg;
-			int i;
+			int i, res;
 			struct xmit_frame	*xmit_frame;
+			u8 val8;
 
 			xmit_frame = rtw_IOL_accquire_xmit_frame(padapter);
 			if (!xmit_frame) {

@@ -3424,7 +3431,8 @@ static int rtw_dbg_port(struct net_device *dev,
 			if (rtl8188e_IOL_exec_cmds_sync(padapter, xmit_frame, 5000, 0) != _SUCCESS)
 				ret = -EPERM;
 
-			rtw_read8(padapter, reg);
+			/* FIXME: is this read necessary? */
+			res = rtw_read8(padapter, reg, &val8);
 		}
 			break;
Signature · 1 line
2.36.1
author
#6 lore raw

PATCH 2/4 staging: r8188eu: add error handling of rtw_read16

In reply to Pavel Skripkin

rtw_read16() reads data from device via USB API which may fail. In case of any failure previous code returned stack data to callers, which is wrong.

Fix it by changing rtw_read16() prototype and prevent caller from touching random stack data

Signed-off-by Pavel Skripkin <[email protected]>
---
 .../staging/r8188eu/hal/rtl8188e_hal_init.c   | 21 ++++++++++++---
 drivers/staging/r8188eu/hal/rtl8188e_phycfg.c |  8 ++++--
 drivers/staging/r8188eu/hal/usb_halinit.c     | 27 ++++++++++++++++---
 drivers/staging/r8188eu/hal/usb_ops_linux.c   | 13 ++++++---
 drivers/staging/r8188eu/include/rtw_io.h      |  2 +-
 drivers/staging/r8188eu/os_dep/ioctl_linux.c  | 11 +++++---
 drivers/staging/r8188eu/os_dep/os_intfs.c     |  6 ++++-
 7 files changed, 69 insertions(+), 19 deletions(-)
drivers/staging/r8188eu/hal/rtl8188e_hal_init.c +17 -4
index e67ecbd1ba79..22661c66cc18 100644
@@ -249,11 +249,14 @@ static void efuse_read_phymap_from_txpktbuf(
 		hi32 = cpu_to_le32(rtw_read32(adapter, REG_PKTBUF_DBG_DATA_H));
 
 		if (i == 0) {
+			int res;
+			u16 reg;
 			/* Although lenc is only used in a debug statement,
 			 * do not remove it as the rtw_read16() call consumes
 			 * 2 bytes from the EEPROM source.
 			 */
-			rtw_read16(adapter, REG_PKTBUF_DBG_DATA_L);
+			res = rtw_read16(adapter, REG_PKTBUF_DBG_DATA_L, &reg);
+			(void) res;
 
 			len = le32_to_cpu(lo32) & 0x0000ffff;
 

@@ -355,25 +358,35 @@ int rtl8188e_IOL_exec_cmds_sync(struct adapter *adapter, struct xmit_frame *xmit
 void rtl8188e_EfusePowerSwitch(struct adapter *pAdapter, u8 PwrState)
 {
 	u16	tmpV16;
+	int res;
 
 	if (PwrState) {
 		rtw_write8(pAdapter, REG_EFUSE_ACCESS, EFUSE_ACCESS_ON);
 
 		/*  1.2V Power: From VDDON with Power Cut(0x0000h[15]), defualt valid */
-		tmpV16 = rtw_read16(pAdapter, REG_SYS_ISO_CTRL);
+		res = rtw_read16(pAdapter, REG_SYS_ISO_CTRL, &tmpV16);
+		if (res)
+			return;
+
 		if (!(tmpV16 & PWC_EV12V)) {
 			tmpV16 |= PWC_EV12V;
 			rtw_write16(pAdapter, REG_SYS_ISO_CTRL, tmpV16);
 		}
 		/*  Reset: 0x0000h[28], default valid */
-		tmpV16 =  rtw_read16(pAdapter, REG_SYS_FUNC_EN);
+		res = rtw_read16(pAdapter, REG_SYS_FUNC_EN, &tmpV16);
+		if (res)
+			return;
+
 		if (!(tmpV16 & FEN_ELDR)) {
 			tmpV16 |= FEN_ELDR;
 			rtw_write16(pAdapter, REG_SYS_FUNC_EN, tmpV16);
 		}
 
 		/*  Clock: Gated(0x0008h[5]) 8M(0x0008h[1]) clock from ANA, default valid */
-		tmpV16 = rtw_read16(pAdapter, REG_SYS_CLKR);
+		res = rtw_read16(pAdapter, REG_SYS_CLKR, &tmpV16);
+		if (res)
+			return;
+
 		if ((!(tmpV16 & LOADER_CLK_EN))  || (!(tmpV16 & ANA8M))) {
 			tmpV16 |= (LOADER_CLK_EN | ANA8M);
 			rtw_write16(pAdapter, REG_SYS_CLKR, tmpV16);
drivers/staging/r8188eu/hal/rtl8188e_phycfg.c +6 -2
index 985339a974fc..298c3d9bc7be 100644
@@ -484,13 +484,17 @@ PHY_BBConfig8188E(
 {
 	int	rtStatus = _SUCCESS;
 	struct hal_data_8188e *pHalData = &Adapter->haldata;
-	u32 RegVal;
+	u16 RegVal;
 	u8 CrystalCap;
+	int res;
 
 	phy_InitBBRFRegisterDefinition(Adapter);
 
 	/*  Enable BB and RF */
-	RegVal = rtw_read16(Adapter, REG_SYS_FUNC_EN);
+	res = rtw_read16(Adapter, REG_SYS_FUNC_EN, &RegVal);
+	if (res)
+		return _FAIL;
+
 	rtw_write16(Adapter, REG_SYS_FUNC_EN, (u16)(RegVal | BIT(13) | BIT(0) | BIT(1)));
 
 	/*  20090923 Joseph: Advised by Steven and Jenyu. Power sequence before init RF. */
drivers/staging/r8188eu/hal/usb_halinit.c +23 -4
index 6e3c8af5c4e7..1a68e4f19dc2 100644
@@ -52,6 +52,8 @@ void rtl8188eu_interface_configure(struct adapter *adapt)
 u32 rtl8188eu_InitPowerOn(struct adapter *adapt)
 {
 	u16 value16;
+	int res;
+
 	/*  HW Power on sequence */
 	struct hal_data_8188e *haldata = &adapt->haldata;
 	if (haldata->bMacPwrCtrlOn)

@@ -65,7 +67,10 @@ u32 rtl8188eu_InitPowerOn(struct adapter *adapt)
 	rtw_write16(adapt, REG_CR, 0x00);  /* suggseted by zhouzhou, by page, 20111230 */
 
 		/*  Enable MAC DMA/WMAC/SCHEDULE/SEC block */
-	value16 = rtw_read16(adapt, REG_CR);
+	res = rtw_read16(adapt, REG_CR, &value16);
+	if (res)
+		return _FAIL;
+
 	value16 |= (HCI_TXDMA_EN | HCI_RXDMA_EN | TXDMA_EN | RXDMA_EN
 				| PROTOCOL_EN | SCHEDULE_EN | ENSEC | CALTMR_EN);
 	/*  for SDIO - Set CR bit10 to enable 32k calibration. Suggested by SD1 Gimmy. Added by tynli. 2011.08.31. */

@@ -166,7 +171,14 @@ static void _InitNormalChipRegPriority(struct adapter *Adapter, u16 beQ,
 				       u16 bkQ, u16 viQ, u16 voQ, u16 mgtQ,
 				       u16 hiQ)
 {
-	u16 value16	= (rtw_read16(Adapter, REG_TRXDMA_CTRL) & 0x7);
+	u16 value16;
+	int res;
+
+	res = rtw_read16(Adapter, REG_TRXDMA_CTRL, &value16);
+	if (res)
+		return;
+
+	value16 &= 0x7;
 
 	value16 |= _TXDMA_BEQ_MAP(beQ)	| _TXDMA_BKQ_MAP(bkQ) |
 		   _TXDMA_VIQ_MAP(viQ)	| _TXDMA_VOQ_MAP(voQ) |

@@ -630,7 +642,10 @@ u32 rtl8188eu_hal_init(struct adapter *Adapter)
 	/*  Hw bug which Hw initials RxFF boundary size to a value which is larger than the real Rx buffer size in 88E. */
 	/*  */
 	/*  Enable MACTXEN/MACRXEN block */
-	value16 = rtw_read16(Adapter, REG_CR);
+	res = rtw_read16(Adapter, REG_CR, &value16);
+	if (res)
+		return _FAIL;
+
 	value16 |= (MACTXEN | MACRXEN);
 	rtw_write8(Adapter, REG_CR, value16);
 

@@ -703,7 +718,11 @@ u32 rtl8188eu_hal_init(struct adapter *Adapter)
 	rtw_write16(Adapter, REG_TX_RPT_TIME, 0x3DF0);
 
 	/* enable tx DMA to drop the redundate data of packet */
-	rtw_write16(Adapter, REG_TXDMA_OFFSET_CHK, (rtw_read16(Adapter, REG_TXDMA_OFFSET_CHK) | DROP_DATA_EN));
+	res = rtw_read16(Adapter, REG_TXDMA_OFFSET_CHK, &value16);
+	if (res)
+		return _FAIL;
+
+	rtw_write16(Adapter, REG_TXDMA_OFFSET_CHK, (value16 | DROP_DATA_EN));
 
 	/*  2010/08/26 MH Merge from 8192CE. */
 	if (pwrctrlpriv->rf_pwrstate == rf_on) {
drivers/staging/r8188eu/hal/usb_ops_linux.c +9 -4
index f399a7fd8b97..7d62f1f3d26e 100644
@@ -103,16 +103,21 @@ int __must_check rtw_read8(struct adapter *adapter, u32 addr, u8 *data)
 	return usb_read(intf, value, data, 1);
 }
 
-u16 rtw_read16(struct adapter *adapter, u32 addr)
+int __must_check rtw_read16(struct adapter *adapter, u32 addr, u16 *data)
 {
 	struct io_priv *io_priv = &adapter->iopriv;
 	struct intf_hdl *intf = &io_priv->intf;
 	u16 value = addr & 0xffff;
-	__le16 data;
+	__le16 le_data;
+	int res;
 
-	usb_read(intf, value, &data, 2);
+	res = usb_read(intf, value, &le_data, 2);
+	if (res)
+		return res;
 
-	return le16_to_cpu(data);
+	*data = le16_to_cpu(le_data);
+
+	return 0;
 }
 
 u32 rtw_read32(struct adapter *adapter, u32 addr)
drivers/staging/r8188eu/include/rtw_io.h +1 -1
index 1198d3850a6d..ce3369e33d66 100644
@@ -221,7 +221,7 @@ void _rtw_attrib_read(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 void _rtw_attrib_write(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 
 int __must_check rtw_read8(struct adapter *adapter, u32 addr, u8 *data);
-u16 rtw_read16(struct adapter *adapter, u32 addr);
+int __must_check rtw_read16(struct adapter *adapter, u32 addr, u16 *data);
 u32 rtw_read32(struct adapter *adapter, u32 addr);
 void _rtw_read_mem(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 u32 rtw_read_port(struct adapter *adapter, u8 *pmem);
drivers/staging/r8188eu/os_dep/ioctl_linux.c +8 -3
index 66aac2cbe3a9..1b35951a53cb 100644
@@ -1919,7 +1919,10 @@ static int rtw_wx_read32(struct net_device *dev,
 		sprintf(extra, "0x%02X", data32 & 0xff);
 		break;
 	case 2:
-		data32 = rtw_read16(padapter, addr);
+		ret = rtw_read16(padapter, addr, (u16 *) &data32);
+		if (ret)
+			goto err_free_ptmp;
+
 		sprintf(extra, "0x%04X", data32);
 		break;
 	case 4:

@@ -3441,8 +3444,9 @@ static int rtw_dbg_port(struct net_device *dev,
 			u16 reg = arg;
 			u16 start_value = 200;
 			u32 write_num = extra_arg;
+			u16 val16;
 
-			int i;
+			int i, res;
 			struct xmit_frame	*xmit_frame;
 
 			xmit_frame = rtw_IOL_accquire_xmit_frame(padapter);

@@ -3456,7 +3460,8 @@ static int rtw_dbg_port(struct net_device *dev,
 			if (rtl8188e_IOL_exec_cmds_sync(padapter, xmit_frame, 5000, 0) != _SUCCESS)
 				ret = -EPERM;
 
-			rtw_read16(padapter, reg);
+			/* FIXME: is this read necessary? */
+			res = rtw_read16(padapter, reg, &val16);
 		}
 			break;
 		case 0x08: /* continuous write dword test */
drivers/staging/r8188eu/os_dep/os_intfs.c +5 -1
index 891c85b088ca..d9325ef6ac28 100644
@@ -740,12 +740,16 @@ static void rtw_fifo_cleanup(struct adapter *adapter)
 {
 	struct pwrctrl_priv *pwrpriv = &adapter->pwrctrlpriv;
 	u8 trycnt = 100;
+	int res;
 
 	/* pause tx */
 	rtw_write8(adapter, REG_TXPAUSE, 0xff);
 
 	/* keep sn */
-	adapter->xmitpriv.nqos_ssn = rtw_read16(adapter, REG_NQOS_SEQ);
+	/* FIXME: return an error to caller */
+	res = rtw_read16(adapter, REG_NQOS_SEQ, &adapter->xmitpriv.nqos_ssn);
+	if (res)
+		return;
 
 	if (!pwrpriv->bkeepfwalive) {
 		/* RX DMA stop */
Signature · 1 line
2.36.1
author
#8 lore raw

PATCH 3/4 staging: r8188eu: add error handling of rtw_read32

In reply to Pavel Skripkin

rtw_read32() reads data from device via USB API which may fail. In case of any failure previous code returned stack data to callers, which is wrong.

Fix it by changing rtw_read32() prototype and prevent caller from touching random stack data

Signed-off-by Pavel Skripkin <[email protected]>
---
 drivers/staging/r8188eu/core/rtw_cmd.c        | 15 +++++-
 drivers/staging/r8188eu/core/rtw_efuse.c      | 19 +++++--
 drivers/staging/r8188eu/core/rtw_fw.c         | 16 ++++--
 drivers/staging/r8188eu/core/rtw_mlme_ext.c   | 13 ++++-
 drivers/staging/r8188eu/core/rtw_pwrctrl.c    |  9 +++-
 .../r8188eu/hal/Hal8188ERateAdaptive.c        | 21 ++++++--
 drivers/staging/r8188eu/hal/HalPhyRf_8188e.c  |  3 +-
 .../staging/r8188eu/hal/rtl8188e_hal_init.c   | 27 ++++++++--
 drivers/staging/r8188eu/hal/rtl8188e_phycfg.c | 12 ++++-
 drivers/staging/r8188eu/hal/usb_halinit.c     | 53 ++++++++++++++++---
 drivers/staging/r8188eu/hal/usb_ops_linux.c   | 13 +++--
 drivers/staging/r8188eu/include/rtw_io.h      |  2 +-
 drivers/staging/r8188eu/os_dep/ioctl_linux.c  | 32 ++++++++---
 drivers/staging/r8188eu/os_dep/os_intfs.c     | 13 ++++-
 14 files changed, 204 insertions(+), 44 deletions(-)
drivers/staging/r8188eu/core/rtw_cmd.c +13 -2
index 06523d91939a..5b6a891b5d67 100644
@@ -898,8 +898,12 @@ static void traffic_status_watchdog(struct adapter *padapter)
 static void rtl8188e_sreset_xmit_status_check(struct adapter *padapter)
 {
 	u32 txdma_status;
+	int res;
+
+	res = rtw_read32(padapter, REG_TXDMA_STATUS, &txdma_status);
+	if (res)
+		return;
 
-	txdma_status = rtw_read32(padapter, REG_TXDMA_STATUS);
 	if (txdma_status != 0x00)
 		rtw_write32(padapter, REG_TXDMA_STATUS, txdma_status);
 	/* total xmit irp = 4 */

@@ -1177,7 +1181,14 @@ u8 rtw_ps_cmd(struct adapter *padapter)
 
 static bool rtw_is_hi_queue_empty(struct adapter *adapter)
 {
-	return (rtw_read32(adapter, REG_HGQ_INFORMATION) & 0x0000ff00) == 0;
+	int res;
+	u32 reg;
+
+	res = rtw_read32(adapter, REG_HGQ_INFORMATION, &reg);
+	if (res)
+		return false;
+
+	return (reg & 0x0000ff00) == 0;
 }
 
 static void rtw_chk_hi_queue_hdl(struct adapter *padapter)
drivers/staging/r8188eu/core/rtw_efuse.c +15 -4
index a2691c7f96f6..7105122c2ba0 100644
@@ -47,9 +47,18 @@ ReadEFuseByte(
 
 	/* Check bit 32 read-ready */
 	retry = 0;
-	value32 = rtw_read32(Adapter, EFUSE_CTRL);
-	while (!(((value32 >> 24) & 0xff) & 0x80)  && (retry < 10000)) {
-		value32 = rtw_read32(Adapter, EFUSE_CTRL);
+	res = rtw_read32(Adapter, EFUSE_CTRL, &value32);
+	if (res)
+		return;
+
+	while (retry < 10000) {
+		res = rtw_read32(Adapter, EFUSE_CTRL, &value32);
+		if (res)
+			continue;
+
+		if (((value32 >> 24) & 0xff) & 0x80)
+			break;
+
 		retry++;
 	}
 

@@ -58,7 +67,9 @@ ReadEFuseByte(
 	/*  Designer says that there shall be some delay after ready bit is set, or the */
 	/*  result will always stay on last data we read. */
 	udelay(50);
-	value32 = rtw_read32(Adapter, EFUSE_CTRL);
+	res = rtw_read32(Adapter, EFUSE_CTRL, &value32);
+	if (res)
+		return;
 
 	*pbuf = (u8)(value32 & 0xff);
drivers/staging/r8188eu/core/rtw_fw.c +13 -3
index 701b033830bc..602b959437db 100644
@@ -194,10 +194,14 @@ static int fw_free_to_go(struct adapter *padapter)
 {
 	u32	counter = 0;
 	u32	value32;
+	int res;
 
 	/*  polling CheckSum report */
 	do {
-		value32 = rtw_read32(padapter, REG_MCUFWDL);
+		res = rtw_read32(padapter, REG_MCUFWDL, &value32);
+		if (res)
+			continue;
+
 		if (value32 & FWDL_CHKSUM_RPT)
 			break;
 	} while (counter++ < POLLING_READY_TIMEOUT_COUNT);

@@ -205,7 +209,10 @@ static int fw_free_to_go(struct adapter *padapter)
 	if (counter >= POLLING_READY_TIMEOUT_COUNT)
 		return _FAIL;
 
-	value32 = rtw_read32(padapter, REG_MCUFWDL);
+	res = rtw_read32(padapter, REG_MCUFWDL, &value32);
+	if (res)
+		return _FAIL;
+
 	value32 |= MCUFWDL_RDY;
 	value32 &= ~WINTINI_RDY;
 	rtw_write32(padapter, REG_MCUFWDL, value32);

@@ -215,7 +222,10 @@ static int fw_free_to_go(struct adapter *padapter)
 	/*  polling for FW ready */
 	counter = 0;
 	do {
-		value32 = rtw_read32(padapter, REG_MCUFWDL);
+		res = rtw_read32(padapter, REG_MCUFWDL, &value32);
+		if (res)
+			continue;
+
 		if (value32 & WINTINI_RDY)
 			return _SUCCESS;
 		udelay(5);
drivers/staging/r8188eu/core/rtw_mlme_ext.c +11 -2
index d4e59fab367c..e54d4139466d 100644
@@ -6023,6 +6023,7 @@ static void mlme_join(struct adapter *adapter, int type)
 	struct mlme_priv *mlmepriv = &adapter->mlmepriv;
 	u8 retry_limit = 0x30, reg;
 	int res;
+	u32 reg32;
 
 	switch (type) {
 	case 0:

@@ -6030,8 +6031,12 @@ static void mlme_join(struct adapter *adapter, int type)
 		/* enable to rx data frame, accept all data frame */
 		rtw_write16(adapter, REG_RXFLTMAP2, 0xFFFF);
 
+		res = rtw_read32(adapter, REG_RCR, &reg32);
+		if (res)
+			return;
+
 		rtw_write32(adapter, REG_RCR,
-			    rtw_read32(adapter, REG_RCR) | RCR_CBSSID_DATA | RCR_CBSSID_BCN);
+			    reg32 | RCR_CBSSID_DATA | RCR_CBSSID_BCN);
 
 		if (check_fwstate(mlmepriv, WIFI_STATION_STATE)) {
 			retry_limit = 48;

@@ -6844,10 +6849,14 @@ static u8 chk_ap_is_alive(struct sta_info *psta)
 
 static void rtl8188e_sreset_linked_status_check(struct adapter *padapter)
 {
-	u32 rx_dma_status =  rtw_read32(padapter, REG_RXDMA_STATUS);
+	u32 rx_dma_status;
 	int res;
 	u8 reg;
 
+	res = rtw_read32(padapter, REG_RXDMA_STATUS, &rx_dma_status);
+	if (res)
+		return;
+
 	if (rx_dma_status != 0x00)
 		rtw_write32(padapter, REG_RXDMA_STATUS, rx_dma_status);
drivers/staging/r8188eu/core/rtw_pwrctrl.c +8 -1
index 6990808ef353..1fe3d3d9cfb9 100644
@@ -229,6 +229,9 @@ void rtw_set_ps_mode(struct adapter *padapter, u8 ps_mode, u8 smart_ps, u8 bcn_a
 
 static bool lps_rf_on(struct adapter *adapter)
 {
+	int res;
+	u32 reg;
+
 	/* When we halt NIC, we should check if FW LPS is leave. */
 	if (adapter->pwrctrlpriv.rf_pwrstate == rf_off) {
 		/*  If it is in HW/SW Radio OFF or IPS state, we do not check Fw LPS Leave, */

@@ -236,7 +239,11 @@ static bool lps_rf_on(struct adapter *adapter)
 		return true;
 	}
 
-	if (rtw_read32(adapter, REG_RCR) & 0x00070000)
+	res = rtw_read32(adapter, REG_RCR, &reg);
+	if (res)
+		return false;
+
+	if (reg & 0x00070000)
 		return false;
 
 	return true;
drivers/staging/r8188eu/hal/Hal8188ERateAdaptive.c +17 -4
index 57e8f5573846..3cefdf90d6e0 100644
@@ -279,6 +279,7 @@ static int odm_ARFBRefresh_8188E(struct odm_dm_struct *dm_odm, struct odm_ra_inf
 {  /*  Wilson 2011/10/26 */
 	u32 MaskFromReg;
 	s8 i;
+	int res;
 
 	switch (pRaInfo->RateID) {
 	case RATR_INX_WIRELESS_NGB:

@@ -303,19 +304,31 @@ static int odm_ARFBRefresh_8188E(struct odm_dm_struct *dm_odm, struct odm_ra_inf
 		pRaInfo->RAUseRate = (pRaInfo->RateMask) & 0x0000000d;
 		break;
 	case 12:
-		MaskFromReg = rtw_read32(dm_odm->Adapter, REG_ARFR0);
+		res = rtw_read32(dm_odm->Adapter, REG_ARFR0, &MaskFromReg);
+		if (res)
+			return res;
+
 		pRaInfo->RAUseRate = (pRaInfo->RateMask) & MaskFromReg;
 		break;
 	case 13:
-		MaskFromReg = rtw_read32(dm_odm->Adapter, REG_ARFR1);
+		res = rtw_read32(dm_odm->Adapter, REG_ARFR1, &MaskFromReg);
+		if (res)
+			return res;
+
 		pRaInfo->RAUseRate = (pRaInfo->RateMask) & MaskFromReg;
 		break;
 	case 14:
-		MaskFromReg = rtw_read32(dm_odm->Adapter, REG_ARFR2);
+		res = rtw_read32(dm_odm->Adapter, REG_ARFR2, &MaskFromReg);
+		if (res)
+			return res;
+
 		pRaInfo->RAUseRate = (pRaInfo->RateMask) & MaskFromReg;
 		break;
 	case 15:
-		MaskFromReg = rtw_read32(dm_odm->Adapter, REG_ARFR3);
+		res = rtw_read32(dm_odm->Adapter, REG_ARFR3, &MaskFromReg);
+		if (res)
+			return res;
+
 		pRaInfo->RAUseRate = (pRaInfo->RateMask) & MaskFromReg;
 		break;
 	default:
drivers/staging/r8188eu/hal/HalPhyRf_8188e.c +2 -1
index aa4b4459329e..a97d9c3da16d 100644
@@ -482,7 +482,8 @@ static void _PHY_SaveMACRegisters(
 		MACBackup[i] = reg;
 	}
 
-	MACBackup[i] = rtw_read32(adapt, MACReg[i]);
+	/* FIXME: return an error to caller */
+	res = rtw_read32(adapt, MACReg[i], MACBackup + i);
 }
 
 static void reload_adda_reg(struct adapter *adapt, u32 *ADDAReg, u32 *ADDABackup, u32 RegiesterNum)
drivers/staging/r8188eu/hal/rtl8188e_hal_init.c +23 -4
index 22661c66cc18..f1dd60b30533 100644
@@ -215,6 +215,7 @@ static void efuse_read_phymap_from_txpktbuf(
 	u16 limit = *size;
 	u8 reg;
 	u8 *pos = content;
+	u32 reg32;
 
 	if (bcnhead < 0) { /* if not valid */
 		res = rtw_read8(adapter, REG_TDECTRL + 1, &reg);

@@ -245,8 +246,18 @@ static void efuse_read_phymap_from_txpktbuf(
 		} while (time_before(jiffies, timeout));
 
 		/* data from EEPROM needs to be in LE */
-		lo32 = cpu_to_le32(rtw_read32(adapter, REG_PKTBUF_DBG_DATA_L));
-		hi32 = cpu_to_le32(rtw_read32(adapter, REG_PKTBUF_DBG_DATA_H));
+		res = rtw_read32(adapter, REG_PKTBUF_DBG_DATA_L, &reg32);
+		if (res)
+			return;
+
+		lo32 = cpu_to_le32(reg32);
+
+
+		res = rtw_read32(adapter, REG_PKTBUF_DBG_DATA_H, &reg32);
+		if (res)
+			return;
+
+		hi32 = cpu_to_le32(reg32);
 
 		if (i == 0) {
 			int res;

@@ -544,8 +555,12 @@ void rtl8188e_read_chip_version(struct adapter *padapter)
 	u32				value32;
 	struct HAL_VERSION		ChipVersion;
 	struct hal_data_8188e *pHalData = &padapter->haldata;
+	int res;
+
+	res = rtw_read32(padapter, REG_SYS_CFG, &value32);
+	if (res)
+		return;
 
-	value32 = rtw_read32(padapter, REG_SYS_CFG);
 	ChipVersion.ChipType = ((value32 & RTL_ID) ? TEST_CHIP : NORMAL_CHIP);
 
 	ChipVersion.VendorType = ((value32 & VENDOR_ID) ? CHIP_VENDOR_UMC : CHIP_VENDOR_TSMC);

@@ -596,12 +611,16 @@ static s32 _LLTWrite(struct adapter *padapter, u32 address, u32 data)
 	s32	count = 0;
 	u32	value = _LLT_INIT_ADDR(address) | _LLT_INIT_DATA(data) | _LLT_OP(_LLT_WRITE_ACCESS);
 	u16	LLTReg = REG_LLT_INIT;
+	int res;
 
 	rtw_write32(padapter, LLTReg, value);
 
 	/* polling */
 	do {
-		value = rtw_read32(padapter, LLTReg);
+		res = rtw_read32(padapter, LLTReg, &value);
+		if (res)
+			continue;
+
 		if (_LLT_NO_ACTIVE == _LLT_OP_VALUE(value))
 			break;
drivers/staging/r8188eu/hal/rtl8188e_phycfg.c +10 -2
index 298c3d9bc7be..dea6d915a1f4 100644
@@ -56,8 +56,12 @@ rtl8188e_PHY_QueryBBReg(
 	)
 {
 	u32 ReturnValue = 0, OriginalValue, BitShift;
+	int res;
+
+	res = rtw_read32(Adapter, RegAddr, &OriginalValue);
+	if (res)
+		return 0;
 
-	OriginalValue = rtw_read32(Adapter, RegAddr);
 	BitShift = phy_CalculateBitShift(BitMask);
 	ReturnValue = (OriginalValue & BitMask) >> BitShift;
 	return ReturnValue;

@@ -84,9 +88,13 @@ rtl8188e_PHY_QueryBBReg(
 void rtl8188e_PHY_SetBBReg(struct adapter *Adapter, u32 RegAddr, u32 BitMask, u32 Data)
 {
 	u32 OriginalValue, BitShift;
+	int res;
 
 	if (BitMask != bMaskDWord) { /* if not "double word" write */
-		OriginalValue = rtw_read32(Adapter, RegAddr);
+		res = rtw_read32(Adapter, RegAddr, &OriginalValue);
+		if (res)
+			return;
+
 		BitShift = phy_CalculateBitShift(BitMask);
 		Data = ((OriginalValue & (~BitMask)) | (Data << BitShift));
 	}
drivers/staging/r8188eu/hal/usb_halinit.c +45 -8

124 lines not on this page · raw message

drivers/staging/r8188eu/hal/usb_ops_linux.c +9 -4
index 7d62f1f3d26e..c1a4d023f627 100644
@@ -120,16 +120,21 @@ int __must_check rtw_read16(struct adapter *adapter, u32 addr, u16 *data)
 	return 0;
 }
 
-u32 rtw_read32(struct adapter *adapter, u32 addr)
+int __must_check rtw_read32(struct adapter *adapter, u32 addr, u32 *data)
 {
 	struct io_priv *io_priv = &adapter->iopriv;
 	struct intf_hdl *intf = &io_priv->intf;
 	u16 value = addr & 0xffff;
-	__le32 data;
+	__le32 le_data;
+	int res;
 
-	usb_read(intf, value, &data, 4);
+	res = usb_read(intf, value, &le_data, 4);
+	if (res)
+		return res;
 
-	return le32_to_cpu(data);
+	*data = le32_to_cpu(le_data);
+
+	return 0;
 }
 
 int rtw_write8(struct adapter *adapter, u32 addr, u8 val)
drivers/staging/r8188eu/include/rtw_io.h +1 -1
index ce3369e33d66..1c6097367a67 100644
@@ -222,7 +222,7 @@ void _rtw_attrib_write(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 
 int __must_check rtw_read8(struct adapter *adapter, u32 addr, u8 *data);
 int __must_check rtw_read16(struct adapter *adapter, u32 addr, u16 *data);
-u32 rtw_read32(struct adapter *adapter, u32 addr);
+int __must_check rtw_read32(struct adapter *adapter, u32 addr, u32 *data);
 void _rtw_read_mem(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 u32 rtw_read_port(struct adapter *adapter, u8 *pmem);
 void rtw_read_port_cancel(struct adapter *adapter);
drivers/staging/r8188eu/os_dep/ioctl_linux.c +26 -6
index 1b35951a53cb..00d1ba62c248 100644
@@ -1926,7 +1926,10 @@ static int rtw_wx_read32(struct net_device *dev,
 		sprintf(extra, "0x%04X", data32);
 		break;
 	case 4:
-		data32 = rtw_read32(padapter, addr);
+		ret = rtw_read32(padapter, addr, &data32);
+		if (ret)
+			goto err_free_ptmp;
+
 		sprintf(extra, "0x%08X", data32);
 		break;
 	default:

@@ -3213,18 +3216,29 @@ static int rtw_rereg_nd_name(struct net_device *dev,
 static void mac_reg_dump(struct adapter *padapter)
 {
 	int i, j = 1;
+	u32 reg;
+	int res;
+
 	pr_info("\n ======= MAC REG =======\n");
 	for (i = 0x0; i < 0x300; i += 4) {
 		if (j % 4 == 1)
 			pr_info("0x%02x", i);
-		pr_info(" 0x%08x ", rtw_read32(padapter, i));
+
+		res = rtw_read32(padapter, i, &reg);
+		if (!res)
+			pr_info(" 0x%08x ", reg);
+
 		if ((j++) % 4 == 0)
 			pr_info("\n");
 	}
 	for (i = 0x400; i < 0x800; i += 4) {
 		if (j % 4 == 1)
 			pr_info("0x%02x", i);
-		pr_info(" 0x%08x ", rtw_read32(padapter, i));
+
+		res = rtw_read32(padapter, i, &reg);
+		if (!res)
+			pr_info(" 0x%08x ", reg);
+
 		if ((j++) % 4 == 0)
 			pr_info("\n");
 	}

@@ -3232,13 +3246,18 @@ static void mac_reg_dump(struct adapter *padapter)
 
 static void bb_reg_dump(struct adapter *padapter)
 {
-	int i, j = 1;
+	int i, j = 1, res;
+	u32 reg;
+
 	pr_info("\n ======= BB REG =======\n");
 	for (i = 0x800; i < 0x1000; i += 4) {
 		if (j % 4 == 1)
 			pr_info("0x%02x", i);
 
-		pr_info(" 0x%08x ", rtw_read32(padapter, i));
+		res = rtw_read32(padapter, i, &reg);
+		if (!res)
+			pr_info(" 0x%08x ", reg);
+
 		if ((j++) % 4 == 0)
 			pr_info("\n");
 	}

@@ -3484,7 +3503,8 @@ static int rtw_dbg_port(struct net_device *dev,
 			if (rtl8188e_IOL_exec_cmds_sync(padapter, xmit_frame, 5000, 0) != _SUCCESS)
 				ret = -EPERM;
 
-			rtw_read32(padapter, reg);
+			/* FIXME: is this read necessary? */
+			ret = rtw_read32(padapter, reg, &write_num);
 		}
 			break;
 		}
drivers/staging/r8188eu/os_dep/os_intfs.c +11 -2
index d9325ef6ac28..cac9553666e6 100644
@@ -741,6 +741,7 @@ static void rtw_fifo_cleanup(struct adapter *adapter)
 	struct pwrctrl_priv *pwrpriv = &adapter->pwrctrlpriv;
 	u8 trycnt = 100;
 	int res;
+	u32 reg;
 
 	/* pause tx */
 	rtw_write8(adapter, REG_TXPAUSE, 0xff);

@@ -753,10 +754,18 @@ static void rtw_fifo_cleanup(struct adapter *adapter)
 
 	if (!pwrpriv->bkeepfwalive) {
 		/* RX DMA stop */
+		res = rtw_read32(adapter, REG_RXPKT_NUM, &reg);
+		if (res)
+			return;
+
 		rtw_write32(adapter, REG_RXPKT_NUM,
-			    (rtw_read32(adapter, REG_RXPKT_NUM) | RW_RELEASE_EN));
+			    (reg | RW_RELEASE_EN));
 		do {
-			if (!(rtw_read32(adapter, REG_RXPKT_NUM) & RXDMA_IDLE))
+			res = rtw_read32(adapter, REG_RXPKT_NUM, &reg);
+			if (res)
+				continue;
+
+			if (!(reg & RXDMA_IDLE))
 				break;
 		} while (trycnt--);
Signature · 1 line
2.36.1
author
#11 lore raw

PATCH 4/4 MAINTAINERS: add myself as r8188eu reviewer

In reply to Pavel Skripkin

I was reviewing r8188eu patches for a while, but I am missing some of them, since I am not in CC list. I want to be CC'ed to help reviewing and testing more patches.

Signed-off-by Pavel Skripkin <[email protected]>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)
MAINTAINERS +1 -0
index 0a635f2ae5b9..bd38a2e465cc 100644
@@ -18716,6 +18716,7 @@ F:	drivers/staging/olpc_dcon/
 STAGING - REALTEK RTL8188EU DRIVERS
 M:	Larry Finger <[email protected]>
 M:	Phillip Potter <[email protected]>
+R:	Pavel Skripkin <[email protected]>
 S:	Supported
 F:	drivers/staging/r8188eu/
Signature · 1 line
2.36.1
build bot
#2 lore raw

Re: [PATCH 1/4] staging: r8188eu: add error handling of rtw_read8

In reply to Pavel Skripkin
Thank you for the patch! Perhaps something to improve:

Hi Pavel,

Thank you for the patch! Perhaps something to improve:

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

url:    https://github.com/intel-lab-lkp/linux/commits/Pavel-Skripkin/staging-r8188eu-add-error-handling-of-usb-read-errors/20220519-061342
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git e41f7a5521d7f03dca99e3207633df71740569dd
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20220519/[email protected]/config)
compiler: m68k-linux-gcc (GCC) 11.3.0

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/f8c9bd199a25d7a1d8f6fed1b0d5da9cec1f8faa
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Pavel-Skripkin/staging-r8188eu-add-error-handling-of-usb-read-errors/20220519-061342 git checkout f8c9bd199a25d7a1d8f6fed1b0d5da9cec1f8faa
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/staging/r8188eu/

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

drivers/staging/r8188eu/hal/usb_halinit.c: In function '_InitBeaconParameters':

>> drivers/staging/r8188eu/hal/usb_halinit.c:465:13: warning: variable 'res' set but not used [-Wunused-but-set-variable]
     465 |         int res;
         |             ^~~
--
   drivers/staging/r8188eu/os_dep/ioctl_linux.c: In function 'rtw_dbg_port':
>> drivers/staging/r8188eu/os_dep/ioctl_linux.c:3419:32: warning: variable 'res' set but not used [-Wunused-but-set-variable]
    3419 |                         int i, res;
         |                                ^~~

vim +/res +465 drivers/staging/r8188eu/hal/usb_halinit.c

461
462 static void _InitBeaconParameters(struct adapter *Adapter)
463 {
464 struct hal_data_8188e *haldata = &Adapter->haldata;

 > 465		int res;

466
467 rtw_write16(Adapter, REG_BCN_CTRL, 0x1010);
468
469 /* TODO: Remove these magic number */
470 rtw_write16(Adapter, REG_TBTT_PROHIBIT, 0x6404);/* ms */
471 rtw_write8(Adapter, REG_DRVERLYINT, DRIVER_EARLY_INT_TIME);/* 5ms */
472 rtw_write8(Adapter, REG_BCNDMATIM, BCN_DMA_ATIME_INT_TIME); /* 2ms */
473
474 /* Suggested by designer timchen. Change beacon AIFS to the largest number */
475 /* beacause test chip does not contension before sending beacon. by tynli. 2009.11.03 */
476 rtw_write16(Adapter, REG_BCNTCFG, 0x660F);
477
478 /* FIXME: return an error to caller */
479 res = rtw_read8(Adapter, REG_FWHW_TXQ_CTRL + 2, &haldata->RegFwHwTxQCtrl);
480 res = rtw_read8(Adapter, REG_TBTT_PROHIBIT + 2, &haldata->RegReg542);
481 res = rtw_read8(Adapter, REG_CR + 1, &haldata->RegCR_1);
482 }
483

Signature · 2 lines
0-DAY CI Kernel Test Service
https://01.org/lkp
#3 lore raw

Re: [PATCH 1/4] staging: r8188eu: add error handling of rtw_read8

In reply to Pavel Skripkin
Quoted 16 lines · Pavel Skripkin
drivers/staging/r8188eu/core/rtw_fw.c +2 -0
@@ -240,12 +259,14 @@ int rtl8188e_firmware_download(struct adapter *padapter)
 {
 	int ret = _SUCCESS;
 	u8 write_fw_retry = 0;
+	u8 reg;
 	unsigned long fwdl_timeout;
 	struct dvobj_priv *dvobj = adapter_to_dvobj(padapter);
 	struct device *device = dvobj_to_dev(dvobj);
 	struct rt_firmware_hdr *fwhdr = NULL;
 	u8 *fw_data;
 	u32 fw_size;
+	int res;
 
 	if (!dvobj->firmware.data)
 		ret = load_firmware(&dvobj->firmware, device);

drivers/staging/r8188eu/core/rtw_fw.c int rtl8188e_firmware_download(struct adapter *padapter) · line 295

Quoted 7 lines
drivers/staging/r8188eu/core/rtw_fw.c +3 -1
@@ -269,7 +290,11 @@ int rtl8188e_firmware_download(struct adapter *padapter)
 
 	/*  Suggested by Filen. If 8051 is running in RAM code, driver should inform Fw to reset by itself, */
 	/*  or it will cause download Fw fail. 2010.02.01. by tynli. */
-	if (rtw_read8(padapter, REG_MCUFWDL) & RAM_DL_SEL) { /* 8051 RAM code */
+	res = rtw_read8(padapter, REG_MCUFWDL, &reg);
+	if (res)
+		goto exit;

You didn't introduce this bug, but this path needs to have an error code set. Also we really need to get rid of the _FAIL garbage. When I saw this, I got "ret" and "res" mixed up so I thought we were returning
negative error codes instead of _FAIL. That would But then I saw we
are returning success.

Quoted 6 lines
drivers/staging/r8188eu/core/rtw_fw.c +2 -0
+
+	if (reg & RAM_DL_SEL) { /* 8051 RAM code */
 		rtw_write8(padapter, REG_MCUFWDL, 0x00);
 		rtw_reset_8051(padapter);
 	}

drivers/staging/r8188eu/core/rtw_fw.c int rtl8188e_firmware_download(struct adapter *padapter) · line 311

Quoted 10 lines
drivers/staging/r8188eu/core/rtw_fw.c +6 -1
@@ -278,7 +303,14 @@ int rtl8188e_firmware_download(struct adapter *padapter)
 	fwdl_timeout = jiffies + msecs_to_jiffies(500);
 	while (1) {
 		/* reset the FWDL chksum */
-		rtw_write8(padapter, REG_MCUFWDL, rtw_read8(padapter, REG_MCUFWDL) | FWDL_CHKSUM_RPT);
+		res = rtw_read8(padapter, REG_MCUFWDL, &reg);
+		if (res == -ENODEV)
+			break;
+
+		if (res)
+			continue;

This continue is wrong. If res = -EPERM then it's a forever loop. Let's just break for every error.

Quoted 10 lines
drivers/staging/r8188eu/core/rtw_fw.c +2 -0
+
+		rtw_write8(padapter, REG_MCUFWDL, reg | FWDL_CHKSUM_RPT);
 
 		ret = write_fw(padapter, fw_data, fw_size);
drivers/staging/r8188eu/core/rtw_led.c +0 -0
index 2f3000428af7..b532e614c5b6 100644

drivers/staging/r8188eu/core/rtw_led.c static void ResetLedStatus(struct LED_871x *pLed) · line 37

Quoted 5 lines
drivers/staging/r8188eu/core/rtw_led.c +1 -1
@@ -34,28 +34,38 @@ static void ResetLedStatus(struct LED_871x *pLed)
 
 static void SwLedOn(struct adapter *padapter, struct LED_871x *pLed)
 {
-	u8	LedCfg;
+	u8 LedCfg;

Please don't make unrelated changes.

regards,
dan carpenter

#7 lore raw

Re: [PATCH 2/4] staging: r8188eu: add error handling of rtw_read16

In reply to Pavel Skripkin

drivers/staging/r8188eu/hal/rtl8188e_hal_init.c static void efuse_read_phymap_from_txpktbuf( · line 254

Quoted 11 lines · Pavel Skripkin
drivers/staging/r8188eu/hal/rtl8188e_hal_init.c +2 -0
index e67ecbd1ba79..22661c66cc18 100644
@@ -249,11 +249,14 @@ static void efuse_read_phymap_from_txpktbuf(
 		hi32 = cpu_to_le32(rtw_read32(adapter, REG_PKTBUF_DBG_DATA_H));
 
 		if (i == 0) {
+			int res;
+			u16 reg;
 			/* Although lenc is only used in a debug statement,

Blank line after declarations.

I think it's better to put "int res" declarations at the start of the function. That's where people will expect to see it.

Quoted 9 lines
drivers/staging/r8188eu/hal/rtl8188e_hal_init.c +2 -1
 			 * do not remove it as the rtw_read16() call consumes
 			 * 2 bytes from the EEPROM source.
 			 */
-			rtw_read16(adapter, REG_PKTBUF_DBG_DATA_L);
+			res = rtw_read16(adapter, REG_PKTBUF_DBG_DATA_L, &reg);
+			(void) res;
 
 			len = le32_to_cpu(lo32) & 0x0000ffff;

[ snip ]

Quoted 18 lines
drivers/staging/r8188eu/include/rtw_io.h +1 -1
index 1198d3850a6d..ce3369e33d66 100644
@@ -221,7 +221,7 @@ void _rtw_attrib_read(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 void _rtw_attrib_write(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 
 int __must_check rtw_read8(struct adapter *adapter, u32 addr, u8 *data);
-u16 rtw_read16(struct adapter *adapter, u32 addr);
+int __must_check rtw_read16(struct adapter *adapter, u32 addr, u16 *data);
 u32 rtw_read32(struct adapter *adapter, u32 addr);
 void _rtw_read_mem(struct adapter *adapter, u32 addr, u32 cnt, u8 *pmem);
 u32 rtw_read_port(struct adapter *adapter, u8 *pmem);
drivers/staging/r8188eu/os_dep/ioctl_linux.c +0 -0
index 66aac2cbe3a9..1b35951a53cb 100644

drivers/staging/r8188eu/os_dep/ioctl_linux.c static int rtw_wx_read32(struct net_device *dev, · line 1922

Quoted 5 lines
drivers/staging/r8188eu/os_dep/ioctl_linux.c +1 -1
@@ -1919,7 +1919,10 @@ static int rtw_wx_read32(struct net_device *dev,
 		sprintf(extra, "0x%02X", data32 & 0xff);
 		break;
 	case 2:
-		data32 = rtw_read16(padapter, addr);
+		ret = rtw_read16(padapter, addr, (u16 *) &data32);

Checkpatch.

I have an unpublished Smatch warning for casts like this. You can't pass a data32 pointer to something which is takes a u16 pointer and expect it to work. The last two bytes are uninitialized.

And even if you zero out the bytes, it is a bug on big endian systems.

drivers/staging/r8188eu/os_dep/ioctl_linux.c static int rtw_wx_read32(struct net_device *dev, · line 1928

Quoted 6 lines
drivers/staging/r8188eu/os_dep/ioctl_linux.c +3 -0
+		if (ret)
+			goto err_free_ptmp;
+
 		sprintf(extra, "0x%04X", data32);
 		break;
 	case 4:

regards,
dan carpenter

author
#4 lore raw

Re: [PATCH 1/4] staging: r8188eu: add error handling of rtw_read8

In reply to Dan Carpenter

Hi Dan,

Quoted 18 lines · Dan Carpenter
Quoted 17 lines · Pavel Skripkin
drivers/staging/r8188eu/core/rtw_fw.c +2 -0
@@ -240,12 +259,14 @@ int rtl8188e_firmware_download(struct adapter *padapter)
 {
 	int ret = _SUCCESS;
 	u8 write_fw_retry = 0;
+	u8 reg;
 	unsigned long fwdl_timeout;
 	struct dvobj_priv *dvobj = adapter_to_dvobj(padapter);
 	struct device *device = dvobj_to_dev(dvobj);
 	struct rt_firmware_hdr *fwhdr = NULL;
 	u8 *fw_data;
 	u32 fw_size;
+	int res;
 
 	if (!dvobj->firmware.data)
 		ret = load_firmware(&dvobj->firmware, device);

@@ -269,7 +290,11 @@ int rtl8188e_firmware_download(struct adapter *padapter)
Quoted 13 lines

drivers/staging/r8188eu/core/rtw_fw.c int rtl8188e_firmware_download(struct adapter *padapter) · line 295

Quoted 6 lines
drivers/staging/r8188eu/core/rtw_fw.c +3 -1
 	/*  Suggested by Filen. If 8051 is running in RAM code, driver should inform Fw to reset by itself, */
 	/*  or it will cause download Fw fail. 2010.02.01. by tynli. */
-	if (rtw_read8(padapter, REG_MCUFWDL) & RAM_DL_SEL) { /* 8051 RAM code */
+	res = rtw_read8(padapter, REG_MCUFWDL, &reg);
+	if (res)
+		goto exit;

You didn't introduce this bug, but this path needs to have an error code set. Also we really need to get rid of the _FAIL garbage. When I saw this, I got "ret" and "res" mixed up so I thought we were returning
negative error codes instead of _FAIL. That would But then I saw we
are returning success.

I see now, that 'res' and 'ret' got mixed up in my mind too. Will fix up

Quoted 11 lines
Quoted 11 lines
drivers/staging/r8188eu/core/rtw_fw.c +3 -1
+
+	if (reg & RAM_DL_SEL) { /* 8051 RAM code */
 		rtw_write8(padapter, REG_MCUFWDL, 0x00);
 		rtw_reset_8051(padapter);
 	}

@@ -278,7 +303,14 @@ int rtl8188e_firmware_download(struct adapter *padapter)
 	fwdl_timeout = jiffies + msecs_to_jiffies(500);
 	while (1) {
 		/* reset the FWDL chksum */
-		rtw_write8(padapter, REG_MCUFWDL, rtw_read8(padapter, REG_MCUFWDL) | FWDL_CHKSUM_RPT);
+		res = rtw_read8(padapter, REG_MCUFWDL, &reg);
Quoted 9 lines

drivers/staging/r8188eu/core/rtw_fw.c int rtl8188e_firmware_download(struct adapter *padapter) · line 311

Quoted 5 lines
drivers/staging/r8188eu/core/rtw_fw.c +5 -0
+		if (res == -ENODEV)
+			break;
+
+		if (res)
+			continue;

This continue is wrong. If res = -EPERM then it's a forever loop. Let's just break for every error.

I was trying to avoid strict breaking the loop on any error, since I am afraid this might break the driver.

What about:

	do {
		/* reset the FWDL chksum */
		ret = rtw_read8(padapter, REG_MCUFWDL, &reg);
		if (ret == -ENODEV || ret == -EPERM)
			break;

		if (ret) {
			ret == _FAIL;
			continue;
		}

		rtw_write8(padapter, REG_MCUFWDL, reg | FWDL_CHKSUM_RPT);

		ret = write_fw(padapter, fw_data, fw_size);
	} while (!(ret == _SUCCESS ||
		    (time_after(jiffies, fwdl_timeout) && write_fw_retry++ >= 3)))

The idea is to break only on fatal errors to make things less strict

With regards,
Pavel Skripkin

#9 lore raw

Re: [PATCH 3/4] staging: r8188eu: add error handling of rtw_read32

In reply to Pavel Skripkin
Quoted 9 lines · Pavel Skripkin
drivers/staging/r8188eu/core/rtw_efuse.c +0 -1
index a2691c7f96f6..7105122c2ba0 100644
@@ -47,9 +47,18 @@ ReadEFuseByte(
 
 	/* Check bit 32 read-ready */
 	retry = 0;
-	value32 = rtw_read32(Adapter, EFUSE_CTRL);

drivers/staging/r8188eu/core/rtw_efuse.c ReadEFuseByte( · line 57

Quoted 10 lines
drivers/staging/r8188eu/core/rtw_efuse.c +8 -2
-	while (!(((value32 >> 24) & 0xff) & 0x80)  && (retry < 10000)) {
-		value32 = rtw_read32(Adapter, EFUSE_CTRL);
+	res = rtw_read32(Adapter, EFUSE_CTRL, &value32);
+	if (res)
+		return;
+
+	while (retry < 10000) {
+		res = rtw_read32(Adapter, EFUSE_CTRL, &value32);
+		if (res)
+			continue;

Forever loop. Always put the ++ in side the while (). Apparently, Smatch does not catch this. #Idea #Oppurtunity

Quoted 6 lines
drivers/staging/r8188eu/core/rtw_efuse.c +4 -0
+
+		if (((value32 >> 24) & 0xff) & 0x80)
+			break;
+
 		retry++;
 	}

[ snip ]

drivers/staging/r8188eu/core/rtw_efuse.c static int fw_free_to_go(struct adapter *padapter) · line 231

Quoted 12 lines
drivers/staging/r8188eu/core/rtw_efuse.c +4 -1
@@ -215,7 +222,10 @@ static int fw_free_to_go(struct adapter *padapter)
 	/*  polling for FW ready */
 	counter = 0;
 	do {
-		value32 = rtw_read32(padapter, REG_MCUFWDL);
+		res = rtw_read32(padapter, REG_MCUFWDL, &value32);
+		if (res)
+			continue;
+
 		if (value32 & WINTINI_RDY)
 			return _SUCCESS;
 		udelay(5);

You really want to do this delay on each iteration. So write it like so:

		res = rtw_read32(padapter, REG_MCUFWDL, &value32);
		if (!res && value32 & WINTINI_RDY)
			return _SUCCESS;
		udelay(5);

drivers/staging/r8188eu/core/rtw_mlme_ext.c static void mlme_join(struct adapter *adapter, int type) · line 6026

Quoted 9 lines
drivers/staging/r8188eu/core/rtw_mlme_ext.c +1 -0
index d4e59fab367c..e54d4139466d 100644
@@ -6023,6 +6023,7 @@ static void mlme_join(struct adapter *adapter, int type)
 	struct mlme_priv *mlmepriv = &adapter->mlmepriv;
 	u8 retry_limit = 0x30, reg;
 	int res;
+	u32 reg32;

The reg32 should got before the res so it's in reverse Christmas tree order.

[ snip ]

drivers/staging/r8188eu/core/rtw_mlme_ext.c static void efuse_read_phymap_from_txpktbuf( · line 255

Quoted 13 lines
drivers/staging/r8188eu/core/rtw_mlme_ext.c +7 -2
@@ -245,8 +246,18 @@ static void efuse_read_phymap_from_txpktbuf(
 		} while (time_before(jiffies, timeout));
 
 		/* data from EEPROM needs to be in LE */
-		lo32 = cpu_to_le32(rtw_read32(adapter, REG_PKTBUF_DBG_DATA_L));
-		hi32 = cpu_to_le32(rtw_read32(adapter, REG_PKTBUF_DBG_DATA_H));
+		res = rtw_read32(adapter, REG_PKTBUF_DBG_DATA_L, &reg32);
+		if (res)
+			return;
+
+		lo32 = cpu_to_le32(reg32);
+
+

Double blank line. Checkpatch?

Quoted 4 lines
drivers/staging/r8188eu/core/rtw_mlme_ext.c +0 -0
@@ -596,12 +611,16 @@ static s32 _LLTWrite(struct adapter *padapter, u32 address, u32 data)
 	s32	count = 0;
 	u32	value = _LLT_INIT_ADDR(address) | _LLT_INIT_DATA(data) | _LLT_OP(_LLT_WRITE_ACCESS);
 	u16	LLTReg = REG_LLT_INIT;

drivers/staging/r8188eu/core/rtw_mlme_ext.c static s32 _LLTWrite(struct adapter *padapter, u32 address, u32 data) · line 622

Quoted 10 lines
drivers/staging/r8188eu/core/rtw_mlme_ext.c +4 -1
+	int res;
 
 	rtw_write32(padapter, LLTReg, value);
 
 	/* polling */
 	do {
-		value = rtw_read32(padapter, LLTReg);
+		res = rtw_read32(padapter, LLTReg, &value);
+		if (res)
+			continue;

This continue has the potential to lead to a forever loop. The limit check needs to be a part of the do while() condition. Probably send that patch first, by itself as a clean up before adding this continue.

drivers/staging/r8188eu/core/rtw_mlme_ext.c static s32 _LLTWrite(struct adapter *padapter, u32 address, u32 data) · line 626

Quoted 4 lines
drivers/staging/r8188eu/core/rtw_mlme_ext.c +1 -0
+
 		if (_LLT_NO_ACTIVE == _LLT_OP_VALUE(value))
 			break;

regards,
dan carpenter

Acked-by on v1 4/4
#12 lore raw

Re: [PATCH 4/4] MAINTAINERS: add myself as r8188eu reviewer

In reply to Pavel Skripkin
Quoted 14 lines · Pavel Skripkin

I was reviewing r8188eu patches for a while, but I am missing some of them, since I am not in CC list. I want to be CC'ed to help reviewing and testing more patches.

Signed-off-by Pavel Skripkin <[email protected]>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)
MAINTAINERS +0 -0
index 0a635f2ae5b9..bd38a2e465cc 100644

MAINTAINERS F: drivers/staging/olpc_dcon/ · line 18721

Quoted 6 lines
MAINTAINERS +1 -0
@@ -18716,6 +18716,7 @@ F:	drivers/staging/olpc_dcon/
 STAGING - REALTEK RTL8188EU DRIVERS
 M:	Larry Finger <[email protected]>
 M:	Phillip Potter <[email protected]>
+R:	Pavel Skripkin <[email protected]>
 S:	Supported
 F:	drivers/staging/r8188eu/
Acked-by Dan Carpenter <[email protected]>

regards,
dan carpenter

author
#10 lore raw

Re: [PATCH 3/4] staging: r8188eu: add error handling of rtw_read32

In reply to Dan Carpenter

Hi Dan,

Quoted 7 lines · Dan Carpenter
Quoted 6 lines · Pavel Skripkin
drivers/staging/r8188eu/core/rtw_efuse.c +0 -0
index a2691c7f96f6..7105122c2ba0 100644
@@ -47,9 +47,18 @@ ReadEFuseByte(
Quoted 17 lines

drivers/staging/r8188eu/core/rtw_efuse.c ReadEFuseByte( · line 57

Quoted 13 lines
drivers/staging/r8188eu/core/rtw_efuse.c +8 -3
 	/* Check bit 32 read-ready */
 	retry = 0;
-	value32 = rtw_read32(Adapter, EFUSE_CTRL);
-	while (!(((value32 >> 24) & 0xff) & 0x80)  && (retry < 10000)) {
-		value32 = rtw_read32(Adapter, EFUSE_CTRL);
+	res = rtw_read32(Adapter, EFUSE_CTRL, &value32);
+	if (res)
+		return;
+
+	while (retry < 10000) {
+		res = rtw_read32(Adapter, EFUSE_CTRL, &value32);
+		if (res)
+			continue;

Forever loop. Always put the ++ in side the while (). Apparently, Smatch does not catch this. #Idea #Oppurtunity

I have missed it every single loop... :(

That's why I don't like 'while' loops, 'for' suits much better for this kind of things.

Thanks you for your review! Will address your comments in next version

With regards,
Pavel Skripkin

#5 lore raw

Re: [PATCH 1/4] staging: r8188eu: add error handling of rtw_read8

In reply to Pavel Skripkin
Quoted 43 lines · Pavel Skripkin
Quoted 11 lines
Quoted 11 lines
drivers/staging/r8188eu/core/rtw_fw.c +3 -1
+
+	if (reg & RAM_DL_SEL) { /* 8051 RAM code */
 		rtw_write8(padapter, REG_MCUFWDL, 0x00);
 		rtw_reset_8051(padapter);
 	}

@@ -278,7 +303,14 @@ int rtl8188e_firmware_download(struct adapter *padapter)
 	fwdl_timeout = jiffies + msecs_to_jiffies(500);
 	while (1) {
 		/* reset the FWDL chksum */
-		rtw_write8(padapter, REG_MCUFWDL, rtw_read8(padapter, REG_MCUFWDL) | FWDL_CHKSUM_RPT);
+		res = rtw_read8(padapter, REG_MCUFWDL, &reg);
Quoted 9 lines

drivers/staging/r8188eu/core/rtw_fw.c int rtl8188e_firmware_download(struct adapter *padapter) · line 311

Quoted 5 lines
drivers/staging/r8188eu/core/rtw_fw.c +5 -0
+		if (res == -ENODEV)
+			break;
+
+		if (res)
+			continue;

This continue is wrong. If res = -EPERM then it's a forever loop. Let's just break for every error.

I was trying to avoid strict breaking the loop on any error, since I am afraid this might break the driver.

What about:

	do {
		/* reset the FWDL chksum */
		ret = rtw_read8(padapter, REG_MCUFWDL, &reg);
		if (ret == -ENODEV || ret == -EPERM)
			break;

		if (ret) {
			ret == _FAIL;
			continue;
		}

		rtw_write8(padapter, REG_MCUFWDL, reg | FWDL_CHKSUM_RPT);

		ret = write_fw(padapter, fw_data, fw_size);
	} while (!(ret == _SUCCESS ||
		    (time_after(jiffies, fwdl_timeout) && write_fw_retry++ >= 3)))
Quoted 2 lines

The idea is to break only on fatal errors to make things less strict

This is too complicated.

Treat all the errors the same, and use one time out condition. Either based on the jiffies or the retry count.

regards,
dan carpenter