LinuxLists.cc - [PATCH 0/4] staging: r8188eu: add error handling of usb read errors

2022-05-19 12:59:38

On Thu, May 19, 2022 at 01:12:01AM +0300, Pavel Skripkin wrote:
> diff --git a/drivers/staging/r8188eu/core/rtw_efuse.c b/drivers/staging/r8188eu/core/rtw_efuse.c
> index a2691c7f96f6..7105122c2ba0 100644
> --- a/drivers/staging/r8188eu/core/rtw_efuse.c
> +++ b/drivers/staging/r8188eu/core/rtw_efuse.c
> @@ -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;

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

> +
> + if (((value32 >> 24) & 0xff) & 0x80)
> + break;
> +
> retry++;
> }

[ snip ]

> @@ -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);

> diff --git a/drivers/staging/r8188eu/core/rtw_mlme_ext.c b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
> index d4e59fab367c..e54d4139466d 100644
> --- a/drivers/staging/r8188eu/core/rtw_mlme_ext.c
> +++ b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
> @@ -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 ]

> @@ -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?

> @@ -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;

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.

> +
> if (_LLT_NO_ACTIVE == _LLT_OP_VALUE(value))
> break;
>

regards,
dan carpenter