From: Holger Hans Peter Freyther holger@moiji-mobile.com

The dispatching might lead to the removal of more paging requests and makes "request" invalid. Remove it before calling the callback. --- openbsc/src/libbsc/paging.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/openbsc/src/libbsc/paging.c b/openbsc/src/libbsc/paging.c index 2d08af7..8bc10e6 100644 --- a/openbsc/src/libbsc/paging.c +++ b/openbsc/src/libbsc/paging.c @@ -366,13 +366,19 @@ static void _paging_request_stop(struct gsm_bts *bts, struct gsm_subscriber *sub llist_for_each_entry_safe(req, req2, &bts_entry->pending_requests, entry) { if (req->subscr == subscr) { - if (conn && req->cbfn) { + gsm_cbfn *cbfn = req->cbfn; + void *param = req->cbfn_param; + + /* now give up the data structure */ + paging_remove_request(&bts->paging, req); + req = NULL; + + if (conn && cbfn) { LOGP(DPAG, LOGL_DEBUG, "Stop paging on bts %d, calling cbfn.\n", bts->nr); - req->cbfn(GSM_HOOK_RR_PAGING, GSM_PAGING_SUCCEEDED, - msg, conn, req->cbfn_param); + cbfn(GSM_HOOK_RR_PAGING, GSM_PAGING_SUCCEEDED, + msg, conn, param); } else LOGP(DPAG, LOGL_DEBUG, "Stop paging on bts %d silently.\n", bts->nr); - paging_remove_request(&bts->paging, req); break; } }

From: Holger Hans Peter Freyther holger@moiji-mobile.com

The idea of "subscriber_get_channel" was that different requests would be coordinated. At the same time we have seen that the "queue" can get stuck at both 31C3 and the rhizomatica installations.

Voice calls and SMS do not need coordination. We should be able to send SMS on a voice channel and switch the MS from a SDCCH to a TCH in case we establish a voice call. The SMS code itself needs to coordinate to obey the limit of one SMS per direction but this should be enforced in the sms layer and not on the subscriber.

Modify the code to have a simple paging coordination. The subscriber code will schedule the paging and register who would like to know about success/failure.

This allowed to greatly simplify the paging response handling for the transaction code (and in fact we could move the transaction list into the subscriber structure now). The code gained to support to cancel the notification of a request (but not the paging itself yet).

TODO: Cancel paging request in case no one cares about it anymore. --- openbsc/include/openbsc/gsm_data.h | 1 - openbsc/include/openbsc/gsm_subscriber.h | 14 ++- openbsc/include/openbsc/transaction.h | 2 +- openbsc/src/libmsc/gsm_04_08.c | 95 +++++++----------- openbsc/src/libmsc/gsm_04_11.c | 8 +- openbsc/src/libmsc/gsm_subscriber.c | 154 ++++++++---------------------- openbsc/src/libmsc/osmo_msc.c | 8 -- openbsc/src/libmsc/transaction.c | 7 +- openbsc/src/libmsc/vty_interface_layer3.c | 2 + openbsc/tests/channel/channel_test.c | 12 ++- 10 files changed, 102 insertions(+), 201 deletions(-)

diff --git a/openbsc/include/openbsc/gsm_data.h b/openbsc/include/openbsc/gsm_data.h index 51d68d2..30a397c 100644 --- a/openbsc/include/openbsc/gsm_data.h +++ b/openbsc/include/openbsc/gsm_data.h @@ -119,7 +119,6 @@ struct gsm_subscriber_connection {

/* Are we part of a special "silent" call */ int silent_call; - int put_channel;

/* bsc structures */ struct osmo_bsc_sccp_con *sccp_con; diff --git a/openbsc/include/openbsc/gsm_subscriber.h b/openbsc/include/openbsc/gsm_subscriber.h index c5585ce..7d6c776 100644 --- a/openbsc/include/openbsc/gsm_subscriber.h +++ b/openbsc/include/openbsc/gsm_subscriber.h @@ -22,6 +22,8 @@ struct vty; struct sgsn_mm_ctx; struct sgsn_subscriber_data;

+struct subscr_request; + struct gsm_subscriber_group { struct gsm_network *net;

@@ -66,7 +68,7 @@ struct gsm_subscriber { struct llist_head entry;

/* pending requests */ - int in_callback; + int is_paging; struct llist_head requests;

/* GPRS/SGSN related fields */ @@ -101,9 +103,6 @@ struct gsm_subscriber *subscr_get_by_id(struct gsm_subscriber_group *sgrp, struct gsm_subscriber *subscr_get_or_create(struct gsm_subscriber_group *sgrp, const char *imsi); int subscr_update(struct gsm_subscriber *s, struct gsm_bts *bts, int reason); -void subscr_put_channel(struct gsm_subscriber *subscr); -void subscr_get_channel(struct gsm_subscriber *subscr, - int type, gsm_cbfn *cbfn, void *param); struct gsm_subscriber *subscr_active_by_tmsi(struct gsm_subscriber_group *sgrp, uint32_t tmsi); struct gsm_subscriber *subscr_active_by_imsi(struct gsm_subscriber_group *sgrp, @@ -116,6 +115,13 @@ void subscr_update_from_db(struct gsm_subscriber *subscr); void subscr_expire(struct gsm_subscriber_group *sgrp); int subscr_update_expire_lu(struct gsm_subscriber *subscr, struct gsm_bts *bts);

+/* + * Paging handling with authentication + */ +struct subscr_request *subscr_request_channel(struct gsm_subscriber *subscr, + int type, gsm_cbfn *cbfn, void *param); +void subscr_remove_request(struct subscr_request *req); + /* internal */ struct gsm_subscriber *subscr_alloc(void); extern struct llist_head active_subscribers; diff --git a/openbsc/include/openbsc/transaction.h b/openbsc/include/openbsc/transaction.h index 82891b8..6ef1612 100644 --- a/openbsc/include/openbsc/transaction.h +++ b/openbsc/include/openbsc/transaction.h @@ -36,7 +36,7 @@ struct gsm_trans { int tch_recv;

/* is thats one paging? */ - struct gsm_network **paging_request; + struct subscr_request *paging_request;

union { struct { diff --git a/openbsc/src/libmsc/gsm_04_08.c b/openbsc/src/libmsc/gsm_04_08.c index 941090c..5609602 100644 --- a/openbsc/src/libmsc/gsm_04_08.c +++ b/openbsc/src/libmsc/gsm_04_08.c @@ -1384,68 +1384,42 @@ static int gsm48_cc_tx_setup(struct gsm_trans *trans, void *arg);

/* call-back from paging the B-end of the connection */ static int setup_trig_pag_evt(unsigned int hooknum, unsigned int event, - struct msgb *msg, void *_conn, void *param) + struct msgb *msg, void *_conn, void *_transt) { - int found = 0; struct gsm_subscriber_connection *conn = _conn; - struct gsm_network **paging_request = param, *net; - struct gsm_trans *transt, *tmp; + struct gsm_trans *transt = _transt;

- if (hooknum != GSM_HOOK_RR_PAGING) - return -EINVAL; - - net = *paging_request; - if (!net) { - DEBUGP(DCC, "Error Network not set!\n"); - return -EINVAL; - } + OSMO_ASSERT(!transt->conn); + OSMO_ASSERT(conn);

/* check all tranactions (without lchan) for subscriber */ - llist_for_each_entry_safe(transt, tmp, &net->trans_list, entry) { - if (transt->paging_request != paging_request || transt->conn) - continue; - switch (event) { - case GSM_PAGING_SUCCEEDED: - if (!conn) // paranoid - break; - DEBUGP(DCC, "Paging subscr %s succeeded!\n", - transt->subscr->extension); - found = 1; - /* Assign lchan */ - if (!transt->conn) { - transt->paging_request = NULL; - transt->conn = conn; - conn->put_channel = 1; - } - /* send SETUP request to called party */ - gsm48_cc_tx_setup(transt, &transt->cc.msg); - break; - case GSM_PAGING_EXPIRED: - case GSM_PAGING_BUSY: - DEBUGP(DCC, "Paging subscr %s expired!\n", - transt->subscr->extension); - /* Temporarily out of order */ - found = 1; - mncc_release_ind(transt->net, transt, - transt->callref, - GSM48_CAUSE_LOC_PRN_S_LU, - GSM48_CC_CAUSE_DEST_OOO); - transt->callref = 0; - transt->paging_request = NULL; - trans_free(transt); - break; - } + switch (event) { + case GSM_PAGING_SUCCEEDED: + DEBUGP(DCC, "Paging subscr %s succeeded!\n", transt->subscr->extension); + /* Assign lchan */ + transt->conn = conn; + /* send SETUP request to called party */ + gsm48_cc_tx_setup(transt, &transt->cc.msg); + break; + case GSM_PAGING_EXPIRED: + case GSM_PAGING_BUSY: + DEBUGP(DCC, "Paging subscr %s expired!\n", + transt->subscr->extension); + /* Temporarily out of order */ + mncc_release_ind(transt->net, transt, + transt->callref, + GSM48_CAUSE_LOC_PRN_S_LU, + GSM48_CC_CAUSE_DEST_OOO); + transt->callref = 0; + transt->paging_request = NULL; + trans_free(transt); + break; + default: + LOGP(DCC, LOGL_ERROR, "Unknown paging event %d\n", event); + break; }

- talloc_free(paging_request); - - /* - * FIXME: The queue needs to be kicked. This is likely to go through a RF - * failure and then the subscr will be poke again. This needs a lot of fixing - * in the subscriber queue code. - */ - if (!found && conn) - conn->put_channel = 1; + transt->paging_request = NULL; return 0; }

@@ -3124,19 +3098,16 @@ int mncc_tx_to_cc(struct gsm_network *net, int msg_type, void *arg) /* store setup informations until paging was successfull */ memcpy(&trans->cc.msg, data, sizeof(struct gsm_mncc));

- /* Get a channel */ - trans->paging_request = talloc_zero(subscr->group->net, - struct gsm_network*); + /* Request a channel */ + trans->paging_request = subscr_request_channel(subscr, + RSL_CHANNEED_TCH_F, setup_trig_pag_evt, + trans); if (!trans->paging_request) { LOGP(DCC, LOGL_ERROR, "Failed to allocate paging token.\n"); subscr_put(subscr); trans_free(trans); return 0; } - - *trans->paging_request = subscr->group->net; - subscr_get_channel(subscr, RSL_CHANNEED_TCH_F, setup_trig_pag_evt, trans->paging_request); - subscr_put(subscr); return 0; } diff --git a/openbsc/src/libmsc/gsm_04_11.c b/openbsc/src/libmsc/gsm_04_11.c index bcc25e4..1b2a42c 100644 --- a/openbsc/src/libmsc/gsm_04_11.c +++ b/openbsc/src/libmsc/gsm_04_11.c @@ -938,6 +938,7 @@ int gsm411_send_sms_subscr(struct gsm_subscriber *subscr, struct gsm_sms *sms) { struct gsm_subscriber_connection *conn; + void *res;

/* check if we already have an open lchan to the subscriber. * if yes, send the SMS this way */ @@ -947,7 +948,12 @@ int gsm411_send_sms_subscr(struct gsm_subscriber *subscr, }

/* if not, we have to start paging */ - subscr_get_channel(subscr, RSL_CHANNEED_SDCCH, paging_cb_send_sms, sms); + res = subscr_request_channel(subscr, RSL_CHANNEED_SDCCH, + paging_cb_send_sms, sms); + if (!res) { + send_signal(S_SMS_UNKNOWN_ERROR, NULL, sms, GSM_PAGING_BUSY); + sms_free(sms); + } return 0; }

diff --git a/openbsc/src/libmsc/gsm_subscriber.c b/openbsc/src/libmsc/gsm_subscriber.c index 9037e89..4559de5 100644 --- a/openbsc/src/libmsc/gsm_subscriber.c +++ b/openbsc/src/libmsc/gsm_subscriber.c @@ -56,28 +56,11 @@ int gsm48_secure_channel(struct gsm_subscriber_connection *conn, int key_seq, struct subscr_request { struct llist_head entry;

- /* back reference */ - struct gsm_subscriber *subscr; - - /* the requested channel type */ - int channel_type; - - /* what did we do */ - int state; - /* the callback data */ gsm_cbfn *cbfn; void *param; };

-enum { - REQ_STATE_INITIAL, - REQ_STATE_QUEUED, - REQ_STATE_PAGED, - REQ_STATE_FAILED_START, - REQ_STATE_DISPATCHED, -}; - static struct gsm_subscriber *get_subscriber(struct gsm_subscriber_group *sgrp, int type, const char *ident) { @@ -94,16 +77,14 @@ static struct gsm_subscriber *get_subscriber(struct gsm_subscriber_group *sgrp, static int subscr_paging_dispatch(unsigned int hooknum, unsigned int event, struct msgb *msg, void *data, void *param) { - struct subscr_request *request; + struct subscr_request *request, *tmp; struct gsm_subscriber_connection *conn = data; struct gsm_subscriber *subscr = param; struct paging_signal_data sig_data;

- /* There is no request anymore... */ - if (llist_empty(&subscr->requests)) - return -1; + OSMO_ASSERT(subscr->is_paging);

- /* Dispatch signal */ + /* Inform parts of the system we don't know */ sig_data.subscr = subscr; sig_data.bts = conn ? conn->bts : NULL; sig_data.conn = conn; @@ -116,34 +97,22 @@ static int subscr_paging_dispatch(unsigned int hooknum, unsigned int event, );

/* - * FIXME: What to do with paging requests coming during - * this callback? We must be sure to not start paging when - * we have an active connection to a subscriber and to make - * the subscr_put_channel work as required... + * Stop paging on all other BTS. E.g. if this is + * the first timeout on a BTS then the others will + * timeout soon as well. Let's just stop everything + * and forget we wanted to page. */ - request = (struct subscr_request *)subscr->requests.next; - request->state = REQ_STATE_DISPATCHED; - llist_del(&request->entry); - subscr->in_callback = 1; - request->cbfn(hooknum, event, msg, data, request->param); - subscr->in_callback = 0; + paging_request_stop(NULL, subscr, NULL, NULL); + subscr->is_paging = 0;

- if (event != GSM_PAGING_SUCCEEDED) { - /* - * This is a workaround for a bigger issue. We have - * issued paging that might involve multiple BTSes - * and one of them have failed now. We will stop the - * other paging requests as well as the next timeout - * would work on the next paging request and the queue - * will do bad things. This should be fixed by counting - * the outstanding results. - */ - paging_request_stop(NULL, subscr, NULL, NULL); - subscr_put_channel(subscr); + llist_for_each_entry_safe(request, tmp, &subscr->requests, entry) { + llist_del(&request->entry); + request->cbfn(hooknum, event, msg, data, request->param); + talloc_free(request); }

+ /* balanced with the moment we start paging */ subscr_put(subscr); - talloc_free(request); return 0; }

@@ -194,86 +163,43 @@ static int subscr_paging_cb(unsigned int hooknum, unsigned int event, return gsm48_secure_channel(conn, pr->key_seq, subscr_paging_sec_cb, param); }

- -static void subscr_send_paging_request(struct gsm_subscriber *subscr) +struct subscr_request *subscr_request_channel(struct gsm_subscriber *subscr, + int channel_type, gsm_cbfn *cbfn, void *param) { - struct subscr_request *request; int rc; - struct gsm_network *net = subscr->group->net; - - assert(!llist_empty(&subscr->requests)); - - request = (struct subscr_request *)subscr->requests.next; - request->state = REQ_STATE_PAGED; - rc = paging_request(net, subscr, request->channel_type, - subscr_paging_cb, subscr); - - /* paging failed, quit now */ - if (rc <= 0) { - request->state = REQ_STATE_FAILED_START; - subscr_paging_cb(GSM_HOOK_RR_PAGING, GSM_PAGING_BUSY, - NULL, NULL, subscr); - } -} - -void subscr_get_channel(struct gsm_subscriber *subscr, - int type, gsm_cbfn *cbfn, void *param) -{ struct subscr_request *request;

- request = talloc(tall_sub_req_ctx, struct subscr_request); - if (!request) { - if (cbfn) - cbfn(GSM_HOOK_RR_PAGING, GSM_PAGING_OOM, - NULL, NULL, param); - return; + /* Start paging.. we know it is async so we can do it before */ + if (!subscr->is_paging) { + LOGP(DMM, LOGL_DEBUG, "Subscriber %s not paged yet.\n", + subscr_name(subscr)); + rc = paging_request(subscr->group->net, subscr, channel_type, + subscr_paging_cb, subscr); + if (rc <= 0) { + LOGP(DMM, LOGL_ERROR, "Subscriber %s paging failed: %d\n", + subscr_name(subscr), rc); + return NULL; + } + /* reduced on the first paging callback */ + subscr_get(subscr); + subscr->is_paging = 1; }

- memset(request, 0, sizeof(*request)); - request->subscr = subscr_get(subscr); - request->channel_type = type; + /* TODO: Stop paging in case of memory allocation failure */ + request = talloc_zero(subscr, struct subscr_request); + if (!request) + return NULL; + request->cbfn = cbfn; request->param = param; - request->state = REQ_STATE_INITIAL; - - /* - * FIXME: We might be able to assign more than one - * channel, e.g. voice and SMS submit at the same - * time. - */ - if (!subscr->in_callback && llist_empty(&subscr->requests)) { - /* add to the list, send a request */ - llist_add_tail(&request->entry, &subscr->requests); - subscr_send_paging_request(subscr); - } else { - /* this will be picked up later, from subscr_put_channel */ - llist_add_tail(&request->entry, &subscr->requests); - request->state = REQ_STATE_QUEUED; - } + llist_add_tail(&request->entry, &subscr->requests); + return request; }

-void subscr_put_channel(struct gsm_subscriber *subscr) +void subscr_remove_request(struct subscr_request *request) { - /* - * FIXME: Continue with other requests now... by checking - * the gsm_subscriber inside the gsm_lchan. Drop the ref count - * of the lchan after having asked the next requestee to handle - * the channel. - */ - /* - * FIXME: is the lchan is of a different type we could still - * issue an immediate assignment for another channel and then - * close this one. - */ - /* - * Currently we will drop the last ref of the lchan which - * will result in a channel release on RSL and we will start - * the paging. This should work most of the time as the MS - * will listen to the paging requests before we timeout - */ - - if (subscr && !llist_empty(&subscr->requests)) - subscr_send_paging_request(subscr); + llist_del(&request->entry); + talloc_free(request); }

struct gsm_subscriber *subscr_create_subscriber(struct gsm_subscriber_group *sgrp, diff --git a/openbsc/src/libmsc/osmo_msc.c b/openbsc/src/libmsc/osmo_msc.c index f3badb7..b4facff 100644 --- a/openbsc/src/libmsc/osmo_msc.c +++ b/openbsc/src/libmsc/osmo_msc.c @@ -39,10 +39,6 @@ static void msc_sapi_n_reject(struct gsm_subscriber_connection *conn, int dlci) static int msc_clear_request(struct gsm_subscriber_connection *conn, uint32_t cause) { gsm0408_clear_request(conn, cause); - if (conn->put_channel) { - conn->put_channel = 0; - subscr_put_channel(conn->subscr); - } return 1; }

@@ -173,9 +169,5 @@ void msc_release_connection(struct gsm_subscriber_connection *conn)

conn->in_release = 1; gsm0808_clear(conn); - if (conn->put_channel) { - conn->put_channel = 0; - subscr_put_channel(conn->subscr); - } subscr_con_free(conn); } diff --git a/openbsc/src/libmsc/transaction.c b/openbsc/src/libmsc/transaction.c index 2101ae9..a750362 100644 --- a/openbsc/src/libmsc/transaction.c +++ b/openbsc/src/libmsc/transaction.c @@ -98,11 +98,8 @@ void trans_free(struct gsm_trans *trans) break; }

- /* FIXME: implement a sane way to stop this. */ - if (!trans->conn && trans->paging_request) { - LOGP(DNM, LOGL_ERROR, - "Transaction freed while paging for sub: %llu\n", - trans->subscr->id); + if (trans->paging_request) { + subscr_remove_request(trans->paging_request); trans->paging_request = NULL; }

diff --git a/openbsc/src/libmsc/vty_interface_layer3.c b/openbsc/src/libmsc/vty_interface_layer3.c index 04e65e7..6cf51a3 100644 --- a/openbsc/src/libmsc/vty_interface_layer3.c +++ b/openbsc/src/libmsc/vty_interface_layer3.c @@ -107,6 +107,8 @@ static void subscr_dump_full_vty(struct vty *vty, struct gsm_subscriber *subscr) "%a, %d %b %Y %T %z", localtime(&subscr->expire_lu)); expire_time[sizeof(expire_time) - 1] = '\0'; vty_out(vty, " Expiration Time: %s%s", expire_time, VTY_NEWLINE); + vty_out(vty, " Paging: %s paging%s", + subscr->is_paging ? "is" : "not", VTY_NEWLINE); vty_out(vty, " Use count: %u%s", subscr->use_count, VTY_NEWLINE); }

diff --git a/openbsc/tests/channel/channel_test.c b/openbsc/tests/channel/channel_test.c index 2369339..cd6a1fd 100644 --- a/openbsc/tests/channel/channel_test.c +++ b/openbsc/tests/channel/channel_test.c @@ -31,6 +31,8 @@

static int s_end = 0; static struct gsm_subscriber_connection s_conn; +static void *s_data; +static gsm_cbfn *s_cbfn;

/* our handler */ static int subscr_cb(unsigned int hook, unsigned int event, struct msgb *msg, void *data, void *param) @@ -48,7 +50,8 @@ static int subscr_cb(unsigned int hook, unsigned int event, struct msgb *msg, vo /* mock object for testing, directly invoke the cb... maybe later through the timer */ int paging_request(struct gsm_bts *bts, struct gsm_subscriber *subscriber, int type, gsm_cbfn *cbfn, void *data) { - cbfn(101, 200, (void*)0x1323L, &s_conn, data); + s_data = data; + s_cbfn = cbfn;

/* claim we have patched */ return 1; @@ -80,11 +83,10 @@ int main(int argc, char **argv) OSMO_ASSERT(subscr->group->net == network);

/* Ask for a channel... */ - subscr_get_channel(subscr, RSL_CHANNEED_TCH_F, subscr_cb, (void*)0x2342L); + subscr_request_channel(subscr, RSL_CHANNEED_TCH_F, subscr_cb, (void*)0x2342L); + s_cbfn(101, 200, (void*)0x1323L, &s_conn, s_data);

- while (!s_end) { - osmo_select_main(0); - } + OSMO_ASSERT(s_end);

return EXIT_SUCCESS; }