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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 31/03/2026 22:25, Luka Gejak wrote:
> 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.


This is absolutely unreadable review. Please provide reviews inline, as
expected in Linux kernel review style.

It all looks like some output of microslop with:

"I have performed a detailed technical audit of the series. "
"Requirement: We cannot simplify these call-site checks until the "
"To maintain a clean git history, we follow the "one change per patch
rule. "

Only AI reasons that way uses such idealized teaching explanation.


Best regards,
Krzysztof