Messages in this thread Patch in this message | | | Subject | Re: [PATCH] checkpatch: add ALLOC_UNNECESSARY_ARRAY test | From | Joe Perches <> | Date | Sat, 20 Aug 2022 20:47:07 -0400 |
| |
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.
| |