[PATCH 0/5] rtw_sdio_if1_init cleanup and small logic tweak

[PATCH 0/5] rtw_sdio_if1_init cleanup and small logic tweak

[Omer El Idrissi]

This patch series does some general cleanup for rtw_sdio_if1_init. Among
the changes are: using direct returns, removing the use of
vendor-defined macros, adding a separate label for freeing and
unregistering padapter->rtw_wdev.

Signed-off-by: Omer El Idrissi <omer.e.idrissi@gmail.com>

Omer El Idrissi (5):
  staging: rtl8723bs: use direct returns in rtw_sdio_if1_init
  staging: rtl8723bs: remove useless line in rtw_sdio_if1_init
  staging: rtl8723bs: remove use of vendor-defined status macros
  staging: rtl8723bs: replace function with error handling alternative
  staging: rtl8723bs: add separate label for freeing rtw_wdev

 drivers/staging/rtl8723bs/include/sdio_hal.h |  1 -
 drivers/staging/rtl8723bs/os_dep/sdio_intf.c | 56 ++++++++------------
 2 files changed, 22 insertions(+), 35 deletions(-)

-- 
2.51.0

[PATCH 1/5] staging: rtl8723bs: use direct returns in rtw_sdio_if1_init

[Omer El Idrissi]

Clean up rtw_sdio_if1_init to use direct returns for the sake of
readability.

Signed-off-by: Omer El Idrissi <omer.e.idrissi@gmail.com>
---
 drivers/staging/rtl8723bs/os_dep/sdio_intf.c | 29 ++++++++------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
index d664e254912c..64618632ee78 100644
--- a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
+++ b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
@@ -224,14 +224,13 @@ static void sd_intf_stop(struct adapter *padapter)
 
 static struct adapter *rtw_sdio_if1_init(struct dvobj_priv *dvobj, const struct sdio_device_id  *pdid)
 {
-	int status = _FAIL;
 	struct net_device *pnetdev;
 	struct adapter *padapter = NULL;
 	struct sdio_data *psdio = &dvobj->intf_data;
 
 	padapter = vzalloc(sizeof(*padapter));
 	if (!padapter)
-		goto exit;
+		return NULL;
 
 	padapter->dvobj = dvobj;
 	dvobj->if1 = padapter;
@@ -289,27 +288,21 @@ static struct adapter *rtw_sdio_if1_init(struct dvobj_priv *dvobj, const struct
 
 	rtw_hal_disable_interrupt(padapter);
 
-	status = _SUCCESS;
+	return padapter;
 
 free_hal_data:
-	if (status != _SUCCESS && padapter->HalData)
-		kfree(padapter->HalData);
+	kfree(padapter->HalData);
 
-	if (status != _SUCCESS) {
-		rtw_wdev_unregister(padapter->rtw_wdev);
-		rtw_wdev_free(padapter->rtw_wdev);
-	}
+	rtw_wdev_unregister(padapter->rtw_wdev);
+	rtw_wdev_free(padapter->rtw_wdev);
 
 free_adapter:
-	if (status != _SUCCESS) {
-		if (pnetdev)
-			rtw_free_netdev(pnetdev);
-		else
-			vfree((u8 *)padapter);
-		padapter = NULL;
-	}
-exit:
-	return padapter;
+	if (pnetdev)
+		rtw_free_netdev(pnetdev);
+	else
+		vfree((u8 *)padapter);
+
+	return NULL;
 }
 
 static void rtw_sdio_if1_deinit(struct adapter *if1)
-- 
2.51.0

[PATCH 2/5] staging: rtl8723bs: remove useless line in rtw_sdio_if1_init

[Omer El Idrissi]

Remove line in rtw_sdio_if1_init that just sets padapter to itself.
padapter is allocated at the top of the function, put inside the pnetdev
priv (rtw_init_netdev), and retrieved with rtw_netdev_priv, which
doesn't change anything.

Signed-off-by: Omer El Idrissi <omer.e.idrissi@gmail.com>
---
 drivers/staging/rtl8723bs/os_dep/sdio_intf.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
index 64618632ee78..8412331c4949 100644
--- a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
+++ b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
@@ -247,8 +247,6 @@ static struct adapter *rtw_sdio_if1_init(struct dvobj_priv *dvobj, const struct
 
 	SET_NETDEV_DEV(pnetdev, dvobj_to_dev(dvobj));
 
-	padapter = rtw_netdev_priv(pnetdev);
-
 	/* 3 3. init driver special setting, interface, OS and hardware relative */
 
 	/* 4 3.1 set hardware operation functions */
-- 
2.51.0

[PATCH 3/5] staging: rtl8723bs: remove use of vendor-defined status macros

[Omer El Idrissi]

Remove the check for _FAIL macro on two occurrences in rtw_sdio_if1_init

Signed-off-by: Omer El Idrissi <omer.e.idrissi@gmail.com>
---
 drivers/staging/rtl8723bs/os_dep/sdio_intf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
index 8412331c4949..34ef40a86153 100644
--- a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
+++ b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
@@ -262,7 +262,7 @@ static struct adapter *rtw_sdio_if1_init(struct dvobj_priv *dvobj, const struct
 	padapter->intf_alloc_irq = &sdio_alloc_irq;
 	padapter->intf_free_irq = &sdio_free_irq;
 
-	if (rtw_init_io_priv(padapter, sdio_set_intf_ops) == _FAIL)
+	if (rtw_init_io_priv(padapter, sdio_set_intf_ops))
 		goto free_hal_data;
 
 	rtw_hal_read_chip_version(padapter);
@@ -275,7 +275,7 @@ static struct adapter *rtw_sdio_if1_init(struct dvobj_priv *dvobj, const struct
 	rtw_hal_read_chip_info(padapter);
 
 	/* 3 7. init driver common data */
-	if (rtw_init_drv_sw(padapter) == _FAIL)
+	if (rtw_init_drv_sw(padapter))
 		goto free_hal_data;
 
 	rtw_wdev_alloc(padapter, dvobj_to_dev(dvobj));
-- 
2.51.0

[PATCH 4/5] staging: rtl8723bs: replace function with error handling alternative

[Omer El Idrissi]

Replace the use of rtw_set_hal_ops with rtw_hal_data_init in
rtw_sdio_if1_init , which actually returns error or success and not
void.
rtw_set_hal_ops literally only calls rtw_hal_data_init and just ignores the
possibility of errors.

This is the only place this function is used, so remove it's unnecessary
definitions in include/sdio_hal.h as a prototype and os_dep/sdio_intf.c
as a function.

Signed-off-by: Omer El Idrissi <omer.e.idrissi@gmail.com>
---
 drivers/staging/rtl8723bs/include/sdio_hal.h |  1 -
 drivers/staging/rtl8723bs/os_dep/sdio_intf.c | 11 +++--------
 2 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/rtl8723bs/include/sdio_hal.h b/drivers/staging/rtl8723bs/include/sdio_hal.h
index 6538253765f1..4ad145d5d33f 100644
--- a/drivers/staging/rtl8723bs/include/sdio_hal.h
+++ b/drivers/staging/rtl8723bs/include/sdio_hal.h
@@ -9,6 +9,5 @@
 
 u8 sd_int_isr(struct adapter *padapter);
 void sd_int_dpc(struct adapter *padapter);
-void rtw_set_hal_ops(struct adapter *padapter);
 
 #endif /* __SDIO_HAL_H__ */
diff --git a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
index 34ef40a86153..aea9b4e19874 100644
--- a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
+++ b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
@@ -197,12 +197,6 @@ static void sdio_dvobj_deinit(struct sdio_func *func)
 	}
 }
 
-void rtw_set_hal_ops(struct adapter *padapter)
-{
-	/* alloc memory for HAL DATA */
-	rtw_hal_data_init(padapter);
-}
-
 static void sd_intf_start(struct adapter *padapter)
 {
 	if (!padapter)
@@ -250,8 +244,9 @@ static struct adapter *rtw_sdio_if1_init(struct dvobj_priv *dvobj, const struct
 	/* 3 3. init driver special setting, interface, OS and hardware relative */
 
 	/* 4 3.1 set hardware operation functions */
-	rtw_set_hal_ops(padapter);
-
+	/* allocates padapter->HalData */
+	if (rtw_hal_data_init(padapter))
+		goto free_adapter;
 
 	/* 3 5. initialize Chip version */
 	padapter->intf_start = &sd_intf_start;
-- 
2.51.0

[PATCH 5/5] staging: rtl8723bs: add separate label for freeing rtw_wdev

[Omer El Idrissi]

The original function would "unregister" and "free" padapter->rtw_wdev
IF it ran into issues after "hardware operation functions" were set.
The problem is that rtw_wdev isn't even allocated at that point.
So separate the rtw_wdev_unregister and rtw_wdev_free code into it's own
label, add error check to rtw_wdev_alloc and move it up a bit.
When rtw_init_drv_sw fails goto free_wdev instead of free_hal_data.

Signed-off-by: Omer El Idrissi <omer.e.idrissi@gmail.com>
---
 drivers/staging/rtl8723bs/os_dep/sdio_intf.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
index aea9b4e19874..a088dae40a19 100644
--- a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
+++ b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
@@ -268,12 +268,13 @@ static struct adapter *rtw_sdio_if1_init(struct dvobj_priv *dvobj, const struct
 
 	/* 3 6. read efuse/eeprom data */
 	rtw_hal_read_chip_info(padapter);
+	
+	if (rtw_wdev_alloc(padapter, dvobj_to_dev(dvobj))) 
+		goto free_hal_data;
 
 	/* 3 7. init driver common data */
 	if (rtw_init_drv_sw(padapter))
-		goto free_hal_data;
-
-	rtw_wdev_alloc(padapter, dvobj_to_dev(dvobj));
+		goto free_wdev;
 
 	/* 3 8. get WLan MAC address */
 	/*  set mac addr */
@@ -283,12 +284,13 @@ static struct adapter *rtw_sdio_if1_init(struct dvobj_priv *dvobj, const struct
 
 	return padapter;
 
-free_hal_data:
-	kfree(padapter->HalData);
-
+free_wdev:
 	rtw_wdev_unregister(padapter->rtw_wdev);
 	rtw_wdev_free(padapter->rtw_wdev);
 
+free_hal_data:
+	kfree(padapter->HalData);
+
 free_adapter:
 	if (pnetdev)
 		rtw_free_netdev(pnetdev);
-- 
2.51.0

Re: [PATCH 0/5] rtw_sdio_if1_init cleanup and small logic tweak

[Ethan Tidmore]

On Tue Mar 31, 2026 at 10:32 AM CDT, Omer El Idrissi wrote:
> This patch series does some general cleanup for rtw_sdio_if1_init. Among
> the changes are: using direct returns, removing the use of
> vendor-defined macros, adding a separate label for freeing and
> unregistering padapter->rtw_wdev.
>
> Signed-off-by: Omer El Idrissi <omer.e.idrissi@gmail.com>
>
> Omer El Idrissi (5):
>   staging: rtl8723bs: use direct returns in rtw_sdio_if1_init
>   staging: rtl8723bs: remove useless line in rtw_sdio_if1_init
>   staging: rtl8723bs: remove use of vendor-defined status macros
>   staging: rtl8723bs: replace function with error handling alternative
>   staging: rtl8723bs: add separate label for freeing rtw_wdev
>
>  drivers/staging/rtl8723bs/include/sdio_hal.h |  1 -
>  drivers/staging/rtl8723bs/os_dep/sdio_intf.c | 56 ++++++++------------
>  2 files changed, 22 insertions(+), 35 deletions(-)

Please add "staging: rtl8723bs" to your cover letter header. I have a
filter for rtl8723bs and not getting your cover letter was very confusing.

You don't have to resend the series for this reason however.

Thanks, 

ET

Re: [PATCH 3/5] staging: rtl8723bs: remove use of vendor-defined status macros

[Dan Carpenter]

On Tue, Mar 31, 2026 at 05:32:52PM +0200, Omer El Idrissi wrote:
> Remove the check for _FAIL macro on two occurrences in rtw_sdio_if1_init
> 
> Signed-off-by: Omer El Idrissi <omer.e.idrissi@gmail.com>
> ---
>  drivers/staging/rtl8723bs/os_dep/sdio_intf.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> index 8412331c4949..34ef40a86153 100644
> --- a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> +++ b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> @@ -262,7 +262,7 @@ static struct adapter *rtw_sdio_if1_init(struct dvobj_priv *dvobj, const struct
>  	padapter->intf_alloc_irq = &sdio_alloc_irq;
>  	padapter->intf_free_irq = &sdio_free_irq;
>  
> -	if (rtw_init_io_priv(padapter, sdio_set_intf_ops) == _FAIL)
> +	if (rtw_init_io_priv(padapter, sdio_set_intf_ops))

Here _FAIL is zero so this reverses the condition.  You need to change
rtw_init_io_priv() before you change the callers.  And, in fact, you
should change rtw_init_io_priv() at the same time you change the
callers.  Do it in one patch so it's easier to review.

regards,
dan carpenter

>  		goto free_hal_data;
>  
>  	rtw_hal_read_chip_version(padapter);
> @@ -275,7 +275,7 @@ static struct adapter *rtw_sdio_if1_init(struct dvobj_priv *dvobj, const struct
>  	rtw_hal_read_chip_info(padapter);
>  
>  	/* 3 7. init driver common data */
> -	if (rtw_init_drv_sw(padapter) == _FAIL)
> +	if (rtw_init_drv_sw(padapter))
>  		goto free_hal_data;
>  
>  	rtw_wdev_alloc(padapter, dvobj_to_dev(dvobj));

Re: [PATCH 4/5] staging: rtl8723bs: replace function with error handling alternative

[Dan Carpenter]

On Tue, Mar 31, 2026 at 05:32:53PM +0200, Omer El Idrissi wrote:
> Replace the use of rtw_set_hal_ops with rtw_hal_data_init in
> rtw_sdio_if1_init , which actually returns error or success and not
> void.
> rtw_set_hal_ops literally only calls rtw_hal_data_init and just ignores the
> possibility of errors.
> 

This is a behavior change and it's quite dangerous.  A lot of code only
works because there is no error handling.  We can't merge it without
testing unless it causes a security issue or something.  For example,
not checking the results of allocations could cause a crash so maybe
that's a security bug.

> @@ -250,8 +244,9 @@ static struct adapter *rtw_sdio_if1_init(struct dvobj_priv *dvobj, const struct
>  	/* 3 3. init driver special setting, interface, OS and hardware relative */
>  
>  	/* 4 3.1 set hardware operation functions */
> -	rtw_set_hal_ops(padapter);
> -
> +	/* allocates padapter->HalData */
> +	if (rtw_hal_data_init(padapter))
> +		goto free_adapter;

I don't want to see checks in this style.  I always want them to be
in this format:

	ret = rtw_hal_data_init(padapter);
	if (ret)
		...

But in this case, just leave it because adding a new check would have
to be tested.

regards,
dan carpenter

>  
>  	/* 3 5. initialize Chip version */
>  	padapter->intf_start = &sd_intf_start;
> -- 
> 2.51.0
> 

Re: [PATCH 5/5] staging: rtl8723bs: add separate label for freeing rtw_wdev

[Dan Carpenter]

On Tue, Mar 31, 2026 at 05:32:54PM +0200, Omer El Idrissi wrote:
> The original function would "unregister" and "free" padapter->rtw_wdev
> IF it ran into issues after "hardware operation functions" were set.
> The problem is that rtw_wdev isn't even allocated at that point.
> So separate the rtw_wdev_unregister and rtw_wdev_free code into it's own
> label, add error check to rtw_wdev_alloc and move it up a bit.
> When rtw_init_drv_sw fails goto free_wdev instead of free_hal_data.
> 

This commit message makes it seem like this is a bugfix but really
it's just a cleanup.

> Signed-off-by: Omer El Idrissi <omer.e.idrissi@gmail.com>
> ---
>  drivers/staging/rtl8723bs/os_dep/sdio_intf.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> index aea9b4e19874..a088dae40a19 100644
> --- a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> +++ b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> @@ -268,12 +268,13 @@ static struct adapter *rtw_sdio_if1_init(struct dvobj_priv *dvobj, const struct
>  
>  	/* 3 6. read efuse/eeprom data */
>  	rtw_hal_read_chip_info(padapter);
> +	
> +	if (rtw_wdev_alloc(padapter, dvobj_to_dev(dvobj))) 
> +		goto free_hal_data;
>  
>  	/* 3 7. init driver common data */
>  	if (rtw_init_drv_sw(padapter))
> -		goto free_hal_data;
> -
> -	rtw_wdev_alloc(padapter, dvobj_to_dev(dvobj));
> +		goto free_wdev;
>  

Then this part here re-orders things and adds new error checking
which seems risky.  Adding error checking for the rtw_wdev_alloc()
function is probably the right thing, but it needs to be done
separately and labeled with a Fixes tag.  I don't know why you
re-ordered things and it isn't explained in the commit message.

>  	/* 3 8. get WLan MAC address */
>  	/*  set mac addr */
> @@ -283,12 +284,13 @@ static struct adapter *rtw_sdio_if1_init(struct dvobj_priv *dvobj, const struct
>  
>  	return padapter;
>  
> -free_hal_data:
> -	kfree(padapter->HalData);
> -
> +free_wdev:
>  	rtw_wdev_unregister(padapter->rtw_wdev);
>  	rtw_wdev_free(padapter->rtw_wdev);
>  
> +free_hal_data:
> +	kfree(padapter->HalData);
> +

I don't have a problem with re-ordering the cleanup.  That
stuff probably has never been tested and it's easy to review.
https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/

So the commit message would say something like:

"Generally, in a clean up ladder, we would free resources in the
reverse order from how they are allocated.  Here we potentially
call rtw_wdev_unregister(padapter->rtw_wdev) and
rtw_wdev_free(padapter->rtw_wdev) before the padapter->rtw_wdev
has been allocated.  This does not cause a problem because those
functions check for NULL at the start and return directly, but it
is a bit messy.  Re-order the error handling in the cannonical way."

regards,
dan carpenter

>  free_adapter:
>  	if (pnetdev)
>  		rtw_free_netdev(pnetdev);
> -- 
> 2.51.0
>