[PATCH 1/2] Fix bug in uplink header type 1 parsing

Bhargava Abhyankar Bhargava.Abhyankar at radisys.com
Wed Apr 13 09:45:22 UTC 2016


Hello Holger,

> On 08 Apr 2016, at 03:49 AM, Holger Freyther  <holger at freyther.de>  wrote:

> -	e_ti_header = (data[offs-1] + (data[offs] << 8)) >> 7;
> +	e_ti_header = (data[offs - 1] & (0xc0)) >> 6;

>> Could you please elaborate about this hunk? What is the issue, how was it found, how are we certain that this is the fix?

 According to Figure 10.3a.4.3.1 of TS 44.060, E and TI bits are spread across two bytes for header type 3. In earlier patch sent for header type 1 same was incorrectly used. Later for header type 1, E and TI bits extraction was corrected  as given in the spec 10.3a.4.1.1 of TS 44.060  for first RLC data block. According to the fix, The E and TI field are present in last byte of RLC header.

> -	rlc->block_info[0].e   = !!(e_ti_header & 0x01);
> -	rlc->block_info[0].ti  = !!(e_ti_header & 0x02);
> +	rlc->block_info[0].e   = (e_ti_header & 0x01);
> +	rlc->block_info[0].ti  = (e_ti_header & 0x02);

>> what is the issue with this? The intention of !! is to normalize the value to 0 and 1. Now .ti can have 0x02 and 0x00 as value. Why is that needed?
	
Due to oversight  normalization is skipped, Normalizing is required, and will be adapted in correction patch.
 
> -	cur_bit += rlc->data_offs_bits[1] - 2;
> +	cur_bit = rlc->data_offs_bits[1] - 2;

>> Why don't we need to advance this cursor anymore?

In case of header type 1, we have two data blocks and one header.  rlc->data_offs_bits[1] - 2 will have number of bits of header + first data block, then we calculate length of second data block and add it which makes it to be number of bits of header + 2 data blocks.
 
> -	e_ti_header = (data[offs-1] + (data[offs] << 8)) >> 7;
> +	e_ti_header = (data[offs] & (0x03));

>> What is the misunderstanding here. How did we think we needed to take bits from two different octets?

According to Figure 10.3a.4.3.1 of TS 44.060, E and TI bits are spread across two bytes for header type 3. In earlier patch sent for header type 1 same was incorrectly used. Later for header type 1, E and TI bits extraction was corrected  as given in the spec 10.3a.4.1.1 of TS 44.060  for second RLC data block. According to the fix, The E and TI field are present first 2 LSBs after first RLC data block.

> -	rlc->block_info[1].e   = !!(e_ti_header & 0x01);
> -	rlc->block_info[1].ti  = !!(e_ti_header & 0x02);
> +	rlc->block_info[1].e   = (e_ti_header & 0x01);
> +	rlc->block_info[1].ti  = (e_ti_header & 0x02);
>> Again, why the change from 0 or 1 to .. 0 or 1 .. and 0 or 2?

Due to oversight  normalization is skipped, Normalizing is required, and will be adapted in correction patch.

> 	egprs1->r = 1;
> 	egprs1->cv = 7;
> 	egprs1->tfi_a = tfi & (~((~0) << 2));
> -	egprs1->tfi_b = tfi & (~((~0) << 3)) << 2;
> -	egprs1->bsn1_a = 10;
> -	egprs1->bsn1_b = 17;
> -	egprs1->bsn2_a = 0;
> -	egprs1->bsn2_b = 25;
> +	egprs1->tfi_b = (tfi & (~((~0) << 3)) << 2) >> 2;
> +	egprs1->bsn1_a = 0;
> +	egprs1->bsn1_b = 0;
> +	egprs1->bsn2_a = 1;
> +	egprs1->bsn2_b = 0;
> 	egprs1->cps = 15;
> 	egprs1->rsb = 0;
> 	egprs1->pi = 0;
> -	data[6] = 1;
> -	data[6 + 68] = 1;
> -	data[75] = 1;
> -	data[75 + 68] = 1;
> +	data[5] = 0x40;
> +	data[5 + 69] = 1;

>> Please explain the intention here. What is the issue with tfi_b? Why do you change it. Why do you pick the new bsn1_a and bsn1_b values

Tfi_b was filled inappropriately. According to 10.3a.4.1 in 44.060. bsn1_a and bsn1_b are changed to starting values of the sequence number which is 0 and 1. 

Regards
Bhargava Abhyankar


More information about the osmocom-net-gprs mailing list