<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <p>Hi Mauro, as always, thank you for reviewing my code!</p>
    <p><br>
    </p>
    <p>Sorry for making you repeat yourself on the alignment of
      arguments and on multi-line comment syntax, I am aware of these
      and I thought I had fixed them all, but some just slip by
      sometimes.</p>
    <p><br>
    </p>
    <p>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">As we talked via IRC in priv, the best would be to implement the MPEG_TS
generator as part of the bridge DVB driver.

Anyway, I will review the code below assuming that you'll be moving the
implementation to the right place.
</pre>
      </blockquote>
    </p>
    <p>Yes. Please let me know when the changes in
      experimental/media-kconfig land, since I'd like to rename and move
      all the code - including the bridge I have been working on - to
      test_drivers/vidtv. <br>
    </p>
    <p><br>
    </p>
    <p>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+static void dvb_dummy_fe_thread_mpeg_ts_tick(struct dvb_frontend *fe)
+{
+       struct dvb_dummy_fe_state *state = fe->demodulator_priv;
+       const unsigned int SLEEP_MSECS = 10;
+       u32 ticks = 0;
+       u32 i;
+       char *buf = kzalloc(DMX_BUF_LEN, GFP_KERNEL);
+       u32 buffer_offset;
+
+       struct dvb_dummy_table_pat pat = {0};
+       struct dvb_dummy_table_sdt sdt = {0};
</pre>
        <pre class="moz-quote-pre" wrap="">I guess it is ok here, but allocating too much stuff at the stack is
dangerous. Linux Kernel stack is very small. Perhaps the best would
be to place those at the driver's private struct (with is allocated with
kalloc).

</pre>
      </blockquote>
    </p>
    <p>Well, I wasn't aware of that, but most of the data for these
      tables are heap-allocated. How small are we talking about? <br>
    </p>
    <p>But your suggestion is OK with me as well. It would be even
      better if this entire function wasn't in this patch at all, since
      it will have to be moved to the bridge driver anyways.</p>
    <p>The *_write_args structures are also pretty small.</p>
    <p>
      <blockquote type="cite">
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">+
+       struct dvb_dummy_table_pmt *pmt_sections;
+
+       struct dvb_dummy_table_pat_program *programs = NULL;
+       struct dvb_dummy_table_sdt_service *services = NULL;
+
+       bool update_version_num = false;
+       u16 pmt_pid;
+
+       programs = dummy_fe_pat_prog_cat_into_new(state->channels);
+       services = dummy_fe_sdt_serv_cat_into_new(state->channels);
+
+       /* assemble all programs and assign to PAT */
+       dummy_fe_pat_program_assign(&pat, programs);
+
+       /* assemble all services and assign to SDT */
+       dummy_fe_sdt_service_assign(&sdt, services);
+
+       /* a section for each program_id */
+       pmt_sections = kcalloc(pat.programs,
+                              sizeof(struct dvb_dummy_table_pmt),
+                              GFP_KERNEL);
+
+       dummy_fe_pmt_create_section_for_each_pat_entry(&pat,
+                                                      pmt_sections);
+
+       dummy_fe_pmt_stream_match_with_sections(state->channels,
+                                               pmt_sections,
+                                               pat.programs);
+
+       dummy_fe_pat_table_init(&pat,
+                               update_version_num,
+                               TRANSPORT_STREAM_ID);
+
+       dummy_fe_sdt_table_init(&sdt,
+                               update_version_num,
+                               TRANSPORT_STREAM_ID);
+       while (true) {
+               memset(buf, 0, DMX_BUF_LEN);
+               buffer_offset = 0;
+
+               if ((ticks % 50) == 0) {
+                       /* push PSI packets into the buffer */
+
+                       buffer_offset +=
+                               dummy_fe_pat_write_into(buf,
+                                                       buffer_offset,
+                                                       &pat);
+                       for (i = 0; i < pat.programs; ++i) {
+                               pmt_pid =
+                               dummy_fe_pmt_get_pid(&pmt_sections[i],
+                                                    &pat);
+
+                               /* not found */
+                               WARN_ON(pmt_pid > LAST_VALID_TS_PID);
+
+                               /* write each section into buffer */
+                               buffer_offset +=
+                               dummy_fe_pmt_write_into(buf,
+                                                       buffer_offset,
+                                                       &pmt_sections[i],
+                                                       pmt_pid);
+                       }
+
+                       buffer_offset +=
+                               dummy_fe_sdt_write_into(buf,
+                                                       buffer_offset,
+                                                       &sdt);
+
+                       WARN_ON(buffer_offset > DMX_BUF_LEN); /* overflow */
</pre>
        </blockquote>
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">+                        msleep_interruptible(SLEEP_MSECS);
</pre>
        </blockquote>
        <pre class="moz-quote-pre" wrap="">That doesn't sound right, for two reasons:

1) msleep_interruptible() can take less time than expected, if
   interupted;
2) the time may vary a lot.

I would use the high-res timer here, giving a range for it (maybe a 10ms
range?), e. g., something like:

                        usleep_range(SLEEP_USECS, SLEEP_USECS + 10000);



</pre>
      </blockquote>
      OK. I wonder if this will have to be rewritten in the future,
      since decoders will probably expect X amount of video/audio per Y
      amount of time?</p>
    <p><br>
    </p>
    <p>As for buffer overflows, maybe a better strategy would be to use
      a dynamic array? I would wrap all memcpy() calls and call
      krealloc() as necessary. If we go with a big enough initial size
      (say, 20 TS packets) then this wouldn't happen very often, if at
      all. That would be a simple solution to completely eliminate this
      problem, in my opinion.</p>
    <p><br>
    </p>
    <p>I don't know if there's a limit on how much data you can pass to
      the demux at once, but if there is, we can just split the buffer
      into smaller chunks when calling dmx_swfilter_packets(), since the
      amount of bytes actually present in the buffer will be a multiple
      of 188 anyways.<br>
    </p>
    <p><br>
    </p>
    <p>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+  }
+
+       length += CRC_SIZE_IN_BYTES;
+
+       WARN_ON(length > SDT_MAX_SECTION_LEN);
</pre>
        <pre class="moz-quote-pre" wrap="">even assuming that you fix the above code, and update "s" to the next
SDT data, this is still too dangerous: if are there any risk of going 
past the buffer size, you should check <b class="moz-txt-star"><span class="moz-txt-tag">*</span>before<span class="moz-txt-tag">*</span></b> the bad condition happens,
e. g., something like:

        while (s && length + CRC_SIZE_IN_BYTES < SDT_MAX_SECTION_LEN) {
                ...
        }

        if (s)
                WARN_ON(length > SDT_MAX_SECTION_LEN);

</pre>
        <blockquote type="cite" style="color: #000000;">
          <pre class="moz-quote-pre" wrap="">+        return length;
+}
+</pre>
        </blockquote>
      </blockquote>
      <br>
    </p>
    <p>My understanding is that, e.g. <br>
    </p>
    <pre class="moz-quote-pre" wrap="">length > SDT_MAX_SECTION_LEN</pre>
    <p>doesn't mean that the buffer will necessarily overflow. It is
      just against the spec.<br>
    </p>
    <p><br>
    </p>
    <p>Best regards</p>
    <p>- Daniel.<br>
    </p>
    <br>
  </body>
</html>