[PATCH] libnvme: do not byte swap command dwords

[PATCH] libnvme: do not byte swap command dwords

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The dwords always need to be set in the cpu native format. The driver
will handle setting up the SQE in the nvme little-endian order, and big
endian is never used, so this byte swapping was wrong.

Link: https://github.com/linux-nvme/nvme-cli/issues/2761
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 src/nvme/ioctl.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/nvme/ioctl.c b/src/nvme/ioctl.c
index 18b228d9..86c2b702 100644
--- a/src/nvme/ioctl.c
+++ b/src/nvme/ioctl.c
@@ -1920,9 +1920,9 @@ static int nvme_set_var_size_tags(__u32 *cmd_dw2, __u32 *cmd_dw3, __u32 *cmd_dw1
 		return -1;
 	}
 
-	*cmd_dw2 = cpu_to_be32(cdw2);
-	*cmd_dw3 = cpu_to_be32(cdw3);
-	*cmd_dw14 = cpu_to_be32(cdw14);
+	*cmd_dw2 = cdw2;
+	*cmd_dw3 = cdw3;
+	*cmd_dw14 = cdw14;
 	return 0;
 }
 
-- 
2.47.1

Re: [PATCH] libnvme: do not byte swap command dwords

[Daniel Wagner]

On Thu, Apr 03, 2025 at 07:43:15AM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The dwords always need to be set in the cpu native format. The driver
> will handle setting up the SQE in the nvme little-endian order, and big
> endian is never used, so this byte swapping was wrong.
> 
> Link: https://github.com/linux-nvme/nvme-cli/issues/2761
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  src/nvme/ioctl.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/nvme/ioctl.c b/src/nvme/ioctl.c
> index 18b228d9..86c2b702 100644
> --- a/src/nvme/ioctl.c
> +++ b/src/nvme/ioctl.c
> @@ -1920,9 +1920,9 @@ static int nvme_set_var_size_tags(__u32 *cmd_dw2, __u32 *cmd_dw3, __u32 *cmd_dw1
>  		return -1;
>  	}
>  
> -	*cmd_dw2 = cpu_to_be32(cdw2);
> -	*cmd_dw3 = cpu_to_be32(cdw3);
> -	*cmd_dw14 = cpu_to_be32(cdw14);
> +	*cmd_dw2 = cdw2;
> +	*cmd_dw3 = cdw3;
> +	*cmd_dw14 = cdw14;

in this case shouldn't we use cpu_to_le32?

Re: [PATCH] libnvme: do not byte swap command dwords

[Keith Busch]

On Thu, Apr 03, 2025 at 07:25:13PM +0200, Daniel Wagner wrote:
> On Thu, Apr 03, 2025 at 07:43:15AM -0700, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> > 
> > The dwords always need to be set in the cpu native format. The driver
> > will handle setting up the SQE in the nvme little-endian order, and big
> > endian is never used, so this byte swapping was wrong.
> > 
> > Link: https://github.com/linux-nvme/nvme-cli/issues/2761
> > Signed-off-by: Keith Busch <kbusch@kernel.org>
> > ---
> >  src/nvme/ioctl.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/nvme/ioctl.c b/src/nvme/ioctl.c
> > index 18b228d9..86c2b702 100644
> > --- a/src/nvme/ioctl.c
> > +++ b/src/nvme/ioctl.c
> > @@ -1920,9 +1920,9 @@ static int nvme_set_var_size_tags(__u32 *cmd_dw2, __u32 *cmd_dw3, __u32 *cmd_dw1
> >  		return -1;
> >  	}
> >  
> > -	*cmd_dw2 = cpu_to_be32(cdw2);
> > -	*cmd_dw3 = cpu_to_be32(cdw3);
> > -	*cmd_dw14 = cpu_to_be32(cdw14);
> > +	*cmd_dw2 = cdw2;
> > +	*cmd_dw3 = cdw3;
> > +	*cmd_dw14 = cdw14;
> 
> in this case shouldn't we use cpu_to_le32?

No, user space was only uses cpu native endian when constructing the
command. The driver handles the endian conversion if necessary.

That conversion is just for the command control side. The driver doesn't
do any such manipulation on the data plane, so endianness in payloads
for commands like "copy", "dsm", or PI meatada need to be handled by the
application.

Re: [PATCH] libnvme: do not byte swap command dwords

[Daniel Wagner]

On Thu, Apr 03, 2025 at 07:25:13PM +0200, Daniel Wagner wrote:
> in this case shouldn't we use cpu_to_le32?

Nevermind, just saw your explanation on github. the driver takes care of it...

Re: [PATCH] libnvme: do not byte swap command dwords

[Daniel Wagner]

On Thu, 03 Apr 2025 07:43:15 -0700, Keith Busch wrote:
> The dwords always need to be set in the cpu native format. The driver
> will handle setting up the SQE in the nvme little-endian order, and big
> endian is never used, so this byte swapping was wrong.
> 
> 

Applied, thanks!

[1/1] libnvme: do not byte swap command dwords
      commit: 89ac31f536baf15c1834f877b4c1fb9aca1cf94f

Best regards,
-- 
Daniel Wagner <wagi@kernel.org>