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

[Dan Carpenter <error27@gmail.com>]

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
>