diff mbox series

[LEDE-DEV] uqmi: Fix for big endian arch

Message ID 1522592555-14982-1-git-send-email-oskari@lemmela.net
State Accepted
Delegated to: Felix Fietkau
Headers show
Series [LEDE-DEV] uqmi: Fix for big endian arch | expand

Commit Message

Oskari Lemmelä April 1, 2018, 2:22 p.m. UTC 
leXX_to_cpu function messes up get_next value in big endian arch.

Signed-off-by: Oskari Lemmelä <oskari@lemmela.net>
---
 data/gen-code.pl | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Oskari Lemmelä April 7, 2018, 6:38 a.m. UTC | #1 
Hi,

Here is better fix for endianness issue.

Tested on both ar71xx (big) and sunxi (little).


Use memcpy instead of pointer casting

Signed-off-by: Oskari Lemmelä <oskari@lemmela.net>
---
  data/gen-code.pl | 26 +++++++++++++-------------
  1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/data/gen-code.pl b/data/gen-code.pl
index f45d28a..1af143c 100755
--- a/data/gen-code.pl
+++ b/data/gen-code.pl
@@ -13,22 +13,22 @@ my $varsize_field;
  my %tlv_get = (
  	gint8 => "*(int8_t *) get_next(1)",
  	guint8 => "*(uint8_t *) get_next(1)",
-	gint16 => "le16_to_cpu(*(uint16_t *) get_next(2))",
-	guint16 => "le16_to_cpu(*(uint16_t *) get_next(2))",
-	gint32 => "le32_to_cpu(*(uint32_t *) get_next(4))",
-	guint32 => "le32_to_cpu(*(uint32_t *) get_next(4))",
-	gint64 => "le64_to_cpu(*(uint64_t *) get_next(8))",
-	guint64 => "le64_to_cpu(*(uint64_t *) get_next(8))",
-	gfloat => "({ uint32_t data = le32_to_cpu(*(uint32_t *) get_next(4)); 
float _val; memcpy(&_val, &data, sizeof(_val)); _val; })"
+	gint16 => "({ uint16_t var; memcpy(&var, get_next(2), 2); 
le16_to_cpu(var); })",
+	guint16 => "({ uint16_t var; memcpy(&var, get_next(2), 2); 
le16_to_cpu(var); })",
+	gint32 => "({ uint32_t var; memcpy(&var, get_next(4), 4); 
le32_to_cpu(var); })",
+	guint32 => "({ uint32_t var; memcpy(&var, get_next(4), 4); 
le32_to_cpu(var); })",
+	gint64 => "({ uint64_t var; memcpy(&var, get_next(8), 8); 
le64_to_cpu(var); })",
+	guint64 => "({ uint64_t var; memcpy(&var, get_next(8), 8); 
le64_to_cpu(var); })",
+	gfloat => "({ float var; memcpy(&var, get_next(4), 4); 
le32_to_cpu(var); })"
  );

  my %tlv_get_be = (
-	gint16 => "be16_to_cpu(*(uint16_t *) get_next(2))",
-	guint16 => "be16_to_cpu(*(uint16_t *) get_next(2))",
-	gint32 => "be32_to_cpu(*(uint32_t *) get_next(4))",
-	guint32 => "be32_to_cpu(*(uint32_t *) get_next(4))",
-	gint64 => "be64_to_cpu(*(uint64_t *) get_next(8))",
-	guint64 => "be64_to_cpu(*(uint64_t *) get_next(8))",
+	gint16 => "({ uint16_t var; memcpy(&var, get_next(2), 2); 
be16_to_cpu(var); })",
+	guint16 => "({ uint16_t var; memcpy(&var, get_next(2), 2); 
be16_to_cpu(var); })",
+	gint32 => "({ uint32_t var; memcpy(&var, get_next(4), 4); 
be32_to_cpu(var); })",
+	guint32 => "({ uint32_t var; memcpy(&var, get_next(4), 4); 
be32_to_cpu(var); })",
+	gint64 => "({ uint64_t var; memcpy(&var, get_next(8), 8); 
be64_to_cpu(var); })",
+	guint64 => "({ uint64_t var; memcpy(&var, get_next(8), 8); 
be64_to_cpu(var); })",
  );

  sub gen_tlv_parse_field($$$$) {
Felix Fietkau April 7, 2018, 8:43 a.m. UTC | #2 
On 2018-04-01 16:22, Oskari Lemmela wrote:
> leXX_to_cpu function messes up get_next value in big endian arch.
> 
> Signed-off-by: Oskari Lemmelä <oskari@lemmela.net>
Please try this libubox patch instead. It should ensure that
the argument is evaluated only once.

--- a/utils.h
+++ b/utils.h
@@ -117,21 +117,29 @@ int clock_gettime(int type, struct timespec *tv);
 	(((uint64_t)(x) & (uint64_t)0x00ff000000000000ULL) >> 40) |	\
 	(((uint64_t)(x) & (uint64_t)0xff00000000000000ULL) >> 56)))
 
+/*
+ * This returns a constant expression while determining if an argument is
+ * a constant expression, most importantly without evaluating the argument.
+ */
+#define __is_constant(x)						\
+	(sizeof(int) == sizeof(*(1 ? ((void*)((long)(x) * 0l)) : (int*)1)))
 
-static inline uint16_t __u_bswap16(uint16_t val)
-{
-	return ((val >> 8) & 0xffu) | ((val & 0xffu) << 8);
-}
+#define __eval_once(func, x)						\
+	({ typeof(x) __x = x; func(__x); })
+
+#define __eval_safe(func, x)						\
+	__builtin_choose_expr(__is_constant(x),				\
+			      func(x), __eval_once(func, x))
 
 #if __BYTE_ORDER == __LITTLE_ENDIAN
 
-#define cpu_to_be64(x) __constant_swap64(x)
-#define cpu_to_be32(x) __constant_swap32(x)
-#define cpu_to_be16(x) __constant_swap16(x)
+#define cpu_to_be64(x) __eval_safe(__constant_swap64, x)
+#define cpu_to_be32(x) __eval_safe(__constant_swap32, x)
+#define cpu_to_be16(x) __eval_safe(__constant_swap16, x)
 
-#define be64_to_cpu(x) __constant_swap64(x)
-#define be32_to_cpu(x) __constant_swap32(x)
-#define be16_to_cpu(x) __constant_swap16(x)
+#define be64_to_cpu(x) __eval_safe(__constant_swap64, x)
+#define be32_to_cpu(x) __eval_safe(__constant_swap32, x)
+#define be16_to_cpu(x) __eval_safe(__constant_swap16, x)
 
 #define cpu_to_le64(x) (x)
 #define cpu_to_le32(x) (x)
@@ -143,13 +151,13 @@ static inline uint16_t __u_bswap16(uint16_t val)
 
 #else /* __BYTE_ORDER == __LITTLE_ENDIAN */
 
-#define cpu_to_le64(x) __constant_swap64(x)
-#define cpu_to_le32(x) __constant_swap32(x)
-#define cpu_to_le16(x) __constant_swap16(x)
+#define cpu_to_le64(x) __eval_safe(__constant_swap64, x)
+#define cpu_to_le32(x) __eval_safe(__constant_swap32, x)
+#define cpu_to_le16(x) __eval_safe(__constant_swap16, x)
 
-#define le64_to_cpu(x) __constant_swap64(x)
-#define le32_to_cpu(x) __constant_swap32(x)
-#define le16_to_cpu(x) __constant_swap16(x)
+#define le64_to_cpu(x) __eval_safe(__constant_swap64, x)
+#define le32_to_cpu(x) __eval_safe(__constant_swap32, x)
+#define le16_to_cpu(x) __eval_safe(__constant_swap16, x)
 
 #define cpu_to_be64(x) (x)
 #define cpu_to_be32(x) (x)
Oskari Lemmelä April 7, 2018, 12:29 p.m. UTC | #3 
Hi,

This libubox patch seems to fix issue.

root@test:/tmp# ./uqmi -d /dev/cdc-wdm0 --wda-get-data-format
qmi_parse_wda_get_data_format_response: Invalid TLV length in message, 
tlv=0x11, len=4
"unknown"
root@test:/tmp# ./uqmi-libubox-patch  -d /dev/cdc-wdm0 --wda-get-data-format
"raw-ip"

Oskari

On 04/07/2018 11:43 AM, Felix Fietkau wrote:
> On 2018-04-01 16:22, Oskari Lemmela wrote:
>> leXX_to_cpu function messes up get_next value in big endian arch.
>>
>> Signed-off-by: Oskari Lemmelä <oskari@lemmela.net>
> Please try this libubox patch instead. It should ensure that
> the argument is evaluated only once.
> 
> --- a/utils.h
> +++ b/utils.h
> @@ -117,21 +117,29 @@ int clock_gettime(int type, struct timespec *tv);
>   	(((uint64_t)(x) & (uint64_t)0x00ff000000000000ULL) >> 40) |	\
>   	(((uint64_t)(x) & (uint64_t)0xff00000000000000ULL) >> 56)))
>   
> +/*
> + * This returns a constant expression while determining if an argument is
> + * a constant expression, most importantly without evaluating the argument.
> + */
> +#define __is_constant(x)						\
> +	(sizeof(int) == sizeof(*(1 ? ((void*)((long)(x) * 0l)) : (int*)1)))
>   
> -static inline uint16_t __u_bswap16(uint16_t val)
> -{
> -	return ((val >> 8) & 0xffu) | ((val & 0xffu) << 8);
> -}
> +#define __eval_once(func, x)						\
> +	({ typeof(x) __x = x; func(__x); })
> +
> +#define __eval_safe(func, x)						\
> +	__builtin_choose_expr(__is_constant(x),				\
> +			      func(x), __eval_once(func, x))
>   
>   #if __BYTE_ORDER == __LITTLE_ENDIAN
>   
> -#define cpu_to_be64(x) __constant_swap64(x)
> -#define cpu_to_be32(x) __constant_swap32(x)
> -#define cpu_to_be16(x) __constant_swap16(x)
> +#define cpu_to_be64(x) __eval_safe(__constant_swap64, x)
> +#define cpu_to_be32(x) __eval_safe(__constant_swap32, x)
> +#define cpu_to_be16(x) __eval_safe(__constant_swap16, x)
>   
> -#define be64_to_cpu(x) __constant_swap64(x)
> -#define be32_to_cpu(x) __constant_swap32(x)
> -#define be16_to_cpu(x) __constant_swap16(x)
> +#define be64_to_cpu(x) __eval_safe(__constant_swap64, x)
> +#define be32_to_cpu(x) __eval_safe(__constant_swap32, x)
> +#define be16_to_cpu(x) __eval_safe(__constant_swap16, x)
>   
>   #define cpu_to_le64(x) (x)
>   #define cpu_to_le32(x) (x)
> @@ -143,13 +151,13 @@ static inline uint16_t __u_bswap16(uint16_t val)
>   
>   #else /* __BYTE_ORDER == __LITTLE_ENDIAN */
>   
> -#define cpu_to_le64(x) __constant_swap64(x)
> -#define cpu_to_le32(x) __constant_swap32(x)
> -#define cpu_to_le16(x) __constant_swap16(x)
> +#define cpu_to_le64(x) __eval_safe(__constant_swap64, x)
> +#define cpu_to_le32(x) __eval_safe(__constant_swap32, x)
> +#define cpu_to_le16(x) __eval_safe(__constant_swap16, x)
>   
> -#define le64_to_cpu(x) __constant_swap64(x)
> -#define le32_to_cpu(x) __constant_swap32(x)
> -#define le16_to_cpu(x) __constant_swap16(x)
> +#define le64_to_cpu(x) __eval_safe(__constant_swap64, x)
> +#define le32_to_cpu(x) __eval_safe(__constant_swap32, x)
> +#define le16_to_cpu(x) __eval_safe(__constant_swap16, x)
>   
>   #define cpu_to_be64(x) (x)
>   #define cpu_to_be32(x) (x)
> 
> _______________________________________________
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev
>
diff mbox series

Patch

diff --git a/data/gen-code.pl b/data/gen-code.pl
index f45d28a..cf46b67 100755
--- a/data/gen-code.pl
+++ b/data/gen-code.pl
@@ -13,12 +13,12 @@  my $varsize_field;
 my %tlv_get = (
 	gint8 => "*(int8_t *) get_next(1)",
 	guint8 => "*(uint8_t *) get_next(1)",
-	gint16 => "le16_to_cpu(*(uint16_t *) get_next(2))",
-	guint16 => "le16_to_cpu(*(uint16_t *) get_next(2))",
-	gint32 => "le32_to_cpu(*(uint32_t *) get_next(4))",
-	guint32 => "le32_to_cpu(*(uint32_t *) get_next(4))",
-	gint64 => "le64_to_cpu(*(uint64_t *) get_next(8))",
-	guint64 => "le64_to_cpu(*(uint64_t *) get_next(8))",
+	gint16 => "({ int16_t value = *(int16_t *) get_next(2); int16_t _val = le16_to_cpu(value); _val;})",
+	guint16 => "({ uint16_t value = *(uint16_t *) get_next(2); uint16_t _val = le16_to_cpu(value); _val;})",
+	gint32 => "({ int32_t value = *(int32_t *) get_next(4); int32_t _val = le32_to_cpu(value); _val;})",
+	guint32 => "({ uint32_t value = *(uint32_t *) get_next(4); uint32_t _val = le32_to_cpu(value); _val;})",
+	gint64 => "({ int64_t value = *(int64_t *) get_next(8); int64_t _val = le64_to_cpu(value); _val;})",
+	guint64 => "({ uint64_t value = *(uint64_t *) get_next(8); uint64_t _val = le64_to_cpu(value); _val;})",
 	gfloat => "({ uint32_t data = le32_to_cpu(*(uint32_t *) get_next(4)); float _val; memcpy(&_val, &data, sizeof(_val)); _val; })"
 );