[Linux-kernel-mentees] [PATCH] checkpatch: add fix option for CONSIDER_KSTRTO

[Linux-kernel-mentees] [PATCH] checkpatch: add fix option for CONSIDER_KSTRTO

[Aditya Srivastava]

Checkpatch recommends the usage of kstrto* over simple_strto* and
strict_strto*, as these are obsolete.

E.g., running checkpatch on commit f79b3f338564 ("ftrace: Allow
enabling of filters via index of available_filter_functions") reports
this warning:

WARNING: simple_strtoul is obsolete, use kstrtoul instead
+	long index = simple_strtoul(func_g->search, NULL, 0);

Provide a fix by replacing simple_strto* and strict_strto* with kstrto*

Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
---
 scripts/checkpatch.pl | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7b086d1cd6c2..ca5bb5a3f8f6 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6980,8 +6980,12 @@ sub process {
 
 # recommend kstrto* over simple_strto* and strict_strto*
 		if ($line =~ /\b((simple|strict)_(strto(l|ll|ul|ull)))\s*\(/) {
-			WARN("CONSIDER_KSTRTO",
-			     "$1 is obsolete, use k$3 instead\n" . $herecurr);
+			if (WARN("CONSIDER_KSTRTO",
+				 "$1 is obsolete, use k$3 instead\n" . $herecurr) &&
+			    $fix) {
+				my $kstrto = "k$3";
+				$fixed[$fixlinenr] =~ s/$1/$kstrto/;
+			}
 		}
 
 # check for __initcall(), use device_initcall() explicitly or more appropriate function please
-- 
2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] checkpatch: add fix option for CONSIDER_KSTRTO

[Lukas Bulwahn]

On Mon, Dec 7, 2020 at 1:43 PM Aditya Srivastava <yashsri421@gmail.com> wrote:
>
> Checkpatch recommends the usage of kstrto* over simple_strto* and
> strict_strto*, as these are obsolete.
>
> E.g., running checkpatch on commit f79b3f338564 ("ftrace: Allow
> enabling of filters via index of available_filter_functions") reports
> this warning:
>
> WARNING: simple_strtoul is obsolete, use kstrtoul instead
> +       long index = simple_strtoul(func_g->search, NULL, 0);
>
> Provide a fix by replacing simple_strto* and strict_strto* with kstrto*
>
> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> ---
>  scripts/checkpatch.pl | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 7b086d1cd6c2..ca5bb5a3f8f6 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -6980,8 +6980,12 @@ sub process {
>
>  # recommend kstrto* over simple_strto* and strict_strto*
>                 if ($line =~ /\b((simple|strict)_(strto(l|ll|ul|ull)))\s*\(/) {
> -                       WARN("CONSIDER_KSTRTO",
> -                            "$1 is obsolete, use k$3 instead\n" . $herecurr);
> +                       if (WARN("CONSIDER_KSTRTO",
> +                                "$1 is obsolete, use k$3 instead\n" . $herecurr) &&
> +                           $fix) {
> +                               my $kstrto = "k$3";
> +                               $fixed[$fixlinenr] =~ s/$1/$kstrto/;
> +                       }
>                 }
>

Does this only warn on added lines?

The fix looks good; I just do not know if the rule is applicable in
full generality.

You might want to check a few instances if that rule actually makes
sense and is accepted.

Lukas

>  # check for __initcall(), use device_initcall() explicitly or more appropriate function please
> --
> 2.17.1
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] checkpatch: add fix option for CONSIDER_KSTRTO

[Aditya]

On 7/12/20 6:24 pm, Lukas Bulwahn wrote:
> On Mon, Dec 7, 2020 at 1:43 PM Aditya Srivastava <yashsri421@gmail.com> wrote:
>>
>> Checkpatch recommends the usage of kstrto* over simple_strto* and
>> strict_strto*, as these are obsolete.
>>
>> E.g., running checkpatch on commit f79b3f338564 ("ftrace: Allow
>> enabling of filters via index of available_filter_functions") reports
>> this warning:
>>
>> WARNING: simple_strtoul is obsolete, use kstrtoul instead
>> +       long index = simple_strtoul(func_g->search, NULL, 0);
>>
>> Provide a fix by replacing simple_strto* and strict_strto* with kstrto*
>>
>> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
>> ---
>>  scripts/checkpatch.pl | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index 7b086d1cd6c2..ca5bb5a3f8f6 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -6980,8 +6980,12 @@ sub process {
>>
>>  # recommend kstrto* over simple_strto* and strict_strto*
>>                 if ($line =~ /\b((simple|strict)_(strto(l|ll|ul|ull)))\s*\(/) {
>> -                       WARN("CONSIDER_KSTRTO",
>> -                            "$1 is obsolete, use k$3 instead\n" . $herecurr);
>> +                       if (WARN("CONSIDER_KSTRTO",
>> +                                "$1 is obsolete, use k$3 instead\n" . $herecurr) &&
>> +                           $fix) {
>> +                               my $kstrto = "k$3";
>> +                               $fixed[$fixlinenr] =~ s/$1/$kstrto/;
>> +                       }
>>                 }
>>
> 
> Does this only warn on added lines?
> No, currently it does not seem to check for added lines. However, I
don't think we need to check for added lines as we are just
substituting in the line (ie no addition or removal)

> The fix looks good; I just do not know if the rule is applicable in
> full generality.
> 
> You might want to check a few instances if that rule actually makes
> sense and is accepted.
> 
I checked for the use of all three formats of function (ie
simple_strto*, strict_strto* and kstrto*) for all the files.

The total count of usage of kstrto* is 2854, whereas
for simple_strto* = 867
strict_strto* = 7

This is the complete list of files with their count of occurrences:

For kstrto* =
https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/consider_kstrto/kstrt.txt

For simple_strto* =
https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/consider_kstrto/simple_strt.txt

For strict_strto* =
https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/consider_kstrto/strict_strt.txt

What do you think?

Thanks
Aditya


> Lukas
> 
>>  # check for __initcall(), use device_initcall() explicitly or more appropriate function please
>> --
>> 2.17.1
>>

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] checkpatch: add fix option for CONSIDER_KSTRTO

[Aditya]

On 7/12/20 8:28 pm, Aditya wrote:
> On 7/12/20 6:24 pm, Lukas Bulwahn wrote:
>> On Mon, Dec 7, 2020 at 1:43 PM Aditya Srivastava <yashsri421@gmail.com> wrote:
>>>
>>> Checkpatch recommends the usage of kstrto* over simple_strto* and
>>> strict_strto*, as these are obsolete.
>>>
>>> E.g., running checkpatch on commit f79b3f338564 ("ftrace: Allow
>>> enabling of filters via index of available_filter_functions") reports
>>> this warning:
>>>
>>> WARNING: simple_strtoul is obsolete, use kstrtoul instead
>>> +       long index = simple_strtoul(func_g->search, NULL, 0);
>>>
>>> Provide a fix by replacing simple_strto* and strict_strto* with kstrto*
>>>
>>> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
>>> ---
>>>  scripts/checkpatch.pl | 8 ++++++--
>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>> index 7b086d1cd6c2..ca5bb5a3f8f6 100755
>>> --- a/scripts/checkpatch.pl
>>> +++ b/scripts/checkpatch.pl
>>> @@ -6980,8 +6980,12 @@ sub process {
>>>
>>>  # recommend kstrto* over simple_strto* and strict_strto*
>>>                 if ($line =~ /\b((simple|strict)_(strto(l|ll|ul|ull)))\s*\(/) {
>>> -                       WARN("CONSIDER_KSTRTO",
>>> -                            "$1 is obsolete, use k$3 instead\n" . $herecurr);
>>> +                       if (WARN("CONSIDER_KSTRTO",
>>> +                                "$1 is obsolete, use k$3 instead\n" . $herecurr) &&
>>> +                           $fix) {
>>> +                               my $kstrto = "k$3";
>>> +                               $fixed[$fixlinenr] =~ s/$1/$kstrto/;
>>> +                       }
>>>                 }
>>>
>>
>> Does this only warn on added lines?
>> No, currently it does not seem to check for added lines. However, I
> don't think we need to check for added lines as we are just
> substituting in the line (ie no addition or removal)
> 
>> The fix looks good; I just do not know if the rule is applicable in
>> full generality.
>>
>> You might want to check a few instances if that rule actually makes
>> sense and is accepted.
>>
> I checked for the use of all three formats of function (ie
> simple_strto*, strict_strto* and kstrto*) for all the files.
> 
> The total count of usage of kstrto* is 2854, whereas
> for simple_strto* = 867
> strict_strto* = 7
> 
> This is the complete list of files with their count of occurrences:
> 
> For kstrto* =
> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/consider_kstrto/kstrt.txt
> 
> For simple_strto* =
> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/consider_kstrto/simple_strt.txt
> 
> For strict_strto* =
> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/consider_kstrto/strict_strt.txt
> 
> What do you think?
> 

Hi Lukas

You probably missed this mail.

Thanks
Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees