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 |
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($$$$) {
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)
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 --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; })" );
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(-)