Re: [Linux-kernel-mentees] [RFC, WIP, v2 3/3] media: dvb_dummy_fe.c: write PSI information into DMX buffer

["Daniel W. S. Almeida" <dwlsalmeida@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5482 bytes --]

Hi Mauro, as always, thank you for reviewing my code!


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.


> 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.

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.


> +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};
> 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).
>
Well, I wasn't aware of that, but most of the data for these tables are 
heap-allocated. How small are we talking about?

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.

The *_write_args structures are also pretty small.

>> +
>> +	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 */
>> +			msleep_interruptible(SLEEP_MSECS);
> 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);
>
>
>
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?


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.


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.


> +	}
> +
> +	length += CRC_SIZE_IN_BYTES;
> +
> +	WARN_ON(length > SDT_MAX_SECTION_LEN);
> 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*before*  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);
>
>> +	return length;
>> +}
>> +

My understanding is that, e.g.

length > SDT_MAX_SECTION_LEN

doesn't mean that the buffer will necessarily overflow. It is just 
against the spec.


Best regards

- Daniel.



[-- Attachment #1.2: Type: text/html, Size: 6873 bytes --]

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees