linux-kernel - Re: [PATCH 3/4] staging: r8188eu: add error handling of rtw

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220519054356.GU4009@kadam>
Date:   Thu, 19 May 2022 08:43:56 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Pavel Skripkin <paskripkin@...il.com>
Cc:     gregkh@...uxfoundation.org, Larry.Finger@...inger.net,
        phil@...lpotter.co.uk, straube.linux@...il.com,
        fmdefrancesco@...il.com, linux-kernel@...r.kernel.org,
        linux-staging@...ts.linux.dev
Subject: Re: [PATCH 3/4] staging: r8188eu: add error handling of rtw_read32

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