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

[Luka Gejak <luka.gejak@linux.dev>]

Hi Omer,
Thank you for submitting this patch series. Efforts to clean up the 
initialization paths in these legacy Realtek staging drivers are 
always welcome, as they are a necessary step toward aligning the code 
with upstream kernel standards and eventually moving the driver out of
staging.
I have performed a detailed technical audit of the series. While the 
high-level goal of improving readability is correct, there are several
critical regressions and architectural issues that prevent patch 
series from being accepted in its current form. 
Please see the detailed breakdown below:
Patch 1/5: Use direct returns in rtw_sdio_if1_init
While the move toward direct returns is visually cleaner, this patch 
introduces a significant risk of a kernel panic. In the original code,
the status variable guarded the cleanup labels. By removing it, you 
have exposed an uninitialized variable bug:
struct net_device *pnetdev; is declared on the stack but not 
initialized to NULL. If the function fails early (e.g., if vzalloc for
padapter fails or an early hardware check triggers a goto), the 
execution jumps to the free_adapter label. The logic then evaluates 
if (pnetdev). Since pnetdev contains uninitialized stack garbage, this
check will likely evaluate to true, causing the kernel to attempt to 
call rtw_free_netdev() on a random memory address.
Requirement: For v2, you must initialize pnetdev = NULL; at the top of
the function to ensure the cleanup path is safe. Check below for more 
information about next steps for patch 2.
Patch 2/5: Remove useless line in rtw_sdio_if1_init
This patch is technically correct in its removal of the redundant 
padapter assignment. However, the diff shows that it also performs 
whitespace cleanup by removing an empty line (the "two minuses" in the
diff). To maintain a clean git history, we follow the "one change per 
patch" rule. Mixing dead code removal with whitespace adjustments 
makes the history harder to parse. Please either keep the whitespace 
as is or move all stylistic cleanups into a dedicated "Cleanup 
whitespace" patch within the series. 
Patch 3/5 & 4/5: Logic simplified for rtw_init_io_priv/rtw_init_drv_sw
I have verified in drivers/staging/rtl8723bs/include/osdep_service.h 
that this driver defines _SUCCESS as 1 and _FAIL as 0. This is the 
inverse of the standard Linux kernel convention (where 0 is success 
and -ERRNO is failure). By changing the check to if (func()), your new
logic triggers the goto error path when the function returns 1 
(Success). This would result in a driver that fails to probe entirely,
as every successful initialization step would be treated as a failure.
Requirement: We cannot simplify these call-site checks until the 
underlying functions themselves are refactored to return standard 
kernel error codes.
Patch 5/5: Logic tweak for rtw_wdev_alloc
Logic: This suffers from the same inversion issue mentioned above 
regarding rtw_wdev_alloc return values. This patch also introduces a 
trailing whitespace on the empty line added before the rtw_wdev_alloc 
check. Please ensure you run scripts/checkpatch.pl --strict on your 
patches before submission to catch these formatting errors.

I recommend the following path for v2:
Step 1: Submit a patch that properly initializes pointers to NULL to 
make the cleanup paths safe from crash. Although initializing pnetdev 
to NULL prevents the immediate crash, the cleanup logic remains 
fragile. In v2, please consider refactoring the error path to use a 
proper LIFO (Last-In, First-Out) label stack. Each goto should jump 
only to the cleanup steps for resources that have actually been 
allocated up to that point. This avoids redundant checks and potential
double-frees or leaks.
Step 2: If you wish to proceed with the macro removal, provide a 
preparatory series that refactors the internal return values of 
rtw_init_io_priv, rtw_init_drv_sw, and rtw_hal_data_init to standard 0
(Success) / -ERR (Failure) conventions.
Step 3: Once the functions follow standard conventions, the cleanup in
Patches 3-5 will be correct.
Thank you again for your contribution. I look forward to reviewing the
revised series.
Best regards,
Luka Gejak