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 07:33:06 +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 1/4] staging: r8188eu: add error handling of rtw_read8
Message-ID: <20220519043306.GS4009@kadam>
References: <cover.1652911343.git.paskripkin@gmail.com>
 <1a9834b705054dcd0b0be0d929084c44a224abaa.1652911343.git.paskripkin@gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1a9834b705054dcd0b0be0d929084c44a224abaa.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?3g7nbmcTI53yjDLJYadOvuypydCNP2oSUdYkneRcC1aGq1VYawRFDvmcQX02?=
 =?us-ascii?Q?7LbkbJi7zDV8wj42MfeYgXhBocKqlB7JS59k8n8WS6uPEbrjjY72Gzr0cCXy?=
 =?us-ascii?Q?lya7VmJUF6D4sWWDACh89Fe3NoSObdl4XvOL4rJupQCEMfg7/CsT1VayVQai?=
 =?us-ascii?Q?KWX5XILXfA0kp2ZEnOGEQ1Oos8pxyC4XgRKOlk4xkRs+xe4N0zFCXMfU7qTb?=
 =?us-ascii?Q?Wb0mlcAlpmTzYsS84AdaG3FA2gNADns46DSrPxTNWCwW45FeycqP1wp9G4l3?=
 =?us-ascii?Q?zOjQCjx2vUkIfiFUyIqg7QXQhU7KOTuUd4eiK/DPnbdR+SDWu92cPqrVwcxC?=
 =?us-ascii?Q?jIp6Woeawm8kihHlFlBaq4xhCqx83otpL5l9MabKthGjHHMNcgHtzApXvS+c?=
 =?us-ascii?Q?3FT46NQYlJH/8CeGUzo+05ZPeV0OFNujpzN7ArF7Uk2RhmQRmgd4RKCr6KEM?=
 =?us-ascii?Q?T1a+t4oQURHGQHktXroX3C9AEC5+6PRaJ6N7A4Z3Bi5k0V2LYP38OMPuQYH2?=
 =?us-ascii?Q?vS8BBz4nyoWxl9/UFaQWEm78y2wedDoSNCfsUf+byiXlY42JQlXDdMl6R26J?=
 =?us-ascii?Q?7/nnO9nNYmJ8EyMP8MpWrm8x0O7cuXgZTLTs8AVOBGD6BZMyIIkY6MemYQwx?=
 =?us-ascii?Q?7V2XRRTeJ9i8Q8hZTFSI4WOs98BxlBtNRzJ7N06BBI7cELtQ2A/tEBbpLBgN?=
 =?us-ascii?Q?CT5LrNyFkH6oHR/xfY5LDfbmj1g1GpdZ2U2dX5u5HkZD0UVmHtOTjEs3+QzQ?=
 =?us-ascii?Q?fPxyvHAxFrmwLwVJnEvvc7x2q4//9yZV6uHj6NLpp2KAjHDmHI77hypXPHGY?=
 =?us-ascii?Q?egdaDX5yQTTfsYzK/kOOCHFx+uqa5v3Z83Rj6JPZvxCdzCMwYCb8YJIHb9I3?=
 =?us-ascii?Q?cbwZFsdakQmlNNn0mUDHV50BLozGnrCy38YbRCTd5IG6XRV0l0Lc/gvW4oUN?=
 =?us-ascii?Q?c6tCezGMH2PQZjrtD5OdAI0O4ZSlnLXAXMN5Vr3mSqzjNdZNXGEWwbaQF1OV?=
 =?us-ascii?Q?7elQ/Q6K5iOkXMwnPgW/GPH2pbNCfbZ2iH51PQbxSxM9XLcU4qjzBScStWCw?=
 =?us-ascii?Q?LhIXkXNR0zSvksw85EbzZRZE+54rz5V9jo2X9MO7W4sIQxzYt07grSuqGIaJ?=
 =?us-ascii?Q?p/vg3D1pSeJddIkRoZ7hsTcjJ2sSw378xHR8z9cfKoOSm7RW7Z7zUeNWtpXs?=
 =?us-ascii?Q?nQvqaKpwva8BHN+bmKWobsQvlzeA+bS5BbY1tmK6f1U0BfhrktvSumAM/oM/?=
 =?us-ascii?Q?UQqkxBeoD3maSSVf0HWpoZzucxzHr3To5QYRVULbiqenHN/3fsBzcBxdfXUl?=
 =?us-ascii?Q?R32tT78loUsPyddX+5kRVnzWnn/b+vjWctHtuALj6PevhPwfmw2CKULmpuiy?=
 =?us-ascii?Q?dnBWTf4lgPSoeezM6IczGJn7yP2sGa37BTM9vXlWJ7FQVMp83HfKuMohdDUx?=
 =?us-ascii?Q?JIK51rx5yf9U2TPnIxxGf0KSNkFgbscRdpEriDpigJN04SEvlY8t0v3NSbYF?=
 =?us-ascii?Q?7jYa3nB+3VB4vynmM51hPQb5qZnZNdZ/yWhN+Qe5QYXUlWQRLiOtulOuQ6ww?=
 =?us-ascii?Q?F5Y8Id57Jfti+24VmFPjW966lz7CNRtGIt8Bp6LAepF+dnr5+bvg7d5/TCC4?=
 =?us-ascii?Q?AU1q3cxaAzBtmeDctwUIkiwdg53CH3COnFGJfWhyxqOToy+Ys4CELxMf5YPe?=
 =?us-ascii?Q?kKeDSPvGe47h4n7zHHhKqSGgYwHUYnd+NVgKfmcuHZaEssRO7g6axL8KFWcb?=
 =?us-ascii?Q?MTHIIyKdZXfh+MyuajoC4m8D+7hPEWw=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a997ca93-31da-48e2-5f25-08da3950b887
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 May 2022 04:33:28.5673
 (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: RWI/rPrlmUFVbI/MDmDrm3RAV1/chvcnSHzPPsMPHAhlIHNQw4YfRXeyMBzrMo54RDD0xW9r31hVNqIDFouw0nFLqZqJc0puc6pueoHx198=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB4464
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 mlxscore=0 malwarescore=0
 suspectscore=0 mlxlogscore=999 adultscore=0 phishscore=0 spamscore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2202240000 definitions=main-2205190028
X-Proofpoint-ORIG-GUID: 6BxcT479mcHjc-rqANcwtGwZkqOleSc1
X-Proofpoint-GUID: 6BxcT479mcHjc-rqANcwtGwZkqOleSc1
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:11:51AM +0300, Pavel Skripkin wrote:
> @@ -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)
>  
>  	/*  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.

> +
> +	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);
> +		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.

> +
> +		rtw_write8(padapter, REG_MCUFWDL, reg | FWDL_CHKSUM_RPT);
>  
>  		ret = write_fw(padapter, fw_data, fw_size);
>  
> diff --git a/drivers/staging/r8188eu/core/rtw_led.c b/drivers/staging/r8188eu/core/rtw_led.c
> index 2f3000428af7..b532e614c5b6 100644
> --- a/drivers/staging/r8188eu/core/rtw_led.c
> +++ b/drivers/staging/r8188eu/core/rtw_led.c
> @@ -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