Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 19 May 2022 08:43:56 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Pavel Skripkin <paskripkin@gmail.com>
Cc:     gregkh@linuxfoundation.org, Larry.Finger@lwfinger.net,
        phil@philpotter.co.uk, straube.linux@gmail.com,
        fmdefrancesco@gmail.com, linux-kernel@vger.kernel.org,
        linux-staging@lists.linux.dev
Subject: Re: [PATCH 3/4] staging: r8188eu: add error handling of rtw_read32
Message-ID: <20220519054356.GU4009@kadam>
References: <cover.1652911343.git.paskripkin@gmail.com>
 <5cab10528fed7d440ee57f93183d18c9de998adb.1652911343.git.paskripkin@gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <5cab10528fed7d440ee57f93183d18c9de998adb.1652911343.git.paskripkin@gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?1SajeVg5BcpxBG40uvkxgXy6JCyAeFXMRe/P29HYzamJuGpEf7OtnTQhM1D+?=
 =?us-ascii?Q?eKiKrQ/Dadfd5f5TxCT2Kl2e2/LZ27n0Mr9yitUFguebNMEwuuKQFZ9IwuTx?=
 =?us-ascii?Q?I/6lgi4HT3UJSIiDvd4IvnOqPbWP38DrlCpVQ4GrxmUoku/Zd98x3fnrv0mN?=
 =?us-ascii?Q?7/z9WzIIWNoIRM8kgHcZaNg7ckw/YVGJKn6x1dq1chr85gFh2L2Nl+FzuouS?=
 =?us-ascii?Q?cYo5fy/GR6N2rMPauVWnN0TGsjOT/oZbjVgMfuaB0Yuucj2NG30rhf4Nkh/k?=
 =?us-ascii?Q?FOlI+loSyDFNcz30VQ6cK9vPW29R5qrH+U9ILWgOpoSNYk9S22uov2QNA4nx?=
 =?us-ascii?Q?2omln+h3+rGVpDaUKhmnwo0eMXESiyXGszWvUj0l4RgoMH/RIuwRIG+KDnMm?=
 =?us-ascii?Q?vQ0NB+2LSW12FMCd5SUyTO8gFwQppkCsvT+MgToi+lUEEw64gspmi6lzVlUu?=
 =?us-ascii?Q?MqRv39DaQ3QXljLaj73Cb+qDRHG+jc7KLDPtrV+HPatVEbvKlGde7q9CGTBc?=
 =?us-ascii?Q?ezNbpz8WCIAc/0KLZgOJ0BujtLrPFCqpPSjio9ywdw+7ph6O+FM9W9p+thPc?=
 =?us-ascii?Q?Z4LtMcSKV4GwJ3SMyMxdSVI7icrTWPZ99LoxtTt0Z4zRU+1+cQamrOux034T?=
 =?us-ascii?Q?Cn9rJdXTsVXEn/3P6ba3SUdvVr4ajB6UvNQWnjOBeQf/sDX9ryYWbaAD9bPi?=
 =?us-ascii?Q?MWDC+XPmnCv27G8KCSa3jL692eH4uz6LYblQ1mcO9gceJU8KQxv8OX4dNkZX?=
 =?us-ascii?Q?Q8FBuZv4AYsxdj0IFcWw50QpVJfgvLxQXnNZ5SxOaB40ADBkfj8ZBKjZRidJ?=
 =?us-ascii?Q?AXySDHnBH0EZ7xXSKZmvH9++piU3GzSNa6+bSZO5RxtQmjFD7XnXDRIkAAy3?=
 =?us-ascii?Q?1cdzlufHvrDiVQfLEVgdZ3iabjDcnJHsoUmj73sb1HMJxknX4zUekZw3yZnA?=
 =?us-ascii?Q?q8rwvd+3zlaf6at+Q0/WEPh9nENfxTNmnpOhpskCFHZq/jXmePjtcQLW85fE?=
 =?us-ascii?Q?fC/p7m4q+G6UB6VL4kgZDnoHTx+q8JhkCoR/BTtrpuLpS1go3lttlhu1qKXJ?=
 =?us-ascii?Q?OgYyyzjhblJgTv9XHD9wRIHs0zELZxwhB89zooD3k0iO4mTQM7XYHxE2xbXo?=
 =?us-ascii?Q?9NbAOFcPrQ3Xw0EFPzMjG7PzKsKC+ktmIwK8YN6XmqUNGZeMkD+VmvxTUM34?=
 =?us-ascii?Q?onMdE44zoD3p+P9rsl1l/TwAVUU0OVK36tZk/quU6bdxU6ImFGWCM7Obc1Ua?=
 =?us-ascii?Q?a4lXd5uJKMEz2FSbrllD7WY5GwpMbnDZTxgs1PQxhMAwSh2J5IVu6xpPuXh5?=
 =?us-ascii?Q?oP8cmkBF6V/XYXgClsFKoc2czYVZ2cvPz2Tn3lTKZed37O8ggItovx8ABwS6?=
 =?us-ascii?Q?dFr05i+kKmemZ1FKTRKaUKpiSVNUigbOe1Vz3vLJ/QH/48HZAA70SSCoWTHj?=
 =?us-ascii?Q?7kkY4brZL7x/Br3+DwReG6/7m3X9IxmR1QlDkDE56CHUCuALlacZSIOIhmOO?=
 =?us-ascii?Q?fSZmk7OnBauAkCeEHYRHV2RBqYFu6gqiVlFV+drLy4koSLCG4IbeoqT7sA0K?=
 =?us-ascii?Q?bi4+eqpRa0o0L3CO6qs8CT3TQXtKCkxqR9x2l2ugZBaC8XLQtlv/uBOqvXuc?=
 =?us-ascii?Q?Ke5+czFryyeU0Bok4EF6xmDEx/v4HFcePY3mtTnDoNjV8pJ/M0+MdSpyD3ok?=
 =?us-ascii?Q?yJ+Drd2Q7HV28p2+TPNSulaZ3btQdrLaz1hh7aKLDjqI0dCXoFNo4xYBGZzU?=
 =?us-ascii?Q?DQmYvgLEmaVRQ10qv2dAFhS8c4y7mtU=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 07fb2c96-b64f-4296-27d3-08da395a9ccf
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 May 2022 05:44:17.0757
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Z636Ao5OlQIIfbXbVPTRYolRE9gZF3+GfazPnYw0fsoVXPcgpyjI+eSjkgXcPYeXWeTa7zBSA0AeyCcBR+QAuBMGmk0CyvYDk8xOsk/YbFY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR10MB1543
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.874
 definitions=2022-05-18_09:2022-05-17,2022-05-18 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 phishscore=0
 mlxlogscore=999 spamscore=0 suspectscore=0 malwarescore=0 mlxscore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2202240000 definitions=main-2205190035
X-Proofpoint-ORIG-GUID: kHS8r3SHf3dqPylbHzNthho8so1YVbqO
X-Proofpoint-GUID: kHS8r3SHf3dqPylbHzNthho8so1YVbqO
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,
        SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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