Messages in this thread | | | Date | Wed, 6 May 2020 10:59:25 +0200 | From | Mauro Carvalho Chehab <> | Subject | Re: [RFC, WIP, v4 09/11] media: vidtv: implement a PES packetizer |
| |
Em Wed, 6 May 2020 03:55:48 -0300 "Daniel W. S. Almeida" <dwlsalmeida@gmail.com> escreveu:
> Hi Mauro, > > > > As commented, don't use WARN_ON(). At most, you could use WARN_ON_ONCE(), > > as otherwise, you may end by causing serious performance issues if > > the code starts to produce a flood of warnings at the dmesg. > > > > I would use pr_warn_ratelimit() on all such cases. > > > > OK. > > > > > > I don't like the idea of changing the "from" buffer endiannes, copy > > and then restore it back to the original state. Is this really needed? > > > > I would, instead, define: > > > > struct pes_header { > > ... > > __be32 bitfield; > > __be16 length; > > ... > > }; > > > > Then wherever you would touch them: > > > > u32 bitfield; > > u16 len; > > > > /* Write into BE fields */ > > pes_header.bitfield = cpu_to_be32(bitfield); > > pes_header.length = cpu_to_be16(len); > > > > /* Read from BE fields */ > > bitfield = be32_to_cpu(pes_header.bitfield); > > len = be16_to_cpu(pes_header.length); > > > > > > As a side effect, when you use "__be16" and "__be32" types, gcc > > and smatch/sparse will warn you if you mess with endiannes. > > > > Same applies to similar code elsewhere. > > > > I don't like it either, it is error prone. I did not know about this > other possibility. Does this work for _bitfields_ though?
See my comment below.
> I think the authors for libdvbv5 used unions precisely so bswap() could > be called on a 'bitfield' member and from then on the bitfields could be > accessed directly, e.g.: > > union { > u16 bitfield; <-- call bswap() on this > struct { > --> then use these directly: > u8 syntax:1; > u8 zero:1; > u8 one:2; > u16 section_length:12; > } __packed; > } __packed > > At least that's what I understood.
You should double-check the structs from the specs. If I'm not mistaken, bytes were swapped on some places. As I commented for patch 08/11, the focus there were to make life simpler for userspace, and not to store a precise copy of the byte order.
> > I found this: > https://lwn.net/Articles/741762/ > > Maybe *_get_bits, *_replace_bits is the equivalent that I should use for bitfields?
I never used them, but, based on their definition:
static __always_inline base type##_get_bits(__##type v, base field) \ { \ return (from(v) & field)/field_multiplier(field); \ }
Calling be16_get_bits should do the right cast to the type.
I don't know what the "from()" and "to()" macros would do.
I guess you will need to do some tests to see if this works as expected.
> > Because I'd rather not do this: > > > u32 bitfield; > > /* Write into BE fields */ > > pes_header.bitfield = cpu_to_be32(bitfield); > > Since I'd have to write the (many!) bitwise operations myself and I'm > sure I will mess this up at _some_ point.
If you mess up, gcc (and/or smatch) will complain. I mean,
if bitfield is declared as __be32, if you do:
u32 bitfield; pes_header.bitfield = bitfield;
this will produce warnings.
Thanks, Mauro
| |