Em Wed, 6 May 2020 03:28:17 -0300
"Daniel W. S. Almeida" <dwlsalmeida@xxxxxxxxx> escreveu:
> >> + /* Just a sanity check, should not really happen because we stuff
> >> + * the packet when we finish a section, i.e. when we write the crc at
> >> + * the end. But if this happens then we have messed up the logic
> >> + * somewhere.
> >> + */
> >> + WARN_ON(args.new_psi_section && !aligned);
> >
> > Please use a ratelimited printk instead (here and on all similar cases
> > at the TS generator).
> >
> > Also, I think that, on such case, the driver should be filling the
> > remaining frame with pad bytes. E. g.:
> >
> > /*
> > * Assuming that vidtv_memset() args from patch 06/11 were changed
> > * according with this prototype:
> > */
> > size_t vidtv_memset(void *to, size_t to_offset, size_t to_size,
> > u8 c, size_t len);
> >
> >
> > #define TS_FILL_BYTE 0xff
> >
> > if (args.new_psi_section && !aligned) {
> > pr_warn_ratelimit("Warning: PSI not aligned. Re-aligning it\n");
> >
> > vidtv_memset(args.dest_buf,
> > args.dest_offset + nbytes_past_boundary,
> > args.dest_buf_sz,
> > TS_FILL_BYTE,
> > TS_PACKET_LEN - nbytes_past_boundary);
> > args.dest_offset += TS_PACKET_LEN - nbytes_past_boundary;
> > aligned = 1;
> > nbytes_past_boundary = 0;
> > }
> >
>
> Sure, that's fine then! Just to be clear this should not happen at all,
> because the writes should go through one of these four functions (IIRC!):
>
> u32 vidtv_ts_null_write_into(struct null_packet_write_args args)
> u32 vidtv_ts_pcr_write_into(struct pcr_write_args args)
>
> ...which will write a single packet at a time, thus leaving the buffer
> aligned if it was already aligned to begin with,
>
> and
>
> u32 vidtv_pes_write_into(struct pes_write_args args)
> static u32
> vidtv_psi_ts_psi_write_into(struct psi_write_args args)
>
> ...which will pad when they finish writing a PES packet or a table
> section, respectively.
>
> I left these warnings behind as a way to warn me if the padding logic
> itself is broken.
OK!
> > Please use a ratelimited printk instead (here and on all similar cases
> > at the TS generator).
>
> OK.
>
>
>
> >> +static void vidtv_psi_desc_to_be(struct vidtv_psi_desc *desc)
> >> +{
> >> + /*
> >> + * Convert descriptor endianness to big-endian on a field-by-field
> >> basis
> >> + * where applicable
> >> + */
> >> +
> >> + switch (desc->type) {
> >> + /* nothing do do */
> >> + case SERVICE_DESCRIPTOR:
> >> + break;
> >> + case REGISTRATION_DESCRIPTOR:
> >> + cpu_to_be32s(&((struct vidtv_psi_desc_registration *)
> >> + desc)->format_identifier);
> >> + pr_alert("%s: descriptor type %d found\n",
> >> + __func__,
> >> + desc->type);
> >> + pr_alert("%s: change 'additional_info' endianness before
> >> calling\n",
> >> + __func__);
> >
> > The above pr_alert() calls sound weird. Why are you unconditionally
> > calling it (and still doing the BE conversion) for all
> > REGISTRATION_DESCRIPTOR types?
>
> To be honest, I did not know what to do. Here's what 13818-1 has to say
> about registration descriptors:
>
> >2.6.9
> > Semantic definition of fields in registration descriptor
> >format_identifier – The format_identifier is a 32-bit value obtained
> >from a Registration Authority as designated by
> >ISO/IEC JTC 1/SC 29.
> >additional_identification_info – The meaning of
> >additional_identification_info bytes, if any, are defined by the
> >assignee of that format_identifier, and once defined they shall not change.
>
> So I took the cue from libdvbv5 and defined the following struct for it,
> with a flexible array member at the end:
André (who re-wrote the libdvbv5 parsers to be more generic)
preferred the approach of keeping the structs in CPU-endian, as it
makes easier from application PoV, as the conversion happens only once
at the library.
That's not the case here. We can fill the structs in big endian,
converting to CPU-endian only on the few places we may need to read
back from it.
>
> struct vidtv_psi_desc_registration {
> u8 type;
> u8 length;
> struct vidtv_psi_desc *next;
>
> /*
> * The format_identifier is a 32-bit value obtained from a Registration
> * Authority as designated by ISO/IEC JTC 1/SC 29.
> */
> u32 format_identifier;
> /*
> * The meaning of additional_identification_info bytes, if any, are
> * defined by the assignee of that format_identifier, and once defined
> * they shall not change.
> */
> u8 additional_identification_info[];
> } __packed
>
>
> As you know, I was changing the endianness from host to BE before
> serializing and then changing back from BE to host. Given the struct
> definition above, there was no way to do this to the
> 'additional_identification_info' member, since we do not know its layout.
>
> If we go with your approach instead (i.e. using __be16, __be32...etc)
> then I think we can remove these two functions (and more)
Yep.
Thanks,
Mauro
