* [PATCH 0/2] powerpc/powernv/vas: Adjustments for two function implementations
@ 2023-12-23 19:15 Markus Elfring
2023-12-23 19:20 ` [PATCH 1/2] powerpc/powernv/vas: One function call less in vas_window_alloc() after error detection Markus Elfring
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Markus Elfring @ 2023-12-23 19:15 UTC (permalink / raw)
To: linuxppc-dev, kernel-janitors, Aneesh Kumar K.V,
Christophe Leroy, Michael Ellerman, Naveen N. Rao,
Nicholas Piggin
Cc: LKML, cocci
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/2] powerpc/powernv/vas: One function call less in vas_window_alloc() after error detection
2023-12-23 19:15 [PATCH 0/2] powerpc/powernv/vas: Adjustments for two function implementations Markus Elfring
@ 2023-12-23 19:20 ` Markus Elfring
2023-12-23 19:22 ` [PATCH 2/2] powerpc/powernv/vas: Return directly after a failed kasprintf() in map_paste_region() Markus Elfring
2024-04-15 7:42 ` [0/2] powerpc/powernv/vas: Adjustments for two function implementations Markus Elfring
2 siblings, 0 replies; 12+ messages in thread
From: Markus Elfring @ 2023-12-23 19:20 UTC (permalink / raw)
To: linuxppc-dev, kernel-janitors, Aneesh Kumar K.V,
Christophe Leroy, Michael Ellerman, Naveen N. Rao,
Nicholas Piggin
Cc: LKML, cocci
From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/2] powerpc/powernv/vas: Return directly after a failed kasprintf() in map_paste_region()
2023-12-23 19:15 [PATCH 0/2] powerpc/powernv/vas: Adjustments for two function implementations Markus Elfring
2023-12-23 19:20 ` [PATCH 1/2] powerpc/powernv/vas: One function call less in vas_window_alloc() after error detection Markus Elfring
@ 2023-12-23 19:22 ` Markus Elfring
2024-04-15 7:42 ` [0/2] powerpc/powernv/vas: Adjustments for two function implementations Markus Elfring
2 siblings, 0 replies; 12+ messages in thread
From: Markus Elfring @ 2023-12-23 19:22 UTC (permalink / raw)
To: linuxppc-dev, kernel-janitors, Aneesh Kumar K.V,
Christophe Leroy, Michael Ellerman, Naveen N. Rao,
Nicholas Piggin
Cc: LKML, cocci
From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [0/2] powerpc/powernv/vas: Adjustments for two function implementations
2023-12-23 19:15 [PATCH 0/2] powerpc/powernv/vas: Adjustments for two function implementations Markus Elfring
2023-12-23 19:20 ` [PATCH 1/2] powerpc/powernv/vas: One function call less in vas_window_alloc() after error detection Markus Elfring
2023-12-23 19:22 ` [PATCH 2/2] powerpc/powernv/vas: Return directly after a failed kasprintf() in map_paste_region() Markus Elfring
@ 2024-04-15 7:42 ` Markus Elfring
2024-04-16 11:11 ` Michael Ellerman
2 siblings, 1 reply; 12+ messages in thread
From: Markus Elfring @ 2024-04-15 7:42 UTC (permalink / raw)
To: linuxppc-dev, kernel-janitors, Aneesh Kumar K.V,
Christophe Leroy, Michael Ellerman, Naveen N. Rao,
Nicholas Piggin
Cc: LKML, cocci
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [0/2] powerpc/powernv/vas: Adjustments for two function implementations
2024-04-15 7:42 ` [0/2] powerpc/powernv/vas: Adjustments for two function implementations Markus Elfring
@ 2024-04-16 11:11 ` Michael Ellerman
2024-04-16 11:32 ` Christophe Leroy
2024-04-16 12:04 ` [0/2] powerpc/powernv/vas: Adjustments for two function implementations Markus Elfring
0 siblings, 2 replies; 12+ messages in thread
From: Michael Ellerman @ 2024-04-16 11:11 UTC (permalink / raw)
To: Markus Elfring, linuxppc-dev, kernel-janitors, Aneesh Kumar K.V,
Christophe Leroy, Naveen N. Rao, Nicholas Piggin
Cc: LKML, cocci
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [0/2] powerpc/powernv/vas: Adjustments for two function implementations
2024-04-16 11:11 ` Michael Ellerman
@ 2024-04-16 11:32 ` Christophe Leroy
2024-04-16 12:14 ` Markus Elfring
2024-04-16 14:05 ` [0/2] powerpc/powernv/vas: Adjustments for map_paste_region()? Markus Elfring
2024-04-16 12:04 ` [0/2] powerpc/powernv/vas: Adjustments for two function implementations Markus Elfring
1 sibling, 2 replies; 12+ messages in thread
From: Christophe Leroy @ 2024-04-16 11:32 UTC (permalink / raw)
To: Michael Ellerman, Markus Elfring, linuxppc-dev, kernel-janitors,
Aneesh Kumar K.V, Naveen N. Rao, Nicholas Piggin
Cc: LKML, cocci
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [0/2] powerpc/powernv/vas: Adjustments for two function implementations
2024-04-16 11:11 ` Michael Ellerman
2024-04-16 11:32 ` Christophe Leroy
@ 2024-04-16 12:04 ` Markus Elfring
1 sibling, 0 replies; 12+ messages in thread
From: Markus Elfring @ 2024-04-16 12:04 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev, kernel-janitors,
Aneesh Kumar K.V, Christophe Leroy, Naveen N. Rao,
Nicholas Piggin
Cc: LKML, cocci
>>> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [0/2] powerpc/powernv/vas: Adjustments for two function implementations
2024-04-16 11:32 ` Christophe Leroy
@ 2024-04-16 12:14 ` Markus Elfring
2024-04-16 12:18 ` Christophe Leroy
2024-04-16 14:05 ` [0/2] powerpc/powernv/vas: Adjustments for map_paste_region()? Markus Elfring
1 sibling, 1 reply; 12+ messages in thread
From: Markus Elfring @ 2024-04-16 12:14 UTC (permalink / raw)
To: Christophe Leroy, Michael Ellerman, linuxppc-dev,
kernel-janitors, Aneesh Kumar K.V, Naveen N. Rao,
Nicholas Piggin
Cc: LKML, cocci
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [0/2] powerpc/powernv/vas: Adjustments for two function implementations
2024-04-16 12:14 ` Markus Elfring
@ 2024-04-16 12:18 ` Christophe Leroy
2024-04-16 12:56 ` Julia Lawall
0 siblings, 1 reply; 12+ messages in thread
From: Christophe Leroy @ 2024-04-16 12:18 UTC (permalink / raw)
To: Markus Elfring, Michael Ellerman, linuxppc-dev, kernel-janitors,
Aneesh Kumar K.V, Naveen N. Rao, Nicholas Piggin
Cc: LKML, cocci
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [0/2] powerpc/powernv/vas: Adjustments for two function implementations
2024-04-16 12:18 ` Christophe Leroy
@ 2024-04-16 12:56 ` Julia Lawall
2024-04-16 13:05 ` Markus Elfring
0 siblings, 1 reply; 12+ messages in thread
From: Julia Lawall @ 2024-04-16 12:56 UTC (permalink / raw)
To: Christophe Leroy
Cc: kernel-janitors, LKML, Nicholas Piggin, Aneesh Kumar K.V,
Markus Elfring, Naveen N. Rao, linuxppc-dev, cocci
[-- Attachment #1: Type: text/plain, Size: 772 bytes --]
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [0/2] powerpc/powernv/vas: Adjustments for two function implementations
2024-04-16 12:56 ` Julia Lawall
@ 2024-04-16 13:05 ` Markus Elfring
0 siblings, 0 replies; 12+ messages in thread
From: Markus Elfring @ 2024-04-16 13:05 UTC (permalink / raw)
To: Julia Lawall, Christophe Leroy, Michael Ellerman, linuxppc-dev,
kernel-janitors, Aneesh Kumar K.V, Naveen N. Rao,
Nicholas Piggin
Cc: LKML, cocci
>>>> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [0/2] powerpc/powernv/vas: Adjustments for map_paste_region()?
2024-04-16 11:32 ` Christophe Leroy
2024-04-16 12:14 ` Markus Elfring
@ 2024-04-16 14:05 ` Markus Elfring
1 sibling, 0 replies; 12+ messages in thread
From: Markus Elfring @ 2024-04-16 14:05 UTC (permalink / raw)
To: Christophe Leroy, Michael Ellerman, linuxppc-dev,
kernel-janitors, Aneesh Kumar K.V, Naveen N. Rao,
Nicholas Piggin
Cc: LKML, cocci
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2024-04-16 14:06 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-23 19:15 [PATCH 0/2] powerpc/powernv/vas: Adjustments for two function implementations Markus Elfring
2023-12-23 19:20 ` [PATCH 1/2] powerpc/powernv/vas: One function call less in vas_window_alloc() after error detection Markus Elfring
2023-12-23 19:22 ` [PATCH 2/2] powerpc/powernv/vas: Return directly after a failed kasprintf() in map_paste_region() Markus Elfring
2024-04-15 7:42 ` [0/2] powerpc/powernv/vas: Adjustments for two function implementations Markus Elfring
2024-04-16 11:11 ` Michael Ellerman
2024-04-16 11:32 ` Christophe Leroy
2024-04-16 12:14 ` Markus Elfring
2024-04-16 12:18 ` Christophe Leroy
2024-04-16 12:56 ` Julia Lawall
2024-04-16 13:05 ` Markus Elfring
2024-04-16 14:05 ` [0/2] powerpc/powernv/vas: Adjustments for map_paste_region()? Markus Elfring
2024-04-16 12:04 ` [0/2] powerpc/powernv/vas: Adjustments for two function implementations Markus Elfring
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).