Here's a small series with some minor fixes I found when looking at multiple SMS in a single RR connection.
For the MO case, it actually now works as described in the spec in the no-error case. But what can happen is that after the second CM SERVICE REQUEST, we may never receive the last CP-ACK of the previous transaction (and thus leak a transaction). The spec says than when receiving a CP-DATA after a CM SERVICE REQUEST we should consider we also implicitely received the last CP-ACK of the previous transaction ... that's not implemented yet.
For the MT case, it currently makes several RR connection which is quite bad ... to be fixed.
Sylvain
From: Sylvain Munaut tnt@246tNt.com
The exact sequence the states the BTS goes through is slightly different for one of the nanoBTS 139 I have and it needs this to start.
Signed-off-by: Sylvain Munaut tnt@246tNt.com --- openbsc/src/bsc_init.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/openbsc/src/bsc_init.c b/openbsc/src/bsc_init.c index ce3d0b4..b72624a 100644 --- a/openbsc/src/bsc_init.c +++ b/openbsc/src/bsc_init.c @@ -356,8 +356,10 @@ int nm_state_event(enum nm_evt evt, u_int8_t obj_class, void *obj, switch (obj_class) { case NM_OC_SITE_MANAGER: bts = container_of(obj, struct gsm_bts, site_mgr); - if (new_state->operational == 2 && - new_state->availability == NM_AVSTATE_OK) + if ((new_state->operational == 2 && + new_state->availability == NM_AVSTATE_OK) || + (new_state->operational == 1 && + new_state->availability == NM_AVSTATE_OFF_LINE)) abis_nm_opstart(bts, obj_class, 0xff, 0xff, 0xff); break; case NM_OC_BTS:
From: Sylvain Munaut tnt@246tNt.com
Signed-off-by: Sylvain Munaut tnt@246tNt.com --- openbsc/src/gsm_04_11.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/openbsc/src/gsm_04_11.c b/openbsc/src/gsm_04_11.c index 4930e5c..d256f7e 100644 --- a/openbsc/src/gsm_04_11.c +++ b/openbsc/src/gsm_04_11.c @@ -173,7 +173,7 @@ static int gsm411_cp_sendmsg(struct msgb *msg, struct gsm_trans *trans, DEBUGP(DSMS, "TX: CP-ACK "); break; case GSM411_MT_CP_ERROR: - DEBUGP(DSMS, "TX: CP-ACK "); + DEBUGP(DSMS, "TX: CP-ERROR "); break; }
@@ -958,7 +958,7 @@ int gsm0411_rcv_sms(struct msgb *msg, u_int8_t link_id) bsc_del_timer(&trans->sms.cp_timer);
if (!trans->sms.is_mt) { - /* FIXME: we have sont one CP-DATA, which was now + /* FIXME: we have sent one CP-DATA, which was now * acknowledged. Check if we want to transfer more, * i.e. multi-part message */ trans->sms.cp_state = GSM411_CPS_IDLE;
From: Sylvain Munaut tnt@246tNt.com
In SMS debug messages, we always display the transaction ID as if we were 'sending' the message.
Signed-off-by: Sylvain Munaut tnt@246tNt.com --- openbsc/src/gsm_04_11.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/openbsc/src/gsm_04_11.c b/openbsc/src/gsm_04_11.c index d256f7e..84139e2 100644 --- a/openbsc/src/gsm_04_11.c +++ b/openbsc/src/gsm_04_11.c @@ -906,7 +906,7 @@ int gsm0411_rcv_sms(struct msgb *msg, u_int8_t link_id) return -EIO; /* FIXME: send some error message */
- DEBUGP(DSMS, "trans_id=%x ", gh->proto_discr >> 4); + DEBUGP(DSMS, "trans_id=%x ", transaction_id); trans = trans_find_by_id(lchan->subscr, GSM48_PDISC_SMS, transaction_id); if (!trans) {
From: Sylvain Munaut tnt@246tNt.com
'unknown' has a negative connotation for a case that's totally normal so refer to it as 'new' so it doesn't sound like a problem.
Signed-off-by: Sylvain Munaut tnt@246tNt.com --- openbsc/src/gsm_04_11.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/openbsc/src/gsm_04_11.c b/openbsc/src/gsm_04_11.c index 84139e2..31fcabf 100644 --- a/openbsc/src/gsm_04_11.c +++ b/openbsc/src/gsm_04_11.c @@ -910,7 +910,7 @@ int gsm0411_rcv_sms(struct msgb *msg, u_int8_t link_id) trans = trans_find_by_id(lchan->subscr, GSM48_PDISC_SMS, transaction_id); if (!trans) { - DEBUGPC(DSMS, "(unknown) "); + DEBUGPC(DSMS, "(new) "); trans = trans_alloc(lchan->subscr, GSM48_PDISC_SMS, transaction_id, new_callref++); if (!trans) {
From: Sylvain Munaut tnt@246tNt.com
Multiple CM SERVICE REQUEST can happen on a single RR connection, in this case, since the subscr reference is tracked through lchan->subscr and will only be put'd once on lchan_free, we need to make sure we don't get several reference ....
Signed-off-by: Sylvain Munaut tnt@246tNt.com --- openbsc/src/gsm_04_08.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/openbsc/src/gsm_04_08.c b/openbsc/src/gsm_04_08.c index 5fea6bf..f87b335 100644 --- a/openbsc/src/gsm_04_08.c +++ b/openbsc/src/gsm_04_08.c @@ -1343,7 +1343,9 @@ static int gsm48_rx_mm_serv_req(struct msgb *msg)
if (!msg->lchan->subscr) msg->lchan->subscr = subscr; - else if (msg->lchan->subscr != subscr) { + else if (msg->lchan->subscr == subscr) + subscr_put(subscr); /* lchan already has a ref, don't need another one */ + else { DEBUGP(DMM, "<- CM Channel already owned by someone else?\n"); subscr_put(subscr); }
From: Sylvain Munaut tnt@246tNt.com
According to GSM 04.07 11.2.3.1.3 , TID 7 is "reserved for future extensions".
Signed-off-by: Sylvain Munaut tnt@246tNt.com --- openbsc/src/transaction.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/openbsc/src/transaction.c b/openbsc/src/transaction.c index 9f1bbf3..1d58333 100644 --- a/openbsc/src/transaction.c +++ b/openbsc/src/transaction.c @@ -133,7 +133,7 @@ int trans_assign_trans_id(struct gsm_subscriber *subscr, used_tid_bitmask |= (1 << trans->transaction_id); }
- for (i = 0; i <= 7; i++) { + for (i = 0; i < 7; i++) { if ((used_tid_bitmask & (1 << (i | ti_flag))) == 0) return i | ti_flag; }
From: Sylvain Munaut tnt@246tNt.com
Signed-off-by: Sylvain Munaut tnt@246tNt.com --- openbsc/src/gsm_04_11.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/openbsc/src/gsm_04_11.c b/openbsc/src/gsm_04_11.c index 31fcabf..9e85f9b 100644 --- a/openbsc/src/gsm_04_11.c +++ b/openbsc/src/gsm_04_11.c @@ -1005,7 +1005,11 @@ int gsm411_send_sms_lchan(struct gsm_lchan *lchan, struct gsm_sms *sms) u_int8_t transaction_id; int rc;
- transaction_id = 4; /* FIXME: we always use 4 for now */ + transaction_id = trans_assign_trans_id(lchan->subscr, GSM48_PDISC_SMS, 0); + if (transaction_id == -1) { + DEBUGP(DSMS, "No available transaction ids\n"); + return -EBUSY; + }
msg->lchan = lchan;
thanks, applied.
On Friday 18 December 2009 18:28:05 Sylvain Munaut wrote:
Here's a small series with some minor fixes I found when looking at multiple SMS in a single RR connection.
For the MO case, it actually now works as described in the spec in the no-error case. But what can happen is that after the second CM SERVICE REQUEST, we may never receive the last CP-ACK of the previous transaction (and thus leak a transaction). The spec says than when receiving a CP-DATA after a CM SERVICE REQUEST we should consider we also implicitely received the last CP-ACK of the previous transaction ... that's not implemented yet.
For the MT case, it currently makes several RR connection which is quite bad ... to be fixed.
Great,
I have applied 2-6, for 1 and 7 I would like to let Harald review it (they seem fine but I can't judge the impacts).
z.
-
Harald Welte -
Holger Freyther -
Sylvain Munaut