arch/powerpc/platforms/powernv/vas-window.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
From: Markus Elfring <elfring@users.sourceforge.net> 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
> 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/1f1c21cf-c34c-418c-b00c-8e6474f12612@web.de/ > Return directly after a failed kasprintf() in map_paste_region() https://patchwork.ozlabs.org/project/linuxppc-dev/patch/f46f04bc-613c-4e98-b602-4c5120556b09@web.de/ > arch/powerpc/platforms/powernv/vas-window.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) Regards, Markus
Markus Elfring <Markus.Elfring@web.de> 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/1f1c21cf-c34c-418c-b00c-8e6474f12612@web.de/ 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/f46f04bc-613c-4e98-b602-4c5120556b09@web.de/ Basically the same reasoning. And it also changes the function from having two return paths (success and error), to three. cheers
>>> One function call less in vas_window_alloc() after error detection >> >> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/1f1c21cf-c34c-418c-b00c-8e6474f12612@web.de/ > > 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/f46f04bc-613c-4e98-b602-4c5120556b09@web.de/ > > 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
Le 16/04/2024 à 13:11, Michael Ellerman a écrit : > Markus Elfring <Markus.Elfring@web.de> 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/1f1c21cf-c34c-418c-b00c-8e6474f12612@web.de/ > > 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/f46f04bc-613c-4e98-b602-4c5120556b09@web.de/ > > 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
> 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
> 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
© 2016 - 2025 Red Hat, Inc.