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. I like the idea, however
during my review I have found that there are several issues.

Patch 1:
While the patch makes it visually cleaner, this patch introduces a 
significant risk of a kernel panic. struct net_device *pnetdev; is 
declared on the stack but not initialized to NULL. If the function 
fails early, 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 rtw_free_netdev() on a random memory address.

Patch 2:
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. You should stick to the 
atomic patch per logical change rule. So either keep the whitespace 
as is or move it into dedicated patch.

Patch 3 and 4:
I have checked 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. 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. You cannot simplify these checks until the 
functions themselves are refactored to return standard kernel error 
codes.

Patch 5:
This patch has the same issue mentioned above regarding return values.
It also introduces a trailing whitespace on the empty line added 
before the rtw_wdev_alloc check. Please run scripts/checkpatch.pl 
--strict on your patches before submission.

First you should 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. So 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.

Secondly, if you wish to proceed with the macro removal, provide a 
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. Once the functions follow 
standard conventions, the cleanup in patches 3-5 will be correct.

Best regards,
Luka Gejak