[PATCH 0/2] powerpc/powernv/vas: Adjustments for two function implementations

[PATCH 0/2] powerpc/powernv/vas: Adjustments for two function implementations

[Markus Elfring]

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

[PATCH 1/2] powerpc/powernv/vas: One function call less in vas_window_alloc() after error detection

[Markus Elfring]

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

[PATCH 2/2] powerpc/powernv/vas: Return directly after a failed kasprintf() in map_paste_region()

[Markus Elfring]

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

Re: [0/2] powerpc/powernv/vas: Adjustments for two function implementations

[Markus Elfring]

> 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

Re: [0/2] powerpc/powernv/vas: Adjustments for two function implementations

[Michael Ellerman]

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

Re: [0/2] powerpc/powernv/vas: Adjustments for two function implementations

[Christophe Leroy]

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

Re: [0/2] powerpc/powernv/vas: Adjustments for two function implementations

[Markus Elfring]

> 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

Re: [0/2] powerpc/powernv/vas: Adjustments for two function implementations

[Christophe Leroy]

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

Re: [0/2] powerpc/powernv/vas: Adjustments for two function implementations

[Julia Lawall]

[-- 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

Re: [0/2] powerpc/powernv/vas: Adjustments for two function implementations

[Markus Elfring]

>>>> 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

Re: [0/2] powerpc/powernv/vas: Adjustments for map_paste_region()?

[Markus Elfring]

> 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

Re: [0/2] powerpc/powernv/vas: Adjustments for two function implementations

[Markus Elfring]

>>>   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