diff mbox series

gve: mixes DPDK and Linux versions in compatibility check

Message ID 20240111120841.136b1e7c@hermes.local (mailing list archive)
State Not Applicable, archived
Delegated to: Ferruh Yigit
Headers
Series gve: mixes DPDK and Linux versions in compatibility check |

Checks

Context Check Description
ci/loongarch-compilation fail ninja build failure
ci/iol-testing fail build patch failure
ci/Intel-compilation fail Compilation issues
ci/github-robot: build fail github build: failed

Commit Message

Stephen Hemminger Jan. 11, 2024, 8:08 p.m. UTC 
  This seems wrong:
	*driver_info = (struct gve_driver_info) {
		.os_type = 5, /* DPDK */
		.driver_major = GVE_VERSION_MAJOR,
		.driver_minor = GVE_VERSION_MINOR,
		.driver_sub = GVE_VERSION_SUB,
		.os_version_major = cpu_to_be32(DPDK_VERSION_MAJOR),
		.os_version_minor = cpu_to_be32(DPDK_VERSION_MINOR),
		.os_version_sub = cpu_to_be32(DPDK_VERSION_SUB),
		.driver_capability_flags = {
			cpu_to_be64(GVE_DRIVER_CAPABILITY_FLAGS1),
			cpu_to_be64(GVE_DRIVER_CAPABILITY_FLAGS2),
			cpu_to_be64(GVE_DRIVER_CAPABILITY_FLAGS3),
			cpu_to_be64(GVE_DRIVER_CAPABILITY_FLAGS4),
		},
	};

	populate_driver_version_strings((char *)driver_info->os_version_str1,
			(char *)driver_info->os_version_str2);

The numeric values os_version_major, os_version_minor use DPDK version which
is good.  But the populate_driver_version_strings gets the Linux kernel
version number which is problematic.  Looks like a bug to me.

Also technically, the Linux kernel version is not the same as the OS release.

Shouldn't it be more like:

Comments

Rushil Gupta Jan. 20, 2024, 7:08 a.m. UTC | #1 
Hi Stephen
We wish to capture both rte version and linux kernel version.

The current implementation is needed to answer questions like how many
customers are on Dpdk 23.11 for Ubuntu 22 vs Debian 11.
Therefore, we need the uts library and the adminq is working as intended.

On Fri, Jan 12, 2024, 1:38 AM Stephen Hemminger <stephen@networkplumber.org>
wrote:

> This seems wrong:
>         *driver_info = (struct gve_driver_info) {
>                 .os_type = 5, /* DPDK */
>                 .driver_major = GVE_VERSION_MAJOR,
>                 .driver_minor = GVE_VERSION_MINOR,
>                 .driver_sub = GVE_VERSION_SUB,
>                 .os_version_major = cpu_to_be32(DPDK_VERSION_MAJOR),
>                 .os_version_minor = cpu_to_be32(DPDK_VERSION_MINOR),
>                 .os_version_sub = cpu_to_be32(DPDK_VERSION_SUB),
>                 .driver_capability_flags = {
>                         cpu_to_be64(GVE_DRIVER_CAPABILITY_FLAGS1),
>                         cpu_to_be64(GVE_DRIVER_CAPABILITY_FLAGS2),
>                         cpu_to_be64(GVE_DRIVER_CAPABILITY_FLAGS3),
>                         cpu_to_be64(GVE_DRIVER_CAPABILITY_FLAGS4),
>                 },
>         };
>
>         populate_driver_version_strings((char
> *)driver_info->os_version_str1,
>                         (char *)driver_info->os_version_str2);
>
> The numeric values os_version_major, os_version_minor use DPDK version
> which
> is good.  But the populate_driver_version_strings gets the Linux kernel
> version number which is problematic.  Looks like a bug to me.
>
> Also technically, the Linux kernel version is not the same as the OS
> release.
>
> Shouldn't it be more like:
>
> diff --git a/drivers/net/gve/base/gve_osdep.h
> b/drivers/net/gve/base/gve_osdep.h
> index a3702f4b8c8d..914746c8d226 100644
> --- a/drivers/net/gve/base/gve_osdep.h
> +++ b/drivers/net/gve/base/gve_osdep.h
> @@ -171,17 +171,4 @@ gve_free_dma_mem(struct gve_dma_mem *mem)
>         mem->pa = 0;
>  }
>
> -static inline void
> -populate_driver_version_strings(char *str1, char *str2)
> -{
> -       struct utsname uts;
> -       if (uname(&uts) >= 0) {
> -               /* release */
> -               rte_strscpy(str1, uts.release,
> -                       OS_VERSION_STRLEN);
> -               /* version */
> -               rte_strscpy(str2, uts.version,
> -                       OS_VERSION_STRLEN);
> -       }
> -}
>  #endif /* _GVE_OSDEP_H_ */
> diff --git a/drivers/net/gve/gve_ethdev.c b/drivers/net/gve/gve_ethdev.c
> index ecd37ff37f55..b8c48fc657b9 100644
> --- a/drivers/net/gve/gve_ethdev.c
> +++ b/drivers/net/gve/gve_ethdev.c
> @@ -273,8 +273,8 @@ gve_verify_driver_compatibility(struct gve_priv *priv)
>                 },
>         };
>
> -       populate_driver_version_strings((char
> *)driver_info->os_version_str1,
> -                       (char *)driver_info->os_version_str2);
> +       rte_strscpy(driver_info.os_version_str1, OS_VERSION_STRLEN,
> +                   rte_version());
>
>         err = gve_adminq_verify_driver_compatibility(priv,
>                 sizeof(struct gve_driver_info),
>
diff mbox series

Patch

diff --git a/drivers/net/gve/base/gve_osdep.h b/drivers/net/gve/base/gve_osdep.h
index a3702f4b8c8d..914746c8d226 100644
--- a/drivers/net/gve/base/gve_osdep.h
+++ b/drivers/net/gve/base/gve_osdep.h
@@ -171,17 +171,4 @@  gve_free_dma_mem(struct gve_dma_mem *mem)
 	mem->pa = 0;
 }
 
-static inline void
-populate_driver_version_strings(char *str1, char *str2)
-{
-	struct utsname uts;
-	if (uname(&uts) >= 0) {
-		/* release */
-		rte_strscpy(str1, uts.release,
-			OS_VERSION_STRLEN);
-		/* version */
-		rte_strscpy(str2, uts.version,
-			OS_VERSION_STRLEN);
-	}
-}
 #endif /* _GVE_OSDEP_H_ */
diff --git a/drivers/net/gve/gve_ethdev.c b/drivers/net/gve/gve_ethdev.c
index ecd37ff37f55..b8c48fc657b9 100644
--- a/drivers/net/gve/gve_ethdev.c
+++ b/drivers/net/gve/gve_ethdev.c
@@ -273,8 +273,8 @@  gve_verify_driver_compatibility(struct gve_priv *priv)
 		},
 	};
 
-	populate_driver_version_strings((char *)driver_info->os_version_str1,
-			(char *)driver_info->os_version_str2);
+	rte_strscpy(driver_info.os_version_str1, OS_VERSION_STRLEN,
+		    rte_version());
 
 	err = gve_adminq_verify_driver_compatibility(priv,
 		sizeof(struct gve_driver_info),