From: Markus Elfring <[email protected]>
Date: Sat, 23 Dec 2023 20:02:02 +0100
A few update suggestions were taken into account
from static source code analysis.
Markus Elfring (2):
One function call less in vas_window_alloc() after error detection
Return directly after a failed kasprintf() in map_paste_region()
arch/powerpc/platforms/powernv/vas-window.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
--
2.43.0
From: Markus Elfring <[email protected]>
Date: Sat, 23 Dec 2023 19:35:13 +0100
The kfree() function was called in one case by the
vas_window_alloc() function during error handling
even if the passed variable contained a null pointer.
This issue was detected by using the Coccinelle software.
Thus use another label.
Signed-off-by: Markus Elfring <[email protected]>
---
arch/powerpc/platforms/powernv/vas-window.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c
index b664838008c1..b51219b4b698 100644
--- a/arch/powerpc/platforms/powernv/vas-window.c
+++ b/arch/powerpc/platforms/powernv/vas-window.c
@@ -545,7 +545,7 @@ static struct pnv_vas_window *vas_window_alloc(struct vas_instance *vinst)
window = kzalloc(sizeof(*window), GFP_KERNEL);
if (!window)
- goto out_free;
+ goto release_window_id;
window->vinst = vinst;
window->vas_win.winid = winid;
@@ -559,6 +559,7 @@ static struct pnv_vas_window *vas_window_alloc(struct vas_instance *vinst)
out_free:
kfree(window);
+release_window_id:
vas_release_window_id(&vinst->ida, winid);
return ERR_PTR(-ENOMEM);
}
--
2.43.0
From: Markus Elfring <[email protected]>
Date: Sat, 23 Dec 2023 19:48:09 +0100
Return directly after a call of the function “kasprintf” failed
at the beginning.
Signed-off-by: Markus Elfring <[email protected]>
---
arch/powerpc/platforms/powernv/vas-window.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c
index b51219b4b698..2f7d1850b1fa 100644
--- a/arch/powerpc/platforms/powernv/vas-window.c
+++ b/arch/powerpc/platforms/powernv/vas-window.c
@@ -78,7 +78,7 @@ static void *map_paste_region(struct pnv_vas_window *txwin)
name = kasprintf(GFP_KERNEL, "window-v%d-w%d", txwin->vinst->vas_id,
txwin->vas_win.winid);
if (!name)
- goto free_name;
+ return ERR_PTR(-ENOMEM);
txwin->paste_addr_name = name;
vas_win_paste_addr(txwin, &start, &len);
--
2.43.0
> A few update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (2):
I would appreciate a bit more information about the reasons
why this patch series was rejected.
> One function call less in vas_window_alloc() after error detection
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/[email protected]/
> Return directly after a failed kasprintf() in map_paste_region()
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/[email protected]/
> arch/powerpc/platforms/powernv/vas-window.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
Regards,
Markus
Markus Elfring <[email protected]> writes:
>> A few update suggestions were taken into account
>> from static source code analysis.
>>
>> Markus Elfring (2):
>
> I would appreciate a bit more information about the reasons
> why this patch series was rejected.
>
>
>> One function call less in vas_window_alloc() after error detection
>
> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/[email protected]/
It introduced a new goto and label to avoid a kfree(NULL) call, but
kfree() explicitly accepts NULL and handles it. So it complicates the
source code for no gain.
>> Return directly after a failed kasprintf() in map_paste_region()
>
> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/[email protected]/
Basically the same reasoning. And it also changes the function from
having two return paths (success and error), to three.
cheers
Le 16/04/2024 à 13:11, Michael Ellerman a écrit :
> Markus Elfring <[email protected]> writes:
>>> A few update suggestions were taken into account
>>> from static source code analysis.
>>>
>>> Markus Elfring (2):
>>
>> I would appreciate a bit more information about the reasons
>> why this patch series was rejected.
>>
>>
>>> One function call less in vas_window_alloc() after error detection
>>
>> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/[email protected]/
>
> It introduced a new goto and label to avoid a kfree(NULL) call, but
> kfree() explicitly accepts NULL and handles it. So it complicates the
> source code for no gain.
This is explicit in Kernel documentation:
/**
* kfree - free previously allocated memory
* @object: pointer returned by kmalloc() or kmem_cache_alloc()
*
* If @object is NULL, no operation is performed.
*/
That's exactly the same behaviour as free() in libc.
So Coccinelle should be fixed if it reports an error for that.
>
>>> Return directly after a failed kasprintf() in map_paste_region()
>>
>> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/[email protected]/
>
> Basically the same reasoning. And it also changes the function from
> having two return paths (success and error), to three.
>
Looking at that function, I however see a missing region release when
ioremap_cache() fails.
Christophe
>>> One function call less in vas_window_alloc() after error detection
>>
>> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/[email protected]/
>
> It introduced a new goto and label to avoid a kfree(NULL) call, but
> kfree() explicitly accepts NULL and handles it. So it complicates the
> source code for no gain.
I proposed to avoid such a redundant function call for the affected
exception handling.
>>> Return directly after a failed kasprintf() in map_paste_region()
>>
>> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/[email protected]/
>
> Basically the same reasoning. And it also changes the function from
> having two return paths (success and error), to three.
See also a corresponding advice once more from the section “7) Centralized exiting of functions”:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.9-rc4#n532
Regards,
Markus
> This is explicit in Kernel documentation:
>
> /**
> * kfree - free previously allocated memory
> * @object: pointer returned by kmalloc() or kmem_cache_alloc()
> *
> * If @object is NULL, no operation is performed.
> */
>
> That's exactly the same behaviour as free() in libc.
>
> So Coccinelle should be fixed if it reports an error for that.
Redundant function calls can occasionally be avoided accordingly,
can't they?
Regards,
Markus
Le 16/04/2024 à 14:14, Markus Elfring a écrit :
>> This is explicit in Kernel documentation:
>>
>> /**
>> * kfree - free previously allocated memory
>> * @object: pointer returned by kmalloc() or kmem_cache_alloc()
>> *
>> * If @object is NULL, no operation is performed.
>> */
>>
>> That's exactly the same behaviour as free() in libc.
>>
>> So Coccinelle should be fixed if it reports an error for that.
>
> Redundant function calls can occasionally be avoided accordingly,
> can't they?
Sure they can, but is that worth it here ?
Christophe
On Tue, 16 Apr 2024, Christophe Leroy wrote:
>
>
> Le 16/04/2024 à 14:14, Markus Elfring a écrit :
> >> This is explicit in Kernel documentation:
> >>
> >> /**
> >> * kfree - free previously allocated memory
> >> * @object: pointer returned by kmalloc() or kmem_cache_alloc()
> >> *
> >> * If @object is NULL, no operation is performed.
> >> */
> >>
> >> That's exactly the same behaviour as free() in libc.
> >>
> >> So Coccinelle should be fixed if it reports an error for that.
> >
> > Redundant function calls can occasionally be avoided accordingly,
> > can't they?
>
> Sure they can, but is that worth it here ?
Coccinelle does what the developer of the semantic patch tells it to do.
It doesn't spontaneously report errors for anything.
julia
>>>> So Coccinelle should be fixed if it reports an error for that.
>>>
>>> Redundant function calls can occasionally be avoided accordingly,
>>> can't they?
>>
>> Sure they can, but is that worth it here ?
>
> Coccinelle does what the developer of the semantic patch tells it to do.
> It doesn't spontaneously report errors for anything.
Some special source code search and transformation patterns are occasionally applied.
The corresponding change acceptance can often be challenging.
Regards,
Markus
> Looking at that function, I however see a missing region release when
> ioremap_cache() fails.
Under which circumstances will the exception handling be completed also for
the implementation of the function “map_paste_region”?
https://elixir.bootlin.com/linux/v6.9-rc4/source/arch/powerpc/platforms/powernv/vas-window.c#L67
Regards,
Markus