linux-kernel - Re: [PATCH] staging: rtl8723bs: rtw_mlme

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <20fad314-6b8f-429a-bb39-921ece6e46ab@gmail.com>
Date: Thu, 1 Aug 2024 07:47:29 +0200
From: Philipp Hortmann <philipp.g.hortmann@...il.com>
To: Santiago Ruano Rincón <santiagorr@...eup.net>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
 helen.koike@...labora.com, ~lkcamp/patches@...ts.sr.ht
Subject: Re: [PATCH] staging: rtl8723bs: rtw_mlme_ext: replace spaces by tabs

On 8/1/24 00:53, Santiago Ruano Rincón wrote:
> Hello Philipp,
> 
> El 30/07/24 a las 21:18, Philipp Hortmann escribió:
>> On 7/30/24 10:05, Santiago Ruano Rincón wrote:
>>> Fix checkpatch error "ERROR: code indent should use tabs where possible"
>>> in include/rtw_mlme_ext.h:388.
>>>
>>> Signed-off-by: Santiago Ruano Rincón <santiagorr@...eup.net>
>>>
>>> ---
>>> I am (mostly) a newcommer. Could you please tell me if there is anything
>>> wrong with this patch? Thank you!
>>>
>>> checkpatch reports a warning in line 387:
>>> "WARNING: line length of 135 exceeds 100 columns"
>>> Should I fix the warning in the same patch, or should I send a second
>>> patch to fix it?
>>> ---
>>> ---
>>>    drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h b/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
>>> index 5b8574f5a..720aeeb00 100644
>>> --- a/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
>>> +++ b/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
>>> @@ -384,8 +384,8 @@ struct mlme_ext_priv {
>>>    	unsigned char default_supported_mcs_set[16];
>>>    	struct ss_res		sitesurvey_res;
>>> -	struct mlme_ext_info mlmext_info;/* for sta/adhoc mode, including current scanning/connecting/connected related info. */
>>> -                                                     /* for ap mode, network includes ap's cap_info */
>>> +	struct mlme_ext_info mlmext_info;	/* for sta/adhoc mode, including current scanning/connecting/connected related info. */
>>> +						/* for ap mode, network includes ap's cap_info */
>>>    	struct timer_list		survey_timer;
>>>    	struct timer_list		link_timer;
>>>    	struct timer_list		sa_query_timer;
>>
>>
>> Hi Santiago,
>>
>> it could be two patches or one. You are only allowed to do one thing at a
>> time. So it is also a little depending on your description. The way you
>> describe this it is more like two patches.
>> If your description would be like:
>>
>> Insert line break to comment, that exceeds 100 columns, to improve
>> readability.
>>
>> It would be OK to correct the indent as well.
>>
>> If you send in a second version of this patch please use a change history.
>> Description from Dan under:
>> https://staticthinking.wordpress.com/2022/07/27/how-to-send-a-v2-patch/
>>
>> In case of questions feel free to contact me directly.
>>
>> Thanks for your support.
>>
>> Bye Philipp
> 
> Thanks a lot for your explanation. Much appreciated.
> 
> Greg has added the commit to his staging-testing branch. So I guess
> the only choice to fix that warning (a CHECK, actually) now is through a
> separate commit.

That is correct.

> Maybe several warnings/checks of the same type in the same file could be
> handled together in a single commit? 

Yes you can fix several issues in one commit if they are of same type. 
Example: Missing linebreaks

But is it worth the effort to fix
> them (when they are non-ERROR), or does it represent more work for you?

If you consider checkpatch warnings and errors it is more complicated. 
We want to fix all errors and warnings when it makes sense. Sometimes 
checkpatch is wrong. Finally you always need to judge if your patch is 
an improvement for readability.

Example:
Checkpatch warnings about not needed brackets in logical statements are 
most likely to be wrong. Reason is that the readability for those 
logical statements is extremely lowered when omitting the brackets.

I cannot respond to you alone as Greg insists on all mails to be on the 
mailing list.
https://lore.kernel.org/linux-staging/001101dae31f$d5ef44d0$81cdce70$@samsung.com/T/#m83a1aac71249e9d6b658bc5e4a7190c757d3547e

Thanks for your support.

Bye Philipp