<p>pespin <strong>submitted</strong> this change.</p><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/26166">View Change</a></p><div style="white-space:pre-wrap">Approvals:
fixeria: Looks good to me, but someone else must approve
pespin: Looks good to me, approved
Jenkins Builder: Verified
</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">bts_pch_timer: Fix timer working only for MI type IMSI<br><br>This commit actually addresses 2 errors:<br><br>1- gprs_bssgp_pcu_rx_paging_ps() called gprs_rlcmac_paging_request()<br>with MI which can be either TMSI or IMSI, and the later always called<br>bts_pch_timer_start() passing mi->imsi regardless of the MI type. Hence,<br>trash was being accessed & stored into bts_pch_timer structures if MI<br>type used for paging was TMSI.<br><br>2- When the MS received the PS paging on CCCH and requests an UL TBF, it<br>will send some data. If one phase access is used for whatever reason,<br>the IMSI may not be yet available in the GprsMs object since we never<br>received it (and we'd only have it by means of PktResourceReq). Hence,<br>let's better first try to match the paging by TLLI/TMSI if set in both<br>places, and otherwise use the IMSI.<br><br>Related: OS#5297<br>Change-Id: Iedffb7c6978a3faf0fc26ce2181dde9791a8b6f4<br>---<br>M src/bts_pch_timer.c<br>M src/bts_pch_timer.h<br>M src/gprs_bssgp_pcu.c<br>M src/gprs_rlcmac.cpp<br>M src/tbf_ul.cpp<br>M tests/alloc/AllocTest.cpp<br>M tests/alloc/AllocTest.err<br>7 files changed, 59 insertions(+), 21 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/src/bts_pch_timer.c b/src/bts_pch_timer.c</span><br><span>index 20373ac..312d85a 100644</span><br><span>--- a/src/bts_pch_timer.c</span><br><span>+++ b/src/bts_pch_timer.c</span><br><span>@@ -26,8 +26,22 @@</span><br><span> #include <gprs_debug.h></span><br><span> #include <gprs_pcu.h></span><br><span> #include <bts_pch_timer.h></span><br><span style="color: hsl(120, 100%, 40%);">+#include <gprs_ms.h></span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-static struct bts_pch_timer *bts_pch_timer_get(struct gprs_rlcmac_bts *bts, const char *imsi)</span><br><span style="color: hsl(120, 100%, 40%);">+static struct bts_pch_timer *bts_pch_timer_get_by_ptmsi(struct gprs_rlcmac_bts *bts, uint32_t ptmsi)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ struct bts_pch_timer *p;</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(ptmsi != GSM_RESERVED_TMSI);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ llist_for_each_entry(p, &bts->pch_timer, entry) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (p->ptmsi != GSM_RESERVED_TMSI && p->ptmsi == ptmsi)</span><br><span style="color: hsl(120, 100%, 40%);">+ return p;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ return NULL;</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+static struct bts_pch_timer *bts_pch_timer_get_by_imsi(struct gprs_rlcmac_bts *bts, const char *imsi)</span><br><span> {</span><br><span> struct bts_pch_timer *p;</span><br><span> </span><br><span>@@ -57,29 +71,46 @@</span><br><span> bts_pch_timer_remove(p);</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-void bts_pch_timer_start(struct gprs_rlcmac_bts *bts, const char *imsi)</span><br><span style="color: hsl(120, 100%, 40%);">+void bts_pch_timer_start(struct gprs_rlcmac_bts *bts, const struct osmo_mobile_identity *mi_paging,</span><br><span style="color: hsl(120, 100%, 40%);">+ const char *imsi)</span><br><span> {</span><br><span style="color: hsl(0, 100%, 40%);">- if (bts_pch_timer_get(bts, imsi))</span><br><span style="color: hsl(120, 100%, 40%);">+ struct bts_pch_timer *p;</span><br><span style="color: hsl(120, 100%, 40%);">+ struct osmo_tdef *tdef;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ /* We already have a timer running for this IMSI */</span><br><span style="color: hsl(120, 100%, 40%);">+ if (bts_pch_timer_get_by_imsi(bts, imsi))</span><br><span> return;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- struct bts_pch_timer *p;</span><br><span> p = talloc_zero(bts, struct bts_pch_timer);</span><br><span> llist_add_tail(&p->entry, &bts->pch_timer);</span><br><span style="color: hsl(0, 100%, 40%);">- osmo_strlcpy(p->imsi, imsi, sizeof(p->imsi));</span><br><span> p->bts = bts;</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_STRLCPY_ARRAY(p->imsi, imsi);</span><br><span style="color: hsl(120, 100%, 40%);">+ p->ptmsi = (mi_paging->type == GSM_MI_TYPE_TMSI) ? mi_paging->tmsi : GSM_RESERVED_TMSI;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- struct osmo_tdef *tdef = osmo_tdef_get_entry(the_pcu->T_defs, 3113);</span><br><span style="color: hsl(120, 100%, 40%);">+ tdef = osmo_tdef_get_entry(the_pcu->T_defs, 3113);</span><br><span> OSMO_ASSERT(tdef);</span><br><span> osmo_timer_setup(&p->T3113, T3113_callback, p);</span><br><span> osmo_timer_schedule(&p->T3113, tdef->val, 0);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- LOGP(DPCU, LOGL_DEBUG, "PCH paging timer started for IMSI=%s\n", p->imsi);</span><br><span style="color: hsl(120, 100%, 40%);">+ if (log_check_level(DPCU, LOGL_DEBUG)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ char str[64];</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_mobile_identity_to_str_buf(str, sizeof(str), mi_paging);</span><br><span style="color: hsl(120, 100%, 40%);">+ LOGP(DPCU, LOGL_DEBUG, "PCH paging timer started for MI=%s IMSI=%s\n", str, p->imsi);</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-void bts_pch_timer_stop(struct gprs_rlcmac_bts *bts, const char *imsi)</span><br><span style="color: hsl(120, 100%, 40%);">+void bts_pch_timer_stop(struct gprs_rlcmac_bts *bts, const struct GprsMs *ms)</span><br><span> {</span><br><span style="color: hsl(0, 100%, 40%);">- struct bts_pch_timer *p = bts_pch_timer_get(bts, imsi);</span><br><span style="color: hsl(120, 100%, 40%);">+ struct bts_pch_timer *p = NULL;</span><br><span style="color: hsl(120, 100%, 40%);">+ uint32_t tlli = ms_tlli(ms);</span><br><span style="color: hsl(120, 100%, 40%);">+ const char *imsi = ms_imsi(ms);</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+ /* First try matching by TMSI if available in MS */</span><br><span style="color: hsl(120, 100%, 40%);">+ if (tlli != GSM_RESERVED_TMSI)</span><br><span style="color: hsl(120, 100%, 40%);">+ p = bts_pch_timer_get_by_ptmsi(bts, tlli);</span><br><span style="color: hsl(120, 100%, 40%);">+ /* Otherwise try matching by IMSI if available in MS */</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!p && imsi[0] != '\0')</span><br><span style="color: hsl(120, 100%, 40%);">+ p = bts_pch_timer_get_by_imsi(bts, imsi);</span><br><span> if (p)</span><br><span> bts_pch_timer_remove(p);</span><br><span> }</span><br><span>diff --git a/src/bts_pch_timer.h b/src/bts_pch_timer.h</span><br><span>index 26b89c8..3e47161 100644</span><br><span>--- a/src/bts_pch_timer.h</span><br><span>+++ b/src/bts_pch_timer.h</span><br><span>@@ -32,11 +32,15 @@</span><br><span> struct llist_head entry;</span><br><span> struct gprs_rlcmac_bts *bts;</span><br><span> struct osmo_timer_list T3113;</span><br><span style="color: hsl(120, 100%, 40%);">+ uint32_t ptmsi; /* GSM_RESERVED_TMSI if not available */</span><br><span> char imsi[OSMO_IMSI_BUF_SIZE];</span><br><span> };</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-void bts_pch_timer_start(struct gprs_rlcmac_bts *bts, const char *imsi);</span><br><span style="color: hsl(0, 100%, 40%);">-void bts_pch_timer_stop(struct gprs_rlcmac_bts *bts, const char *imsi);</span><br><span style="color: hsl(120, 100%, 40%);">+struct GprsMs;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+void bts_pch_timer_start(struct gprs_rlcmac_bts *bts, const struct osmo_mobile_identity *mi_paging,</span><br><span style="color: hsl(120, 100%, 40%);">+ const char *imsi);</span><br><span style="color: hsl(120, 100%, 40%);">+void bts_pch_timer_stop(struct gprs_rlcmac_bts *bts, const struct GprsMs *ms);</span><br><span> void bts_pch_timer_stop_all(struct gprs_rlcmac_bts *bts);</span><br><span> </span><br><span> #ifdef __cplusplus</span><br><span>diff --git a/src/gprs_bssgp_pcu.c b/src/gprs_bssgp_pcu.c</span><br><span>index 0dd6cdc..424a381 100644</span><br><span>--- a/src/gprs_bssgp_pcu.c</span><br><span>+++ b/src/gprs_bssgp_pcu.c</span><br><span>@@ -39,6 +39,7 @@</span><br><span> #include "tbf_dl.h"</span><br><span> #include "llc.h"</span><br><span> #include "gprs_rlcmac.h"</span><br><span style="color: hsl(120, 100%, 40%);">+#include "bts_pch_timer.h"</span><br><span> </span><br><span> /* Tuning parameters for BSSGP flow control */</span><br><span> #define FC_DEFAULT_LIFE_TIME_SECS 10 /* experimental value, 10s */</span><br><span>@@ -319,7 +320,9 @@</span><br><span> </span><br><span> /* FIXME: look if MS is attached a specific BTS and then only page on that one? */</span><br><span> llist_for_each_entry(bts, &the_pcu->bts_list, list) {</span><br><span style="color: hsl(0, 100%, 40%);">- gprs_rlcmac_paging_request(bts, &paging_mi, pgroup);</span><br><span style="color: hsl(120, 100%, 40%);">+ if (gprs_rlcmac_paging_request(bts, &paging_mi, pgroup) < 0)</span><br><span style="color: hsl(120, 100%, 40%);">+ continue;</span><br><span style="color: hsl(120, 100%, 40%);">+ bts_pch_timer_start(bts, &paging_mi, mi_imsi.imsi);</span><br><span> }</span><br><span> return 0;</span><br><span> }</span><br><span>diff --git a/src/gprs_rlcmac.cpp b/src/gprs_rlcmac.cpp</span><br><span>index 22b12df..ffa656c 100644</span><br><span>--- a/src/gprs_rlcmac.cpp</span><br><span>+++ b/src/gprs_rlcmac.cpp</span><br><span>@@ -26,7 +26,6 @@</span><br><span> #include <pcu_l1_if.h></span><br><span> #include <gprs_rlcmac.h></span><br><span> #include <bts.h></span><br><span style="color: hsl(0, 100%, 40%);">-#include <bts_pch_timer.h></span><br><span> #include <encoding.h></span><br><span> #include <tbf.h></span><br><span> #include <gprs_debug.h></span><br><span>@@ -49,7 +48,6 @@</span><br><span> return -1;</span><br><span> }</span><br><span> bts_do_rate_ctr_inc(bts, CTR_PCH_REQUESTS);</span><br><span style="color: hsl(0, 100%, 40%);">- bts_pch_timer_start(bts, mi->imsi);</span><br><span> pcu_l1if_tx_pch(bts, paging_request, plen, pgroup);</span><br><span> bitvec_free(paging_request);</span><br><span> </span><br><span>diff --git a/src/tbf_ul.cpp b/src/tbf_ul.cpp</span><br><span>index 1d06e53..0282122 100644</span><br><span>--- a/src/tbf_ul.cpp</span><br><span>+++ b/src/tbf_ul.cpp</span><br><span>@@ -443,7 +443,7 @@</span><br><span> "Decoded premier TLLI=0x%08x of UL DATA TFI=%d.\n",</span><br><span> new_tlli, rlc->tfi);</span><br><span> update_ms(new_tlli, GPRS_RLCMAC_UL_TBF);</span><br><span style="color: hsl(0, 100%, 40%);">- bts_pch_timer_stop(bts, ms_imsi(ms()));</span><br><span style="color: hsl(120, 100%, 40%);">+ bts_pch_timer_stop(bts, ms());</span><br><span> } else if (new_tlli != GSM_RESERVED_TMSI && new_tlli != tlli()) {</span><br><span> LOGPTBFUL(this, LOGL_NOTICE,</span><br><span> "Decoded TLLI=%08x mismatch on UL DATA TFI=%d. (Ignoring due to contention resolution)\n",</span><br><span>diff --git a/tests/alloc/AllocTest.cpp b/tests/alloc/AllocTest.cpp</span><br><span>index 35bbfc4..cd9c7bc 100644</span><br><span>--- a/tests/alloc/AllocTest.cpp</span><br><span>+++ b/tests/alloc/AllocTest.cpp</span><br><span>@@ -806,15 +806,17 @@</span><br><span> static void test_bts_pch_timer(void)</span><br><span> {</span><br><span> struct gprs_rlcmac_bts *bts = bts_alloc(the_pcu, 0);</span><br><span style="color: hsl(0, 100%, 40%);">- const char *imsi1 = "1234";</span><br><span style="color: hsl(0, 100%, 40%);">- const char *imsi2 = "5678";</span><br><span style="color: hsl(120, 100%, 40%);">+ struct osmo_mobile_identity mi_imsi1, mi_imsi2;</span><br><span style="color: hsl(120, 100%, 40%);">+ mi_imsi1.type = mi_imsi2.type = GSM_MI_TYPE_IMSI;</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_STRLCPY_ARRAY(mi_imsi1.imsi, "1234");</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_STRLCPY_ARRAY(mi_imsi2.imsi, "5678");</span><br><span> </span><br><span> fprintf(stderr, "Testing bts_pch_timer dealloc on bts dealloc\n");</span><br><span> log_set_category_filter(osmo_stderr_target, DPCU, 1, LOGL_DEBUG);</span><br><span> </span><br><span> fprintf(stderr, "Starting PCH timer for 2 IMSI\n");</span><br><span style="color: hsl(0, 100%, 40%);">- bts_pch_timer_start(bts, imsi1);</span><br><span style="color: hsl(0, 100%, 40%);">- bts_pch_timer_start(bts, imsi2);</span><br><span style="color: hsl(120, 100%, 40%);">+ bts_pch_timer_start(bts, &mi_imsi1, mi_imsi1.imsi);</span><br><span style="color: hsl(120, 100%, 40%);">+ bts_pch_timer_start(bts, &mi_imsi2, mi_imsi2.imsi);</span><br><span> </span><br><span> fprintf(stderr, "Deallocating BTS, expecting the PCH timer to be stopped and deallocated\n");</span><br><span> talloc_free(bts);</span><br><span>diff --git a/tests/alloc/AllocTest.err b/tests/alloc/AllocTest.err</span><br><span>index cb98332..53e2edd 100644</span><br><span>--- a/tests/alloc/AllocTest.err</span><br><span>+++ b/tests/alloc/AllocTest.err</span><br><span>@@ -501219,8 +501219,8 @@</span><br><span> DL_ASS_TBF(DL-TFI_1){NONE}: Deallocated</span><br><span> Testing bts_pch_timer dealloc on bts dealloc</span><br><span> Starting PCH timer for 2 IMSI</span><br><span style="color: hsl(0, 100%, 40%);">-PCH paging timer started for IMSI=1234</span><br><span style="color: hsl(0, 100%, 40%);">-PCH paging timer started for IMSI=5678</span><br><span style="color: hsl(120, 100%, 40%);">+PCH paging timer started for MI=IMSI-1234 IMSI=1234</span><br><span style="color: hsl(120, 100%, 40%);">+PCH paging timer started for MI=IMSI-5678 IMSI=5678</span><br><span> Deallocating BTS, expecting the PCH timer to be stopped and deallocated</span><br><span> PCH paging timer stopped for IMSI=1234</span><br><span> PCH paging timer stopped for IMSI=5678</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-pcu/+/26166">change 26166</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/osmo-pcu/+/26166"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-pcu </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: Iedffb7c6978a3faf0fc26ce2181dde9791a8b6f4 </div>
<div style="display:none"> Gerrit-Change-Number: 26166 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: merged </div>