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

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

[Luka Gejak]

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

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

[Greg KH]

On Wed, Apr 01, 2026 at 12:17:41PM +0200, Luka Gejak wrote:
> 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.

<snip>

Please review things "inline" like a human does.  With the code in
place, and the comments right below the code.

As you are using AI for this, PLEASE use an AI tool that actually works.
Use the ones that we have public, which DOES get this right and puts the
review comments in the correct places, and in the correct format.

If I were confronted with a review like this on patch 0/X, it would be
very hard for me to actually digest and handle it, given that the
context is not there at all.

I suggest you take some time off, work on your review workflow, and
then, if you still want to help out with reviews, do it in the proper
way that we all have been doing for decades.

thanks,

greg k-h

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

[Greg KH]

On Wed, Apr 01, 2026 at 12:29:52PM +0200, Greg KH wrote:
> On Wed, Apr 01, 2026 at 12:17:41PM +0200, Luka Gejak wrote:
> > 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.
> 
> <snip>
> 
> Please review things "inline" like a human does.  With the code in
> place, and the comments right below the code.
> 
> As you are using AI for this, PLEASE use an AI tool that actually works.
> Use the ones that we have public, which DOES get this right and puts the
> review comments in the correct places, and in the correct format.
> 
> If I were confronted with a review like this on patch 0/X, it would be
> very hard for me to actually digest and handle it, given that the
> context is not there at all.
> 
> I suggest you take some time off, work on your review workflow, and
> then, if you still want to help out with reviews, do it in the proper
> way that we all have been doing for decades.

And your bot has exploded, sending it's response multiple times to the
list for some reason.  I think I need to add your email address to the
ignore list for now.

greg k-h

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

[Luka Gejak]

On April 1, 2026 12:50:17 PM GMT+02:00, Greg KH <gregkh@linuxfoundation.org> wrote:
>On Wed, Apr 01, 2026 at 12:29:52PM +0200, Greg KH wrote:
>> On Wed, Apr 01, 2026 at 12:17:41PM +0200, Luka Gejak wrote:
>> > 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.
>> 
>> <snip>
>> 
>> Please review things "inline" like a human does.  With the code in
>> place, and the comments right below the code.
>> 
>> As you are using AI for this, PLEASE use an AI tool that actually works.
>> Use the ones that we have public, which DOES get this right and puts the
>> review comments in the correct places, and in the correct format.
>> 
>> If I were confronted with a review like this on patch 0/X, it would be
>> very hard for me to actually digest and handle it, given that the
>> context is not there at all.
>> 
>> I suggest you take some time off, work on your review workflow, and
>> then, if you still want to help out with reviews, do it in the proper
>> way that we all have been doing for decades.
>
>And your bot has exploded, sending it's response multiple times to the
>list for some reason.  I think I need to add your email address to the
>ignore list for now.
>
>greg k-h

Not a bot, but rather, I'm at my phone rn and thunderbird complained 
about error while sending email so I resend it.
Best regards,
Luka Gejak

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

[Luka Gejak]

On April 1, 2026 12:29:52 PM GMT+02:00, Greg KH <gregkh@linuxfoundation.org> wrote:
>On Wed, Apr 01, 2026 at 12:17:41PM +0200, Luka Gejak wrote:
>> 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.
>
><snip>
>
>Please review things "inline" like a human does.  With the code in
>place, and the comments right below the code.
>
>As you are using AI for this, PLEASE use an AI tool that actually works.
>Use the ones that we have public, which DOES get this right and puts the
>review comments in the correct places, and in the correct format.
>
>If I were confronted with a review like this on patch 0/X, it would be
>very hard for me to actually digest and handle it, given that the
>context is not there at all.
>
>I suggest you take some time off, work on your review workflow, and
>then, if you still want to help out with reviews, do it in the proper
>way that we all have been doing for decades.
>
>thanks,
>
>greg k-h

Hi Greg,
Thank you for the guidance. I understand now why the summary-style 
review is difficult to process. I will take your advice, step back, 
and fix my workflow.
Best regards,
Luka Gejak

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

[Dan Carpenter]

On Wed, Apr 01, 2026 at 12:17:41PM +0200, Luka Gejak wrote:
> 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.

That's not true.

regards,
dan carpenter

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

[Luka Gejak]

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

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

[Luka Gejak]

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

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

[Greg KH]

On Tue, Mar 31, 2026 at 10:25:22PM +0200, 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 

<snip>

Luka, this review REALLY looks like it was AI generated, and then
cut-pasted into here.  Please do not do that.

If you wish to use AI to generate reviews, great, then use some of the
tools we already have, and notify the submitter that this is an AI
report and that it should be treated as such.  Don't try to pass it off
as a human review that must actually be trusted.

thanks,

greg k-h

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

[Luka Gejak]

On April 1, 2026 10:29:37 AM GMT+02:00, Greg KH <gregkh@linuxfoundation.org> wrote:
>On Tue, Mar 31, 2026 at 10:25:22PM +0200, 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 
>
><snip>
>
>Luka, this review REALLY looks like it was AI generated, and then
>cut-pasted into here.  Please do not do that.
>
>If you wish to use AI to generate reviews, great, then use some of the
>tools we already have, and notify the submitter that this is an AI
>report and that it should be treated as such.  Don't try to pass it off
>as a human review that must actually be trusted.
>
>thanks,
>
>greg k-h

Hi Greg,
You're right, I used a tool to help format the response because 
English is not my native language and I wanted the review to be clear.
I see now that it made the response look like a bot report and I 
apologize for that. I'll stick to writing reviews manually going 
forward. However the technical issues I pointed out (like the inverted
_SUCCESS/_FAIL logic in the staging headers and the uninitialized 
pnetdev pointer) are real regressions I found while auditing the code 
on my local tree. I'll make sure future feedback is direct and clearly
identified if I use any tooling.
Thanks for the correction.
Best regards,
Luka Gejak

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

[Konstantin Ryabitsev]

On Wed, Apr 01, 2026 at 11:46:59AM +0200, Luka Gejak wrote:
> You're right, I used a tool to help format the response because 
> English is not my native language and I wanted the review to be clear.
> I see now that it made the response look like a bot report and I 
> apologize for that. I'll stick to writing reviews manually going 
> forward. However the technical issues I pointed out (like the inverted
> _SUCCESS/_FAIL logic in the staging headers and the uninitialized 
> pnetdev pointer) are real regressions I found while auditing the code 
> on my local tree. I'll make sure future feedback is direct and clearly
> identified if I use any tooling.

If you want to try something out, there is a new feature in b4 that allows
integrating agent reviews in your response in a format that would be better
received by kernel developers:

https://b4.docs.kernel.org/en/latest/reviewer/getting-started.html

HOWEVER, it does not remove the need to verify what the agent wrote -- LLMs
are *routinely* wrong.

Review and include only those comments that you have verified and are sure
about -- never anything that "looks valid," because you are likely to just
waste everyone's time. If you aren't absolutely sure that the agent's findings
are correct, do not include them, no matter the temptation.

Best regards,
-- 
KR

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

[Krzysztof Kozlowski]

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

[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

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