[PATCH v4] staging: rtl8723bs: Replace custom rtw_cbuf with kfifo

[PATCH v4] staging: rtl8723bs: Replace custom rtw_cbuf with kfifo

[Ashmit Kumar]

The rtl8723bs driver implemented its own custom circular buffer
(rtw_cbuf) for c2h event handling. The kernel already provides a standard,
lockless circular buffer implementation in <linux/kfifo.h>.

This patch replaces the custom rtw_cbuf struct and its associated
functions with the standard kfifo API (kfifo_alloc, kfifo_put,
kfifo_get, kfifo_is_empty, kfifo_free), simplifying the driver code
and relying on the robust kernel infrastructure. Furthermore, the allocation
size is simplified to C2H_QUEUE_MAX_LEN, dropping the vestigial + 1 that
the original naive ring buffer required to disambiguate full from empty.

Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Ashmit Kumar <work.ashmitkumar@gmail.com>
---
Changes in v4:
 - Abandoned ternary operator style fixes in favor of completely
   replacing the custom rtw_cbuf ring buffer with the standard Linux
   kfifo API, as suggested by Greg Kroah-Hartman.

Changes in v3:
 - Formatted as a standalone patch (removed 1/3 series numbering).

Changes in v2:
 - Fixed patch formatting and versioning issues.

Changes in v1:
 - Initial submission addressing ternary operator style issues in rtw_cbuf.

 drivers/staging/rtl8723bs/core/rtw_cmd.c      | 16 ++--
 drivers/staging/rtl8723bs/hal/sdio_ops.c      |  2 +-
 .../staging/rtl8723bs/include/osdep_service.h | 14 ----
 drivers/staging/rtl8723bs/include/rtw_cmd.h   |  3 +-
 .../staging/rtl8723bs/os_dep/osdep_service.c  | 77 -------------------
 5 files changed, 11 insertions(+), 101 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c b/drivers/staging/rtl8723bs/core/rtw_cmd.c
index ce3dfa1fee26..424aa180eecc 100644
--- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
+++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
@@ -202,8 +202,7 @@ int rtw_init_evt_priv(struct evt_priv *pevtpriv)
 
 	_init_workitem(&pevtpriv->c2h_wk, c2h_wk_callback, NULL);
 	pevtpriv->c2h_wk_alive = false;
-	pevtpriv->c2h_queue = rtw_cbuf_alloc(C2H_QUEUE_MAX_LEN + 1);
-	if (!pevtpriv->c2h_queue)
+	if (kfifo_alloc(&pevtpriv->c2h_queue, C2H_QUEUE_MAX_LEN, GFP_KERNEL))
 		return -ENOMEM;
 
 	return 0;
@@ -211,17 +210,17 @@ int rtw_init_evt_priv(struct evt_priv *pevtpriv)
 
 void _rtw_free_evt_priv(struct	evt_priv *pevtpriv)
 {
+	void *c2h;
+
 	_cancel_workitem_sync(&pevtpriv->c2h_wk);
 	while (pevtpriv->c2h_wk_alive)
 		fsleep(10 * USEC_PER_MSEC);
 
-	while (!rtw_cbuf_empty(pevtpriv->c2h_queue)) {
-		void *c2h = rtw_cbuf_pop(pevtpriv->c2h_queue);
-
+	while (kfifo_get(&pevtpriv->c2h_queue, &c2h)) {
 		if (c2h && c2h != (void *)pevtpriv)
 			kfree(c2h);
 	}
-	kfree(pevtpriv->c2h_queue);
+	kfifo_free(&pevtpriv->c2h_queue);
 }
 
 void _rtw_free_cmd_priv(struct	cmd_priv *pcmdpriv)
@@ -1695,12 +1694,13 @@ static void c2h_wk_callback(struct work_struct *work)
 	struct evt_priv *evtpriv = container_of(work, struct evt_priv, c2h_wk);
 	struct adapter *adapter = container_of(evtpriv, struct adapter, evtpriv);
 	u8 *c2h_evt;
+	void *c2h_ptr;
 	c2h_id_filter ccx_id_filter = rtw_hal_c2h_id_filter_ccx(adapter);
 
 	evtpriv->c2h_wk_alive = true;
 
-	while (!rtw_cbuf_empty(evtpriv->c2h_queue)) {
-		c2h_evt = (u8 *)rtw_cbuf_pop(evtpriv->c2h_queue);
+	while (kfifo_get(&evtpriv->c2h_queue, &c2h_ptr)) {
+		c2h_evt = (u8 *)c2h_ptr;
 		if (c2h_evt) {
 			/* This C2H event is read, clear it */
 			c2h_evt_clear(adapter);
diff --git a/drivers/staging/rtl8723bs/hal/sdio_ops.c b/drivers/staging/rtl8723bs/hal/sdio_ops.c
index da2d9088ab5a..e39faed1f195 100644
--- a/drivers/staging/rtl8723bs/hal/sdio_ops.c
+++ b/drivers/staging/rtl8723bs/hal/sdio_ops.c
@@ -763,7 +763,7 @@ void sd_int_dpc(struct adapter *adapter)
 			}
 		} else {
 			/* Error handling for malloc fail */
-			rtw_cbuf_push(adapter->evtpriv.c2h_queue, NULL);
+			kfifo_put(&adapter->evtpriv.c2h_queue, NULL);
 			_set_workitem(&adapter->evtpriv.c2h_wk);
 		}
 	}
diff --git a/drivers/staging/rtl8723bs/include/osdep_service.h b/drivers/staging/rtl8723bs/include/osdep_service.h
index 2f5011a8210c..6feeb1351faa 100644
--- a/drivers/staging/rtl8723bs/include/osdep_service.h
+++ b/drivers/staging/rtl8723bs/include/osdep_service.h
@@ -36,18 +36,4 @@ extern void rtw_free_netdev(struct net_device *netdev);
 void rtw_buf_free(u8 **buf, u32 *buf_len);
 void rtw_buf_update(u8 **buf, u32 *buf_len, u8 *src, u32 src_len);
 
-struct rtw_cbuf {
-	u32 write;
-	u32 read;
-	u32 size;
-	void *bufs[];
-};
-
-bool rtw_cbuf_full(struct rtw_cbuf *cbuf);
-bool rtw_cbuf_empty(struct rtw_cbuf *cbuf);
-bool rtw_cbuf_push(struct rtw_cbuf *cbuf, void *buf);
-void *rtw_cbuf_pop(struct rtw_cbuf *cbuf);
-struct rtw_cbuf *rtw_cbuf_alloc(u32 size);
-
-
 #endif
diff --git a/drivers/staging/rtl8723bs/include/rtw_cmd.h b/drivers/staging/rtl8723bs/include/rtw_cmd.h
index 2e791da7e815..62e6b9011625 100644
--- a/drivers/staging/rtl8723bs/include/rtw_cmd.h
+++ b/drivers/staging/rtl8723bs/include/rtw_cmd.h
@@ -8,6 +8,7 @@
 #define __RTW_CMD_H_
 
 #include <linux/completion.h>
+#include <linux/kfifo.h>
 
 #define C2H_MEM_SZ (16*1024)
 
@@ -58,7 +59,7 @@
 	struct	evt_priv {
 		struct work_struct c2h_wk;
 		bool c2h_wk_alive;
-		struct rtw_cbuf *c2h_queue;
+		DECLARE_KFIFO_PTR(c2h_queue, void *);
 		#define C2H_QUEUE_MAX_LEN 10
 
 		atomic_t event_seq;
diff --git a/drivers/staging/rtl8723bs/os_dep/osdep_service.c b/drivers/staging/rtl8723bs/os_dep/osdep_service.c
index 4cfdf7c62344..1db1c5aff530 100644
--- a/drivers/staging/rtl8723bs/os_dep/osdep_service.c
+++ b/drivers/staging/rtl8723bs/os_dep/osdep_service.c
@@ -122,80 +122,3 @@ void rtw_buf_update(u8 **buf, u32 *buf_len, u8 *src, u32 src_len)
 		kfree(ori);
 }
 
-/**
- * rtw_cbuf_full - test if cbuf is full
- * @cbuf: pointer of struct rtw_cbuf
- *
- * Returns: true if cbuf is full
- */
-inline bool rtw_cbuf_full(struct rtw_cbuf *cbuf)
-{
-	return (cbuf->write == cbuf->read - 1) ? true : false;
-}
-
-/**
- * rtw_cbuf_empty - test if cbuf is empty
- * @cbuf: pointer of struct rtw_cbuf
- *
- * Returns: true if cbuf is empty
- */
-inline bool rtw_cbuf_empty(struct rtw_cbuf *cbuf)
-{
-	return (cbuf->write == cbuf->read) ? true : false;
-}
-
-/**
- * rtw_cbuf_push - push a pointer into cbuf
- * @cbuf: pointer of struct rtw_cbuf
- * @buf: pointer to push in
- *
- * Lock free operation, be careful of the use scheme
- * Returns: true push success
- */
-bool rtw_cbuf_push(struct rtw_cbuf *cbuf, void *buf)
-{
-	if (rtw_cbuf_full(cbuf))
-		return _FAIL;
-
-	cbuf->bufs[cbuf->write] = buf;
-	cbuf->write = (cbuf->write + 1) % cbuf->size;
-
-	return _SUCCESS;
-}
-
-/**
- * rtw_cbuf_pop - pop a pointer from cbuf
- * @cbuf: pointer of struct rtw_cbuf
- *
- * Lock free operation, be careful of the use scheme
- * Returns: pointer popped out
- */
-void *rtw_cbuf_pop(struct rtw_cbuf *cbuf)
-{
-	void *buf;
-
-	if (rtw_cbuf_empty(cbuf))
-		return NULL;
-
-	buf = cbuf->bufs[cbuf->read];
-	cbuf->read = (cbuf->read + 1) % cbuf->size;
-
-	return buf;
-}
-
-/**
- * rtw_cbuf_alloc - allocate a rtw_cbuf with given size and do initialization
- * @size: size of pointer
- *
- * Returns: pointer of srtuct rtw_cbuf, NULL for allocation failure
- */
-struct rtw_cbuf *rtw_cbuf_alloc(u32 size)
-{
-	struct rtw_cbuf *cbuf;
-
-	cbuf = kzalloc_flex(*cbuf, bufs, size);
-	if (cbuf)
-		cbuf->size = size;
-
-	return cbuf;
-}
-- 
2.51.0.windows.1

Re: [PATCH v4] staging: rtl8723bs: Replace custom rtw_cbuf with kfifo

[Greg KH]

On Wed, Aug 05, 2026 at 06:33:16PM +0000, Ashmit Kumar wrote:
> The rtl8723bs driver implemented its own custom circular buffer
> (rtw_cbuf) for c2h event handling. The kernel already provides a standard,
> lockless circular buffer implementation in <linux/kfifo.h>.
> 
> This patch replaces the custom rtw_cbuf struct and its associated
> functions with the standard kfifo API (kfifo_alloc, kfifo_put,
> kfifo_get, kfifo_is_empty, kfifo_free), simplifying the driver code
> and relying on the robust kernel infrastructure. Furthermore, the allocation
> size is simplified to C2H_QUEUE_MAX_LEN, dropping the vestigial + 1 that
> the original naive ring buffer required to disambiguate full from empty.
> 
> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Ashmit Kumar <work.ashmitkumar@gmail.com>
> ---
> Changes in v4:
>  - Abandoned ternary operator style fixes in favor of completely
>    replacing the custom rtw_cbuf ring buffer with the standard Linux
>    kfifo API, as suggested by Greg Kroah-Hartman.

Great, how was this tested?

> @@ -58,7 +59,7 @@
>  	struct	evt_priv {
>  		struct work_struct c2h_wk;
>  		bool c2h_wk_alive;
> -		struct rtw_cbuf *c2h_queue;
> +		DECLARE_KFIFO_PTR(c2h_queue, void *);

Why did you loose the type of the pointer?  Was a LLM used to create
this change?

thanks,

greg k-h

Re: [PATCH v4] staging: rtl8723bs: Replace custom rtw_cbuf with kfifo

[Ashmit Kumar]

Hi Greg,

Regarding the pointer type, no type information was actually lost. The
original struct rtw_cbuf was implemented with an untyped void *bufs[]
array, and I used void * in DECLARE_KFIFO_PTR to preserve that exact
behavior 1:1.

The technical reason the original code used void * is that the
consumer (c2h_wk_callback) expects the queue to multiplex three
different untyped pointer states -

A valid u8 * payload buffer.
A special sentinel pointer (void *)evtpriv used to trigger c2h_evt_clear.
A NULL pointer.

I traced every push into c2h_queue across the codebase. Today, there
is exactly one reachable push site (hal/sdio_ops.c:766), which
exclusively pushes NULL to signal that the interrupt handler failed to
allocate memory. However, because the consumer logic still explicitly
branches on all three pointer states, preserving void * was the safest
way to swap the ring buffer without rewriting the underlying event
state machine.

And to answer your second question, no LLM was used to create this
change. I analyzed the driver's event queue and wrote the migration
manually.

If you prefer, I can submit a v5 that changes the kfifo type to u8 *
and explicitly casts the sentinel, or I can submit a follow-up patch
that rips out the unreachable consumer branches entirely.

Thanks,
Ashmit Kumar


On Thu, Aug 6, 2026 at 10:22 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Aug 05, 2026 at 06:33:16PM +0000, Ashmit Kumar wrote:
> > The rtl8723bs driver implemented its own custom circular buffer
> > (rtw_cbuf) for c2h event handling. The kernel already provides a standard,
> > lockless circular buffer implementation in <linux/kfifo.h>.
> >
> > This patch replaces the custom rtw_cbuf struct and its associated
> > functions with the standard kfifo API (kfifo_alloc, kfifo_put,
> > kfifo_get, kfifo_is_empty, kfifo_free), simplifying the driver code
> > and relying on the robust kernel infrastructure. Furthermore, the allocation
> > size is simplified to C2H_QUEUE_MAX_LEN, dropping the vestigial + 1 that
> > the original naive ring buffer required to disambiguate full from empty.
> >
> > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Ashmit Kumar <work.ashmitkumar@gmail.com>
> > ---
> > Changes in v4:
> >  - Abandoned ternary operator style fixes in favor of completely
> >    replacing the custom rtw_cbuf ring buffer with the standard Linux
> >    kfifo API, as suggested by Greg Kroah-Hartman.
>
> Great, how was this tested?
>
> > @@ -58,7 +59,7 @@
> >       struct  evt_priv {
> >               struct work_struct c2h_wk;
> >               bool c2h_wk_alive;
> > -             struct rtw_cbuf *c2h_queue;
> > +             DECLARE_KFIFO_PTR(c2h_queue, void *);
>
> Why did you loose the type of the pointer?  Was a LLM used to create
> this change?
>
> thanks,
>
> greg k-h