x86/hyperv: Use pointer from memcpy() call for assignment in hv_crash_setup_trampdata()

[PATCH] x86/hyperv: Use pointer from memcpy() call for assignment in hv_crash_setup_trampdata()

Posted by Markus Elfring 3 months, 1 week ago

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 31 Oct 2025 09:24:31 +0100

A pointer was assigned to a variable. The same pointer was used for
the destination parameter of a memcpy() call.
This function is documented in the way that the same value is returned.
Thus convert two separate statements into a direct variable assignment for
the return value from a memory copy action.

The source code was transformed by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/x86/hyperv/hv_crash.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/x86/hyperv/hv_crash.c b/arch/x86/hyperv/hv_crash.c
index c0e22921ace1..745d02066308 100644
--- a/arch/x86/hyperv/hv_crash.c
+++ b/arch/x86/hyperv/hv_crash.c
@@ -464,9 +464,7 @@ static int hv_crash_setup_trampdata(u64 trampoline_va)
 		return -1;
 	}

-	dest = (void *)trampoline_va;
-	memcpy(dest, &hv_crash_asm32, size);
-
+	dest = memcpy((void *)trampoline_va, &hv_crash_asm32, size);
 	dest += size;
 	dest = (void *)round_up((ulong)dest, 16);
 	tramp = (struct hv_crash_tramp_data *)dest;
-- 
2.51.1

Re: [PATCH] x86/hyperv: Use pointer from memcpy() call for assignment in hv_crash_setup_trampdata()

Posted by Wei Liu 3 months, 1 week ago

On Fri, Oct 31, 2025 at 09:33:17AM +0100, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 31 Oct 2025 09:24:31 +0100
> 
> A pointer was assigned to a variable. The same pointer was used for
> the destination parameter of a memcpy() call.
> This function is documented in the way that the same value is returned.
> Thus convert two separate statements into a direct variable assignment for
> the return value from a memory copy action.
> 
> The source code was transformed by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  arch/x86/hyperv/hv_crash.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/arch/x86/hyperv/hv_crash.c b/arch/x86/hyperv/hv_crash.c
> index c0e22921ace1..745d02066308 100644
> --- a/arch/x86/hyperv/hv_crash.c
> +++ b/arch/x86/hyperv/hv_crash.c
> @@ -464,9 +464,7 @@ static int hv_crash_setup_trampdata(u64 trampoline_va)
>  		return -1;
>  	}
>  
> -	dest = (void *)trampoline_va;
> -	memcpy(dest, &hv_crash_asm32, size);
> -
> +	dest = memcpy((void *)trampoline_va, &hv_crash_asm32, size);

I don't think this change is needed.

There aren't that many places in the kernel tree that use this pattern.
The pattern used by the original code is far more pervasive.

Wei

>  	dest += size;
>  	dest = (void *)round_up((ulong)dest, 16);
>  	tramp = (struct hv_crash_tramp_data *)dest;
> -- 
> 2.51.1
>

Re: [PATCH] x86/hyperv: Use pointer from memcpy() call for assignment in hv_crash_setup_trampdata()

Posted by Naman Jain 3 months, 1 week ago


On 10/31/2025 2:03 PM, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 31 Oct 2025 09:24:31 +0100
> 
> A pointer was assigned to a variable. The same pointer was used for
> the destination parameter of a memcpy() call.
> This function is documented in the way that the same value is returned.
> Thus convert two separate statements into a direct variable assignment for
> the return value from a memory copy action.
> 
> The source code was transformed by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>   arch/x86/hyperv/hv_crash.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/arch/x86/hyperv/hv_crash.c b/arch/x86/hyperv/hv_crash.c
> index c0e22921ace1..745d02066308 100644
> --- a/arch/x86/hyperv/hv_crash.c
> +++ b/arch/x86/hyperv/hv_crash.c
> @@ -464,9 +464,7 @@ static int hv_crash_setup_trampdata(u64 trampoline_va)
>   		return -1;
>   	}
>   
> -	dest = (void *)trampoline_va;
> -	memcpy(dest, &hv_crash_asm32, size);
> -
> +	dest = memcpy((void *)trampoline_va, &hv_crash_asm32, size);
>   	dest += size;
>   	dest = (void *)round_up((ulong)dest, 16);
>   	tramp = (struct hv_crash_tramp_data *)dest;


I tried running spatch Coccinelle checks on this file, but could not get 
it to flag this improvement. Do you mind sharing more details on the 
issue reproduction please.

I am OK with this change, though it may cost code readability a little 
bit. But if this is a result of some known standard rule, added as part 
of these Coccinelle rules, we should be good.

Regards,
Naman

Re: [PATCH] x86/hyperv: Use pointer from memcpy() call for assignment in hv_crash_setup_trampdata()

Posted by Markus Elfring 3 months, 1 week ago

…>> +++ b/arch/x86/hyperv/hv_crash.c
>> @@ -464,9 +464,7 @@ static int hv_crash_setup_trampdata(u64 trampoline_va)
>>           return -1;
>>       }
>>   -    dest = (void *)trampoline_va;
>> -    memcpy(dest, &hv_crash_asm32, size);
>> -
>> +    dest = memcpy((void *)trampoline_va, &hv_crash_asm32, size);
>>       dest += size;
>>       dest = (void *)round_up((ulong)dest, 16);
>>       tramp = (struct hv_crash_tramp_data *)dest;
> 
> 
> I tried running spatch Coccinelle checks on this file, but could not get it to flag this improvement.

The proposed source code transformation is not supported by a coccicheck script so far.


> Do you mind sharing more details on the issue reproduction please.

Would you like to take another look at corresponding development discussions?

Example:
[RFC] Increasing usage of direct pointer assignments from memcpy() calls with SmPL?
https://lore.kernel.org/cocci/ddc8654a-9505-451f-87ad-075bfa646381@web.de/
https://sympa.inria.fr/sympa/arc/cocci/2025-10/msg00049.html


> I am OK with this change,

Thanks for a bit of positive feedback.


> though it may cost code readability a little bit.

Would you complain about other variable assignments in such a direction?

Regards,
Markus

Re: [PATCH] x86/hyperv: Use pointer from memcpy() call for assignment in hv_crash_setup_trampdata()

Posted by Naman Jain 3 months, 1 week ago


On 10/31/2025 3:14 PM, Markus Elfring wrote:
> …>> +++ b/arch/x86/hyperv/hv_crash.c
>>> @@ -464,9 +464,7 @@ static int hv_crash_setup_trampdata(u64 trampoline_va)
>>>            return -1;
>>>        }
>>>    -    dest = (void *)trampoline_va;
>>> -    memcpy(dest, &hv_crash_asm32, size);
>>> -
>>> +    dest = memcpy((void *)trampoline_va, &hv_crash_asm32, size);
>>>        dest += size;
>>>        dest = (void *)round_up((ulong)dest, 16);
>>>        tramp = (struct hv_crash_tramp_data *)dest;
>>
>>
>> I tried running spatch Coccinelle checks on this file, but could not get it to flag this improvement.
> 
> The proposed source code transformation is not supported by a coccicheck script so far.
> 
> 
>> Do you mind sharing more details on the issue reproduction please.
> 
> Would you like to take another look at corresponding development discussions?
> 
> Example:
> [RFC] Increasing usage of direct pointer assignments from memcpy() calls with SmPL?
> https://lore.kernel.org/cocci/ddc8654a-9505-451f-87ad-075bfa646381@web.de/
> https://sympa.inria.fr/sympa/arc/cocci/2025-10/msg00049.html
> 
> 
>> I am OK with this change,
> 
> Thanks for a bit of positive feedback.
> 
> 
>> though it may cost code readability a little bit.
> 
> Would you complain about other variable assignments in such a direction?
> 
> Regards,
> Markus

The only thing which concerns readability IMO is that it is based on the 
assumption that the person reading the code is aware of the return value 
of memcpy. Now, it is debatable if that is something which can be 
considered trivial. I don't have a strong opinion there, but would 
prefer it in its current form.

Also, we could have optimized it further by writing it as below, but we 
are not doing that for a reason as we want to keep the code simpler to 
read and understand. The same may apply here as well.

dest = memcpy((void *)trampoline_va, &hv_crash_asm32, size) + size;

Regards,
Naman

Re: [PATCH] x86/hyperv: Use pointer from memcpy() call for assignment in hv_crash_setup_trampdata()

Posted by Julia Lawall 3 months, 1 week ago


On Fri, 31 Oct 2025, Naman Jain wrote:

>
>
> On 10/31/2025 2:03 PM, Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Fri, 31 Oct 2025 09:24:31 +0100
> >
> > A pointer was assigned to a variable. The same pointer was used for
> > the destination parameter of a memcpy() call.
> > This function is documented in the way that the same value is returned.
> > Thus convert two separate statements into a direct variable assignment for
> > the return value from a memory copy action.
> >
> > The source code was transformed by using the Coccinelle software.
> >
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
> >   arch/x86/hyperv/hv_crash.c | 4 +---
> >   1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/hyperv/hv_crash.c b/arch/x86/hyperv/hv_crash.c
> > index c0e22921ace1..745d02066308 100644
> > --- a/arch/x86/hyperv/hv_crash.c
> > +++ b/arch/x86/hyperv/hv_crash.c
> > @@ -464,9 +464,7 @@ static int hv_crash_setup_trampdata(u64 trampoline_va)
> >   		return -1;
> >   	}
> >   -	dest = (void *)trampoline_va;
> > -	memcpy(dest, &hv_crash_asm32, size);
> > -
> > +	dest = memcpy((void *)trampoline_va, &hv_crash_asm32, size);
> >   	dest += size;
> >   	dest = (void *)round_up((ulong)dest, 16);
> >   	tramp = (struct hv_crash_tramp_data *)dest;
>
>
> I tried running spatch Coccinelle checks on this file, but could not get it to
> flag this improvement. Do you mind sharing more details on the issue
> reproduction please.
>
> I am OK with this change, though it may cost code readability a little bit.
> But if this is a result of some known standard rule, added as part of these
> Coccinelle rules, we should be good.

Multiple people have suggested that due to the loos of readability the
change is not a good idea.

It's not done by a standard rule.

julia