linux-kernel - Re: [PATCH v2] rtl8723bs: Replace atomic_t with int for continual_io

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] [day] [month] [year] [list]

Message-ID: <2025121809-nervy-parakeet-8146@gregkh>
Date: Thu, 18 Dec 2025 15:56:47 +0100
From: Greg KH <gregkh@...uxfoundation.org>
To: cjz <guagua210311@...com>
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] rtl8723bs: Replace atomic_t with int for
 continual_io_error (no concurrency)

On Thu, Dec 18, 2025 at 10:46:28PM +0800, cjz wrote:
> From: changjunzheng <guagua210311@...com>
> 
> 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 <guagua210311@...com>

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