sub: Remove the queue from the subscriber code

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.

10 files changed, 102 insertions, 201 deletions

diff --git a/openbsc/include/openbsc/gsm_data.h b/openbsc/include/openbsc/gsm_data.h
index 51d68d23b..30a397cb9 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 c5585ce3d..7d6c776bc 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 82891b8b3..6ef1612a9 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 941090c8d..5609602a7 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 bcc25e4c2..1b2a42c4f 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 9037e89d5..4559de581 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 f3badb779..b4facff8c 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 2101ae96c..a75036253 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 04e65e758..6cf51a33f 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 236933949..cd6a1fd37 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;
 }