* Re: [PATCH] checkpatch: add ALLOC_UNNECESSARY_ARRAY test
2022-08-20 21:41 [PATCH] checkpatch: add ALLOC_UNNECESSARY_ARRAY test Kenneth Lee
@ 2022-08-21 0:47 ` Joe Perches
0 siblings, 0 replies; 2+ messages in thread
From: Joe Perches @ 2022-08-21 0:47 UTC (permalink / raw)
To: Kenneth Lee, apw, dwaipayanray1, lukas.bulwahn; +Cc: linux-kernel
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.
^ permalink raw reply [flat|nested] 2+ messages in thread