From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4C0362CCB9 for ; Thu, 18 Dec 2025 14:56:51 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1766069811; cv=none; b=IvUwof0FmadbRpfFlRaqMZZJ5OC4vdWDkJXSyDXhpC1EtDRbty7FYmcPYquo39Dkk7OQYuaKxGSpdEFr49ZswaA5zipovh2UKpJk68JhJKWOWA8GoSYHsseL1JDUdrHlVz815GAtE+EZh/ITDSw1s9ycblchQxK+6RjeGZL0rA4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1766069811; c=relaxed/simple; bh=zmInG8DyBR4SdrwUKa+kbWrkraSYDJxUyQNLVWEKcQM=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=iqcTb+2dr+34ETijm38SBL2bKhuDLCeBycwJO4pPNwOSU98aLn5viUJBKITy+6IAy/gTXu1aOia6JLwfKsVIHOzVV2LZ1gFA9fghiS6mOtkr/wnuEDoT1N5ho44vjt2p3qKlg4pC/2CrYFIuCAk4JICP8XPKxFdoOdmrisexWGA= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linuxfoundation.org header.i=@linuxfoundation.org header.b=hglsR+cM; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linuxfoundation.org header.i=@linuxfoundation.org header.b="hglsR+cM" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 7703EC4CEFB; Thu, 18 Dec 2025 14:56:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1766069811; bh=zmInG8DyBR4SdrwUKa+kbWrkraSYDJxUyQNLVWEKcQM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=hglsR+cMWRrZKbkaCYV/9y6sidglELIeLayD/84F6Lu0Y3MV1uEIdeFJwhraXp6/C REHWX5VvPEsI9NnQzAQyZbgNb5Bt4OwJ8tIvGco7cx0hco2FJfbUPMqT/xdoipPjuq x2Guy4wj0oA8gg1MSPZTgdoasdYSlLq30FCvp1Mc= Date: Thu, 18 Dec 2025 15:56:47 +0100 From: Greg KH To: cjz Cc: linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] rtl8723bs: Replace atomic_t with int for continual_io_error (no concurrency) Message-ID: <2025121809-nervy-parakeet-8146@gregkh> References: Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: On Thu, Dec 18, 2025 at 10:46:28PM +0800, cjz wrote: > From: changjunzheng > > The 'continual_io_error' variable is defined as atomic_t, but all call sites > of rtw_inc_and_chk_continual_io_error/rtw_reset_continual_io_error are in > process context (sdio_ops_linux.c/sdio_intf.c, SDIO read/write retry logic). > There is no interrupt/thread concurrency modifying this variable, so atomic > operations are unnecessary and introduce slight performance overhead. > > This change replaces atomic_t with a normal int, and replaces atomic_inc_return()/atomic_set() > with ordinary increment/assignment, keeping all functional logic unchanged. Please line-wrap at 72 columns. > --- > v2 changes: > 1. Remove redundant 'error_count' variable and fix variable declaration position (comply with kernel coding standards). > 2. Simplify the function logic of rtw_inc_and_chk_continual_io_error. > > Signed-off-by: changjunzheng The signed off by goes above the --- line. Also, why not use your full name? Or native language name? > --- > drivers/staging/rtl8723bs/core/rtw_io.c | 10 +++------- > drivers/staging/rtl8723bs/include/drv_types.h | 2 +- > 2 files changed, 4 insertions(+), 8 deletions(-) > > diff --git a/drivers/staging/rtl8723bs/core/rtw_io.c b/drivers/staging/rtl8723bs/core/rtw_io.c > index fe9f94001eed..0f52710e6d3a 100644 > --- a/drivers/staging/rtl8723bs/core/rtw_io.c > +++ b/drivers/staging/rtl8723bs/core/rtw_io.c > @@ -139,16 +139,12 @@ int rtw_init_io_priv(struct adapter *padapter, void (*set_intf_ops)(struct adapt > */ > int rtw_inc_and_chk_continual_io_error(struct dvobj_priv *dvobj) > { > - int error_count = atomic_inc_return(&dvobj->continual_io_error); > - > - if (error_count > MAX_CONTINUAL_IO_ERR) > - return true; > - > - return false; > + dvobj->continual_io_error++; > + return (dvobj->continual_io_error > MAX_CONTINUAL_IO_ERR); Why is this function returning an int for a boolean? And this function is odd, it is saying that if we "max out" on errors, the device is removed? And it does so in a simple loop, so why is this structure variable needed at all? Why not just count the errors in the two loops that call this function? It feels like this is an extra layer of indirection that is not needed at all, right? In other words, I think you can make this even simpler :) thanks, greg k-h