From: "Daniel W. S. Almeida" <dwlsalmeida@gmail.com> To: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> Cc: "sean@mess.org" <sean@mess.org>, "kstewart@linuxfoundation.org" <kstewart@linuxfoundation.org>, "allison@lohutok.net" <allison@lohutok.net>, "tglx@linutronix.de" <tglx@linutronix.de>, "linux-media@vger.kernel.org" <linux-media@vger.kernel.org>, "skhan@linuxfoundation.org" <skhan@linuxfoundation.org>, "linux-kernel-mentees@lists.linuxfoundation.org" <linux-kernel-mentees@lists.linuxfoundation.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org> Subject: Re: [RFC, WIP, v4 09/11] media: vidtv: implement a PES packetizer Date: Wed, 6 May 2020 03:55:48 -0300 [thread overview] Message-ID: <40C2F764-6E43-418B-8904-952C5E99D9D9@getmailspring.com> (raw) In-Reply-To: <20200503101621.50047b5c@coco.lan> 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? 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. I found this: https://lwn.net/Articles/741762/ Maybe *_get_bits, *_replace_bits is the equivalent that I should use for bitfields? 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. thanks, - Daniel
WARNING: multiple messages have this Message-ID (diff)
From: "Daniel W. S. Almeida" <dwlsalmeida@gmail.com> To: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> Cc: "kstewart@linuxfoundation.org" <kstewart@linuxfoundation.org>, "sean@mess.org" <sean@mess.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "tglx@linutronix.de" <tglx@linutronix.de>, "linux-kernel-mentees@lists.linuxfoundation.org" <linux-kernel-mentees@lists.linuxfoundation.org>, "allison@lohutok.net" <allison@lohutok.net>, "linux-media@vger.kernel.org" <linux-media@vger.kernel.org> Subject: Re: [Linux-kernel-mentees] [RFC, WIP, v4 09/11] media: vidtv: implement a PES packetizer Date: Wed, 6 May 2020 03:55:48 -0300 [thread overview] Message-ID: <40C2F764-6E43-418B-8904-952C5E99D9D9@getmailspring.com> (raw) In-Reply-To: <20200503101621.50047b5c@coco.lan> 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? 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. I found this: https://lwn.net/Articles/741762/ Maybe *_get_bits, *_replace_bits is the equivalent that I should use for bitfields? 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. thanks, - Daniel _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
next prev parent reply other threads:[~2020-05-06 6:56 UTC|newest] Thread overview: 69+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-05-02 3:22 [RFC, WIP, v4 00/11] media: vidtv: implement a virtual DVB driver Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 3:22 ` [RFC, WIP, v4 01/11] media: vidtv: add Kconfig entry Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 4:58 ` Mauro Carvalho Chehab 2020-05-02 4:58 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 3:22 ` [RFC, WIP, v4 02/11] media: vidtv: implement a tuner driver Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 5:27 ` Mauro Carvalho Chehab 2020-05-02 5:27 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 3:22 ` [RFC, WIP, v4 03/11] media: vidtv: implement a demodulator driver Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 5:58 ` Mauro Carvalho Chehab 2020-05-02 5:58 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 3:22 ` [RFC, WIP, v4 04/11] media: vidtv: move config structs into a separate header Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 6:02 ` Mauro Carvalho Chehab 2020-05-02 6:02 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 9:28 ` kbuild test robot 2020-05-02 3:22 ` [RFC, WIP, v4 05/11] media: vidtv: add a bridge driver Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 6:30 ` Mauro Carvalho Chehab 2020-05-02 6:30 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 21:12 ` Daniel W. S. Almeida 2020-05-02 21:12 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 10:05 ` kbuild test robot 2020-05-02 3:22 ` [RFC, WIP, v4 06/11] media: vidtv: add wrappers for memcpy and memset Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 6:40 ` Mauro Carvalho Chehab 2020-05-02 6:40 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-03 7:06 ` Mauro Carvalho Chehab 2020-05-03 7:06 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 3:22 ` [RFC, WIP, v4 07/11] media: vidtv: add MPEG TS common code Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 7:09 ` Mauro Carvalho Chehab 2020-05-02 7:09 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 22:22 ` Daniel W. S. Almeida 2020-05-02 22:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-03 9:50 ` Mauro Carvalho Chehab 2020-05-03 9:50 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 3:22 ` [RFC, WIP, v4 08/11] media: vidtv: implement a PSI generator Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-03 7:51 ` Mauro Carvalho Chehab 2020-05-03 7:51 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-06 6:28 ` Daniel W. S. Almeida 2020-05-06 6:28 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-06 8:36 ` Mauro Carvalho Chehab 2020-05-06 8:36 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 3:22 ` [RFC, WIP, v4 09/11] media: vidtv: implement a PES packetizer Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-03 8:16 ` Mauro Carvalho Chehab 2020-05-03 8:16 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-06 6:55 ` Daniel W. S. Almeida [this message] 2020-05-06 6:55 ` Daniel W. S. Almeida 2020-05-06 8:59 ` Mauro Carvalho Chehab 2020-05-06 8:59 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 3:22 ` [RFC, WIP, v4 10/11] media: vidtv: Implement a SMPTE 302M encoder Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-03 8:57 ` Mauro Carvalho Chehab 2020-05-03 8:57 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-02 3:22 ` [RFC, WIP, v4 11/11] media: vidtv: Add a MPEG Transport Stream Multiplexer Daniel W. S. Almeida 2020-05-02 3:22 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-02 9:41 ` kbuild test robot 2020-05-03 9:13 ` Mauro Carvalho Chehab 2020-05-03 9:13 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab 2020-05-06 7:05 ` Daniel W. S. Almeida 2020-05-06 7:05 ` [Linux-kernel-mentees] " Daniel W. S. Almeida 2020-05-06 9:01 ` Mauro Carvalho Chehab 2020-05-06 9:01 ` [Linux-kernel-mentees] " Mauro Carvalho Chehab
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=40C2F764-6E43-418B-8904-952C5E99D9D9@getmailspring.com \ --to=dwlsalmeida@gmail.com \ --cc=allison@lohutok.net \ --cc=kstewart@linuxfoundation.org \ --cc=linux-kernel-mentees@lists.linuxfoundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=mchehab+huawei@kernel.org \ --cc=sean@mess.org \ --cc=skhan@linuxfoundation.org \ --cc=tglx@linutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.