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:49:38 +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: <20220519054938.GR29930@kadam>
References: <cover.1652911343.git.paskripkin@gmail.com>
 <1a9834b705054dcd0b0be0d929084c44a224abaa.1652911343.git.paskripkin@gmail.com>
 <20220519043306.GS4009@kadam>
 <ff322920-6ddd-159d-b2f2-c0e4fc2e518f@gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ff322920-6ddd-159d-b2f2-c0e4fc2e518f@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?GrNYdRGrA1TU9DD3n770SZKgZI+e+i3C+l9v4m+ShE033X3/EKomLaw7Jb1X?=
 =?us-ascii?Q?eC3a/arCcvrhRzO7deCT4MBdFQEZh63oH2ChnRaFlmRiK8o762L2dq9V92Ps?=
 =?us-ascii?Q?OwHTx4wVDezKYLFQCLtIpON3j5XY6ns3crPqoNjPILQEYFnUOdjRqiEbgUvB?=
 =?us-ascii?Q?5ysPJNaxo9zvmgnaJSTtGpgEIpngL6pOq4LtZvxKu9utoKDz/djuOlXVKsen?=
 =?us-ascii?Q?BB4br2kQzw8z66NbcWiXYz0O8tBCX4U4DFrJrxmmpUZ3gR5IylUd5Lnz4ijP?=
 =?us-ascii?Q?ynT77C4u6KQyLex2apX5UCSEhtCrinNHsJ9F6nSzCGArD8Bd5mjXQr6x7RBx?=
 =?us-ascii?Q?mzMRHK4u7iNu/fFF5lb2KLy1wxJAZ3bwiZDLo1i3Ri2uReD3jMk0vF6OmWk6?=
 =?us-ascii?Q?ZwCP6Vk8fLLc2gQOM7O/AWChe0KsFfffmQlbRd9kAgNX+LDvMOORKjqdLIOp?=
 =?us-ascii?Q?twynbsyNAvu2XFV8px4cNLEw4RTRCWzQl6xJ4uE9F+hgJbjWx/52ofEP22Sg?=
 =?us-ascii?Q?XEYHFzDJhCePHlI37WkB9XrIE8sql+N4IK+P/asBYdGYy+5YK2BAQLD4HMI5?=
 =?us-ascii?Q?tUB15KUrnVzWehVQXZ+WE3Nfe6gPaas+clrLN+90Zlpxffyn3jdwLL8swA1B?=
 =?us-ascii?Q?YYNo0vdP/yJ4s1pBexb6c8lYIhuM24/uI6XAb9GQRMRZMjW5wiP5/ssCb/DH?=
 =?us-ascii?Q?vJrkgFVI/T7irs/MMXl9j9JHJXdKez1bhrY22+LSgWUwJPpW3qnyAdzDjFL3?=
 =?us-ascii?Q?hUe6sR2CWv96EU/vAzKw3ScWFtp9jeQjr+Lk7o63X/IritlIy7QP7AmJiQR8?=
 =?us-ascii?Q?X9Bm5ygj/Yixt0tZprXJdMkzidSYJAomXvDgQjGQKctfyy0G0/p3yKGPr9on?=
 =?us-ascii?Q?ZqG7ZBIE3MU6b2UFUbAORhPl9XDlXn//vBpg0cIs7cOPaI4K2vB42f+BGoM1?=
 =?us-ascii?Q?xPOpPfEoA4/bylXtJ/53eJWMZD3i5SycxJazMcrGKMPKhHixyvaicCyMkTZC?=
 =?us-ascii?Q?U6ZvKIHT3hRTeZYg+jBO1ok1WQvwhPdcnbM0OkIk6dK4iQh+GjIXWmOawzzd?=
 =?us-ascii?Q?E0LneKHlZxDcctdVLENhOUCBNYnf8tZobrccRoO8nMs0aNg4TBKAtO2LYEUW?=
 =?us-ascii?Q?/38us2GfrlfNqvu+gERUVPg9al2pZbL+XrOlMq5j/u8LqXFJeC76mNgKEWqp?=
 =?us-ascii?Q?DO1PRxt76QJJrPEm+mKTfWJmnZjTXA69cPYp4yvwZ5aJO0KXKwk05FQzdr8h?=
 =?us-ascii?Q?WibeWolNqLEiV4xwbU6EcwO4NYy82MargfvHRPA125VlFebC32/n2ho3NKtK?=
 =?us-ascii?Q?CFuGvYOyC9fiDJIvNF6KyDElTwHi0ojydZjxgTqxFXfTnl+KeyL1bZIM55rD?=
 =?us-ascii?Q?pal+xSNitEZvbgOZJq6c0tqOy5FxnBOasQxBnUV2ITpsDl/kFZvKECW4o3X/?=
 =?us-ascii?Q?+R0zcHpLrRrqr5YvxyRhsscXbSFP8bkcx1Z7aORtrZ0auiKkKSHzRkYBjntd?=
 =?us-ascii?Q?IgwSqkk73J36H8OerMBCuh0mhrsrint3UGpi/XLH5MhC4GPSrxXCCNda26ae?=
 =?us-ascii?Q?0/Y9hDwtKgZTsOYRlzz+AWTq+pzSKdmP87KtJE5ZHtYmY+CDXe5jF0GsI0bH?=
 =?us-ascii?Q?JyiUkuzdf/Qit4PciE5xjTdu8bpbSrlIm1QoQb8EdkJUFJrJVrZ4B3sSKPSg?=
 =?us-ascii?Q?qM4Ji23bqkIefL83BMzrToyQ6uiF6qBKpSAP6PnfJxaeVGJpPa4dUKOQZ6Pv?=
 =?us-ascii?Q?izHd5bttO/+sX6qiMnLtSdtVNvQ3Mx8=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ffc03978-6630-4c2f-9524-08da395b680b
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:49:57.7836
 (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: t1gtHeflqfKfOh7tqSieyakuVlM/vz+bmMxa4s0T4LzxtETIrHfHaRBnTFU6tI8iCwdjN+6PCfc0j44RhmIugv0Sx8oSy3OPiun4eJiHmAQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR1001MB2407
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 malwarescore=0 spamscore=0 mlxscore=0
 phishscore=0 suspectscore=0 mlxlogscore=999 adultscore=0 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2202240000
 definitions=main-2205190036
X-Proofpoint-GUID: 5olyTgP5fPx5N9cQuGeO8Pa1ERfuO9TY
X-Proofpoint-ORIG-GUID: 5olyTgP5fPx5N9cQuGeO8Pa1ERfuO9TY
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,
        RCVD_IN_MSPIKE_H2,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 08:43:23AM +0300, Pavel Skripkin wrote:
> > > +
> > > +	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.
> > 
> 
> 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
> 

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