Using calloc|malloc_array(1, ...) can be simplified to zalloc|malloc(...)
and improves semantics. This is because we are only allocating one element
so there is no need to use the array version of the methods.
This can be applied to kvmalloc_array, kvcalloc, kcalloc, devm_kcalloc,
and devm_kmalloc_array. Note that there is no devm_kvmalloc_array or
devm_kvcalloc, so the regex does not check for this.
Signed-off-by: Kenneth Lee <[email protected]>
---
This is my first patch that is not a trivial cleanup, so please let me
know if I am approaching something wrong. Also unsure if the warning
name should be something else besides ALLOC_UNNECESSARY_ARRAY
---
scripts/checkpatch.pl | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 79e759aac543..83d3a9f308ea 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -7079,6 +7079,19 @@ sub process {
"$1 uses number as first arg, sizeof is generally wrong\n" . $herecurr);
}
+# check for use of unnecessary array alloc methods
+# calloc|malloc_array(1, ...) should be zalloc|malloc(...)
+ if ($line =~ /(\b(?:devm_(?:kcalloc|kmalloc_array))|\b(?:kv|k)(?:calloc|malloc_array))\s*\(\s*1\s*,/) {
+ my $newfunc = "kmalloc";
+ $newfunc = "kvmalloc" if ($1 eq "kvmalloc_array");
+ $newfunc = "kvzalloc" if ($1 eq "kvcalloc");
+ $newfunc = "kzalloc" if ($1 eq "kcalloc");
+ $newfunc = "devm_kzalloc" if ($1 eq "devm_kcalloc");
+ $newfunc = "devm_kmalloc" if ($1 eq "devm_kmalloc_array");
+ WARN("ALLOC_UNNECESSARY_ARRAY",
+ "$1(1, ...) can be simplified to $newfunc(...)\n" . $herecurr);
+ }
+
# check for multiple semicolons
if ($line =~ /;\s*;\s*$/) {
if (WARN("ONE_SEMICOLON",
--
2.31.1
On Sat, 2022-08-20 at 14:41 -0700, Kenneth Lee wrote:
> Using calloc|malloc_array(1, ...) can be simplified to zalloc|malloc(...)
> and improves semantics. This is because we are only allocating one element
> so there is no need to use the array version of the methods.
I don't know if this is particularly useful.
$ git grep -P '(\b(?:devm_(?:kcalloc|kmalloc_array))|\b(?:kv|k)(?:calloc|malloc_array))\s*\(\s*1\s*,' | wc -l
48
If a realloc or equivalent is used to expand the number of elements,
then using 1 as the initial element count seems appropriate.
Have you checked the existing uses?
If these are just using a kcalloc style as a means to zero memory,
then the direct use of the zero allocation equivalent is appropriate.
> This is my first patch that is not a trivial cleanup, so please let me
> know if I am approaching something wrong. Also unsure if the warning
> name should be something else besides ALLOC_UNNECESSARY_ARRAY
Naming is pretty arbitrary, I'm fine with the name.
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -7079,6 +7079,19 @@ sub process {
> "$1 uses number as first arg, sizeof is generally wrong\n" . $herecurr);
> }
>
> +# check for use of unnecessary array alloc methods
> +# calloc|malloc_array(1, ...) should be zalloc|malloc(...)
> + if ($line =~ /(\b(?:devm_(?:kcalloc|kmalloc_array))|\b(?:kv|k)(?:calloc|malloc_array))\s*\(\s*1\s*,/) {
> + my $newfunc = "kmalloc";
> + $newfunc = "kvmalloc" if ($1 eq "kvmalloc_array");
> + $newfunc = "kvzalloc" if ($1 eq "kvcalloc");
> + $newfunc = "kzalloc" if ($1 eq "kcalloc");
> + $newfunc = "devm_kzalloc" if ($1 eq "devm_kcalloc");
> + $newfunc = "devm_kmalloc" if ($1 eq "devm_kmalloc_array");
> + WARN("ALLOC_UNNECESSARY_ARRAY",
> + "$1(1, ...) can be simplified to $newfunc(...)\n" . $herecurr);
This doesn't work with any alloc using dev_<foo> as these functions
all take a struct device * as the first argument.
And perhaps this would be better/more easily extensible with a hash
instead of a list.
Something like:
our %alloc_array = {
'devm_kcalloc' => 'devm_kzalloc',
'devm_kmalloc_array' => 'devm_kmalloc',
'kvmalloc_array' => 'kvmalloc',
'kvcalloc' => 'kvzalloc',
'kcalloc' => 'kzalloc',
'kmalloc_array' => 'kmalloc',
};
#Create a search pattern for all these strings to speed up a loop below
our $alloc_array_search = "";
foreach my $entry (keys %one_alloc_array) {
$alloc_array_search .= '|' if ($alloc_array_search ne "");
$alloc_array_search .= $entry;
}
$alloc_array_search = "(?:${one_alloc_array_search})";
...
if ($line =~ /(\b($alloc_array)\s*\(\s*1\s*,/) {
WARN("ALLOC_UNNECESSARY_ARRAY",
"$1(1, ...) can be simplified to $alloc_array->$1(...)\n" . $herecurr);
Ideally, this would use $stat and check the positional argument.
Maybe something like:
our @alloc_array = (
[ 'devm_kcalloc', 'devm_kzalloc', 1 ],
[ 'devm_kmalloc_array', 'devm_kmalloc', 1 ],
[ 'kvmalloc_array', 'kvmalloc', 0 ],
[ 'kvcalloc', 'kvzalloc', 0 ],
[ 'kcalloc', 'kzalloc', 0 ],
[ 'kmalloc_array', 'kmalloc', 0 ],
);
#Create a search pattern for all these strings to speed up a loop below
our $alloc_array_search = "";
foreach my $entry (keys %one_alloc_array) {
$alloc_array_search .= '|' if ($alloc_array_search ne "");
$alloc_array_search .= $entry->[0];
}
$alloc_array_search = "(?:${one_alloc_array_search})";
Look at the use of mode_permission_funcs for an example of
skipping to a particular argument.