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

Submitted by Bhargava Abhyankar on April 7, 2016, 11:57 a.m.

Details

Message ID 1460030275-543-1-git-send-email-Bhargava.Abhyankar@radisys.com
State Changes Requested
Series "Series without cover letter"
Headers show

Commit Message

Bhargava Abhyankar April 7, 2016, 11:57 a.m.
Fix bug in extraction of E and TI header fields in parsing of uplink
header type 1 RLC data block. Test suite for the same is also updated.
---
 src/decoding.cpp        | 15 ++++++++-------
 tests/edge/EdgeTest.cpp | 16 +++++++---------
 2 files changed, 15 insertions(+), 16 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/decoding.cpp b/src/decoding.cpp
index 6844856..b8e453e 100644
--- a/src/decoding.cpp
+++ b/src/decoding.cpp
@@ -513,9 +513,9 @@  int Decoding::rlc_parse_ul_data_header_egprs_type_1(
 	offs = rlc->data_offs_bits[0] / 8;
 	OSMO_ASSERT(rlc->data_offs_bits[0] % 8 == 0);
 
-	e_ti_header = (data[offs-1] + (data[offs] << 8)) >> 7;
-	rlc->block_info[0].e   = !!(e_ti_header & 0x01);
-	rlc->block_info[0].ti  = !!(e_ti_header & 0x02);
+	e_ti_header = (data[offs - 1] & (0xc0)) >> 6;
+	rlc->block_info[0].e   = (e_ti_header & 0x01);
+	rlc->block_info[0].ti  = (e_ti_header & 0x02);
 	cur_bit += 2;
 
 	rlc->block_info[1].cv  = egprs1->cv;
@@ -524,20 +524,21 @@  int Decoding::rlc_parse_ul_data_header_egprs_type_1(
 		((egprs1->bsn2_a << 0) | (egprs1->bsn2_b << 2));
 	rlc->block_info[1].bsn = rlc->block_info[1].bsn &  (RLC_EGPRS_SNS - 1);
 
-	cur_bit += rlc->data_offs_bits[1] - 2;
+	cur_bit = rlc->data_offs_bits[1] - 2;
 
 	offs = rlc->data_offs_bits[1] / 8;
 	OSMO_ASSERT(rlc->data_offs_bits[1] % 8 == 2);
 
-	e_ti_header = (data[offs-1] + (data[offs] << 8)) >> 7;
-	rlc->block_info[1].e   = !!(e_ti_header & 0x01);
-	rlc->block_info[1].ti  = !!(e_ti_header & 0x02);
+	e_ti_header = (data[offs] & (0x03));
+	rlc->block_info[1].e   = (e_ti_header & 0x01);
+	rlc->block_info[1].ti  = (e_ti_header & 0x02);
 	cur_bit += 2;
 	/* skip data area */
 	cur_bit += cs.maxDataBlockBytes() * 8;
 
 	return cur_bit;
 }
+
 /**
  * \brief Copy LSB bitstream RLC data block to byte aligned buffer.
  *
diff --git a/tests/edge/EdgeTest.cpp b/tests/edge/EdgeTest.cpp
index ff080f9..9b4a1a5 100644
--- a/tests/edge/EdgeTest.cpp
+++ b/tests/edge/EdgeTest.cpp
@@ -1376,18 +1376,16 @@  static gprs_rlcmac_ul_tbf *uplink_header_parsing_test(BTS *the_bts,
 	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;
 	pdch = &the_bts->bts_data()->trx[trx_no].pdch[ts_no];
 	pdch->rcv_block(&data[0], 143, *fn, &meas);
 }

Comments

Holger Freyther April 7, 2016, 10:19 p.m.
> On 07 Apr 2016, at 13:57, Bhargava Abhyankar <Bhargava.Abhyankar@radisys.com> wrote:
> 
> Fix bug in extraction of E and TI header fields in parsing of uplink
> header type 1 RLC data block. Test suite for the same is also updated.
> 

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


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


> 	cur_bit += 2;
> 
> 	rlc->block_info[1].cv  = egprs1->cv;
> @@ -524,20 +524,21 @@ int Decoding::rlc_parse_ul_data_header_egprs_type_1(
> 		((egprs1->bsn2_a << 0) | (egprs1->bsn2_b << 2));
> 	rlc->block_info[1].bsn = rlc->block_info[1].bsn &  (RLC_EGPRS_SNS - 1);
> 
> -	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?

	offs = rlc->data_offs_bits[1] / 8;
> 	OSMO_ASSERT(rlc->data_offs_bits[1] % 8 == 2);
> 
> -	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?


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


> 

> diff --git a/tests/edge/EdgeTest.cpp b/tests/edge/EdgeTest.cpp
> index ff080f9..9b4a1a5 100644
> --- a/tests/edge/EdgeTest.cpp
> +++ b/tests/edge/EdgeTest.cpp
> @@ -1376,18 +1376,16 @@ static gprs_rlcmac_ul_tbf *uplink_header_parsing_test(BTS *the_bts,
> 	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?

holger
Holger Freyther April 7, 2016, 10:22 p.m.
> On 08 Apr 2016, at 00:19, Holger Freyther <holger@freyther.de> wrote:
> 
> 
>> On 07 Apr 2016, at 13:57, Bhargava Abhyankar <Bhargava.Abhyankar@radisys.com> wrote:
>> 
>> Fix bug in extraction of E and TI header fields in parsing of uplink
>> header type 1 RLC data block. Test suite for the same is also updated.
>> 
> 
>> -	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?

And you can confirm that this is an issue in code we have not merged yet? In that case it should be fixed before including a wrong version.

thank you
	holger
Bhargava Abhyankar April 13, 2016, 9:45 a.m.
Hello Holger,

> On 08 Apr 2016, at 03:49 AM, Holger Freyther  <holger@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