[PATCH] platform/chrome: cros_typec_vdm: Fix VDO copy

[PATCH] platform/chrome: cros_typec_vdm: Fix VDO copy

[Prashant Malani]

The usage of memcpy() affects the representation of the VDOs as they are
copied to the EC Host Command buffer. Specifically, all higher order
bits get dropped (for example: a VDO of 0x406 just gets copied as 0x6).

Avoid this by explicitly copying each VDO in the array. The number of
VDOs generated by alternate mode drivers in their VDMs is almost always
just 1 (apart from the header) so this doesn't affect performance in a
meaningful way).

Fixes: 40a9b13a09ef ("platform/chrome: cros_typec_vdm: Add VDM send support")
Signed-off-by: Prashant Malani <pmalani@chromium.org>
---
 drivers/platform/chrome/cros_typec_vdm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/chrome/cros_typec_vdm.c b/drivers/platform/chrome/cros_typec_vdm.c
index aca9d337118e..06f4a55999c5 100644
--- a/drivers/platform/chrome/cros_typec_vdm.c
+++ b/drivers/platform/chrome/cros_typec_vdm.c
@@ -86,10 +86,12 @@ static int cros_typec_port_amode_vdm(struct typec_altmode *amode, const u32 hdr,
 		.command = TYPEC_CONTROL_COMMAND_SEND_VDM_REQ,
 	};
 	struct typec_vdm_req vdm_req = {};
+	int i;
 
 	vdm_req.vdm_data[0] = hdr;
 	vdm_req.vdm_data_objects = cnt;
-	memcpy(&vdm_req.vdm_data[1], vdo, cnt - 1);
+	for (i = 1; i < cnt; i++)
+		vdm_req.vdm_data[i] = vdo[i-1];
 	vdm_req.partner_type = TYPEC_PARTNER_SOP;
 	req.vdm_req_params = vdm_req;
 
-- 
2.39.0.314.g84b9a713c41-goog

Re: [PATCH] platform/chrome: cros_typec_vdm: Fix VDO copy

[Benson Leung]

[-- Attachment #1: Type: text/plain, Size: 1790 bytes --]

Hi Prashant,


On Fri, Jan 13, 2023 at 06:26:26PM +0000, Prashant Malani wrote:
> The usage of memcpy() affects the representation of the VDOs as they are
> copied to the EC Host Command buffer. Specifically, all higher order
> bits get dropped (for example: a VDO of 0x406 just gets copied as 0x6).
> 
> Avoid this by explicitly copying each VDO in the array. The number of
> VDOs generated by alternate mode drivers in their VDMs is almost always
> just 1 (apart from the header) so this doesn't affect performance in a
> meaningful way).
> 
> Fixes: 40a9b13a09ef ("platform/chrome: cros_typec_vdm: Add VDM send support")
> Signed-off-by: Prashant Malani <pmalani@chromium.org>

Reviewed-by: Benson Leung <bleung@chromium.org>


> ---
>  drivers/platform/chrome/cros_typec_vdm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/chrome/cros_typec_vdm.c b/drivers/platform/chrome/cros_typec_vdm.c
> index aca9d337118e..06f4a55999c5 100644
> --- a/drivers/platform/chrome/cros_typec_vdm.c
> +++ b/drivers/platform/chrome/cros_typec_vdm.c
> @@ -86,10 +86,12 @@ static int cros_typec_port_amode_vdm(struct typec_altmode *amode, const u32 hdr,
>  		.command = TYPEC_CONTROL_COMMAND_SEND_VDM_REQ,
>  	};
>  	struct typec_vdm_req vdm_req = {};
> +	int i;
>  
>  	vdm_req.vdm_data[0] = hdr;
>  	vdm_req.vdm_data_objects = cnt;
> -	memcpy(&vdm_req.vdm_data[1], vdo, cnt - 1);
> +	for (i = 1; i < cnt; i++)
> +		vdm_req.vdm_data[i] = vdo[i-1];
>  	vdm_req.partner_type = TYPEC_PARTNER_SOP;
>  	req.vdm_req_params = vdm_req;
>  
> -- 
> 2.39.0.314.g84b9a713c41-goog
> 
> 

-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@google.com
Chromium OS Project
bleung@chromium.org

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] platform/chrome: cros_typec_vdm: Fix VDO copy

[patchwork-bot+chrome-platform]

Hello:

This patch was applied to chrome-platform/linux.git (for-kernelci)
by Prashant Malani <pmalani@chromium.org>:

On Fri, 13 Jan 2023 18:26:26 +0000 you wrote:
> The usage of memcpy() affects the representation of the VDOs as they are
> copied to the EC Host Command buffer. Specifically, all higher order
> bits get dropped (for example: a VDO of 0x406 just gets copied as 0x6).
> 
> Avoid this by explicitly copying each VDO in the array. The number of
> VDOs generated by alternate mode drivers in their VDMs is almost always
> just 1 (apart from the header) so this doesn't affect performance in a
> meaningful way).
> 
> [...]

Here is the summary with links:
  - platform/chrome: cros_typec_vdm: Fix VDO copy
    https://git.kernel.org/chrome-platform/c/478f32ab4daa

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH] platform/chrome: cros_typec_vdm: Fix VDO copy

[patchwork-bot+chrome-platform]

Hello:

This patch was applied to chrome-platform/linux.git (for-next)
by Prashant Malani <pmalani@chromium.org>:

On Fri, 13 Jan 2023 18:26:26 +0000 you wrote:
> The usage of memcpy() affects the representation of the VDOs as they are
> copied to the EC Host Command buffer. Specifically, all higher order
> bits get dropped (for example: a VDO of 0x406 just gets copied as 0x6).
> 
> Avoid this by explicitly copying each VDO in the array. The number of
> VDOs generated by alternate mode drivers in their VDMs is almost always
> just 1 (apart from the header) so this doesn't affect performance in a
> meaningful way).
> 
> [...]

Here is the summary with links:
  - platform/chrome: cros_typec_vdm: Fix VDO copy
    https://git.kernel.org/chrome-platform/c/478f32ab4daa

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH] platform/chrome: cros_typec_vdm: Fix VDO copy

[Tzung-Bi Shih]

On Fri, Jan 13, 2023 at 06:26:26PM +0000, Prashant Malani wrote:
> The usage of memcpy() affects the representation of the VDOs as they are
> copied to the EC Host Command buffer. Specifically, all higher order
> bits get dropped (for example: a VDO of 0x406 just gets copied as 0x6).

memcpy() is byte-oriented; however, `vdo` is a pointer to u32.

> Avoid this by explicitly copying each VDO in the array. The number of
> VDOs generated by alternate mode drivers in their VDMs is almost always
> just 1 (apart from the header) so this doesn't affect performance in a
> meaningful way).

Although the patch has applied, I am wondering if the following would be a
better way to fix the issue:

memcpy(&vdm_req.vdm_data[1], vdo, (cnt - 1) * sizeof(*vdo));

Re: [PATCH] platform/chrome: cros_typec_vdm: Fix VDO copy

[Prashant Malani]

Hi Tzung-Bi,

On Mon, Jan 30, 2023 at 7:49 PM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> On Fri, Jan 13, 2023 at 06:26:26PM +0000, Prashant Malani wrote:
> > The usage of memcpy() affects the representation of the VDOs as they are
> > copied to the EC Host Command buffer. Specifically, all higher order
> > bits get dropped (for example: a VDO of 0x406 just gets copied as 0x6).
>
> memcpy() is byte-oriented; however, `vdo` is a pointer to u32.
>
> > Avoid this by explicitly copying each VDO in the array. The number of
> > VDOs generated by alternate mode drivers in their VDMs is almost always
> > just 1 (apart from the header) so this doesn't affect performance in a
> > meaningful way).
>
> Although the patch has applied, I am wondering if the following would be a
> better way to fix the issue:
>
> memcpy(&vdm_req.vdm_data[1], vdo, (cnt - 1) * sizeof(*vdo));

Yeah, that's right; I forgot that 'cnt' is "number of VDOs" and not
"number of bytes" :S

As I mentioned, in a vast majority of cases, the number of VDOs is just 1, so
I wouldn't bother and just leave this as it is (since there is no major benefit
apart from saving 1 line).