neighbor config: allow re-using ARFCN+BSIC pairs

Fix neighbor config to match OsmoBSC manual: implement the plan for neighbor
configuration that was so far only described in the manual without actually
being in operation.

This first allows re-using ARFCN+BSIC pairs in and across BSS.

So far the handover_start() code always looked for handover target cells across
*all* local cells, even if they were not listed as neighbors to a source cell.
Imply all cells as neighbors only as long as there are no explicit neighbors
configured. As soon as the first 'neighbor' line appears in a 'bts' config,
only the listed neighbors are regarded as handover target cells. (The
'neighbor-list' commands are not related to this, only the relatively new
'neighbor (bts|lac|cgi|...)' commands affect actual handover procedures.)

TTCN3 tests TC_ho_neighbor_config_1 thru _7 play through the various aspects of
neighbor configuration: both the legacy implicit all-cells-are-neighbors as
well as allowing only explicit neighbors by config.

Related: OS#4056
Related: osmo-ttcn3-hacks Ia4ba0e75abd3d45a3422b2525e5f938cdc5a04cc
Change-Id: I29bca59ab232eddc74e0d4698efb9c9992443983

8 files changed, 240 insertions, 60 deletions

diff --git a/include/osmocom/bsc/handover.h b/include/osmocom/bsc/handover.h
index 322913da4..b00ee60f8 100644
--- a/include/osmocom/bsc/handover.h
+++ b/include/osmocom/bsc/handover.h
@@ -10,6 +10,15 @@
 #include <osmocom/bsc/neighbor_ident.h>
 #include <osmocom/bsc/gsm_data.h>
 
+#define LOG_HO(conn, level, fmt, args...) do { \
+	if (conn->ho.fi) \
+		LOGPFSML(conn->ho.fi, level, "%s: " fmt, \
+			 handover_status(conn), ## args); \
+	else \
+		LOGP(DHODEC, level, "%s: " fmt, \
+		     handover_status(conn), ## args); \
+	} while(0)
+
 struct gsm_network;
 struct gsm_lchan;
 struct gsm_bts;
@@ -25,6 +34,8 @@ enum handover_result {
 	HO_RESULT_ERROR,
 };
 
+const char *handover_status(struct gsm_subscriber_connection *conn);
+
 extern const struct value_string handover_result_names[];
 inline static const char *handover_result_name(enum handover_result val)
 { return get_value_string(handover_result_names, val); }
@@ -70,8 +81,11 @@ enum handover_result bsc_tx_bssmap_ho_complete(struct gsm_subscriber_connection
 					       struct gsm_lchan *lchan);
 void bsc_tx_bssmap_ho_failure(struct gsm_subscriber_connection *conn);
 
-struct gsm_bts *bts_by_neighbor_ident(const struct gsm_network *net,
-				      const struct neighbor_ident_key *search_for);
+int find_handover_target_cell(struct gsm_bts **local_target_cell_p,
+			      const struct gsm0808_cell_id_list2 **remote_target_cell_p,
+			      struct gsm_subscriber_connection *conn, const struct neighbor_ident_key *search_for,
+			      bool log_errors);
+
 struct neighbor_ident_key *bts_ident_key(const struct gsm_bts *bts);
 
 void handover_parse_inter_bsc_mt(struct gsm_subscriber_connection *conn,
diff --git a/include/osmocom/bsc/handover_fsm.h b/include/osmocom/bsc/handover_fsm.h
index 7c2145e84..1628d8fd9 100644
--- a/include/osmocom/bsc/handover_fsm.h
+++ b/include/osmocom/bsc/handover_fsm.h
@@ -4,18 +4,6 @@
 #include <osmocom/bsc/debug.h>
 #include <osmocom/bsc/handover.h>
 
-const char *handover_status(struct gsm_subscriber_connection *conn);
-
-/* This macro automatically includes a final \n, if omitted. */
-#define LOG_HO(conn, level, fmt, args...) do { \
-	if (conn->ho.fi) \
-		LOGPFSML(conn->ho.fi, level, "%s: " fmt, \
-			 handover_status(conn), ## args); \
-	else \
-		LOGP(DHODEC, level, "%s: " fmt, \
-		     handover_status(conn), ## args); \
-	} while(0)
-
 /* Terminology:
  * Intra-Cell: stays within one BTS, this should actually be an Assignment.
  * Intra-BSC: stays within one BSC, but moves between BTSes.
diff --git a/include/osmocom/bsc/neighbor_ident.h b/include/osmocom/bsc/neighbor_ident.h
index 17bffbc14..aa3827635 100644
--- a/include/osmocom/bsc/neighbor_ident.h
+++ b/include/osmocom/bsc/neighbor_ident.h
@@ -47,6 +47,8 @@ void neighbor_ident_iter(const struct neighbor_ident_list *nil,
 void neighbor_ident_vty_init(struct gsm_network *net, struct neighbor_ident_list *nil);
 void neighbor_ident_vty_write(struct vty *vty, const char *indent, struct gsm_bts *bts);
 
+bool neighbor_ident_bts_entry_exists(uint8_t from_bts);
+
 #define NEIGHBOR_IDENT_VTY_KEY_PARAMS "arfcn <0-1023> bsic (<0-63>|any)"
 #define NEIGHBOR_IDENT_VTY_KEY_DOC \
 	"ARFCN of neighbor cell\n" "ARFCN value\n" \
diff --git a/src/osmo-bsc/handover_decision_2.c b/src/osmo-bsc/handover_decision_2.c
index a8fff6319..0e24c0d47 100644
--- a/src/osmo-bsc/handover_decision_2.c
+++ b/src/osmo-bsc/handover_decision_2.c
@@ -900,18 +900,8 @@ static void collect_handover_candidate(struct gsm_lchan *lchan, struct neigh_mea
 		return;
 	}
 
-	neighbor_bts = bts_by_neighbor_ident(bts->network, &ni);
-
-	neighbor_cil = neighbor_ident_get(bts->network->neighbor_bss_cells, &ni);
-
-	if (neighbor_bts && neighbor_cil) {
-		LOGPHOBTS(bts, LOGL_ERROR, "Configuration error: %s exists as both local"
-			  " neighbor (bts %u) and remote-BSS neighbor (%s). Will consider only"
-			  " the local-BSS neighbor.\n",
-			  neighbor_ident_key_name(&ni),
-			  neighbor_bts->nr, gsm0808_cell_id_list_name(neighbor_cil));
-		neighbor_cil = NULL;
-	}
+	find_handover_target_cell(&neighbor_bts, &neighbor_cil,
+				  lchan->conn, &ni, false);
 
 	if (!neighbor_bts && !neighbor_cil) {
 		LOGPHOBTS(bts, LOGL_DEBUG, "no neighbor ARFCN %u BSIC %u configured for this cell\n",
diff --git a/src/osmo-bsc/handover_fsm.c b/src/osmo-bsc/handover_fsm.c
index 6d0c2d47e..d1593470b 100644
--- a/src/osmo-bsc/handover_fsm.c
+++ b/src/osmo-bsc/handover_fsm.c
@@ -31,6 +31,7 @@
 #include <osmocom/bsc/bsc_subscriber.h>
 
 #include <osmocom/bsc/handover_fsm.h>
+#include <osmocom/bsc/handover_cfg.h>
 #include <osmocom/bsc/bsc_subscr_conn_fsm.h>
 #include <osmocom/bsc/lchan_select.h>
 #include <osmocom/bsc/lchan_fsm.h>
@@ -200,6 +201,9 @@ void handover_request(struct handover_out_req *req)
 	conn = req->old_lchan->conn;
 	OSMO_ASSERT(conn && conn->fi);
 
+	/* Make sure the handover target neighbor_ident_key contains the correct source bts nr */
+	req->target_nik.from_bts = req->old_lchan->ts->trx->bts->nr;
+
 	/* To make sure we're allowed to start a handover, go through a gscon event dispatch. If that is accepted, the
 	 * same req is passed to handover_start(). */
 	osmo_fsm_inst_dispatch(conn->fi, GSCON_EV_HANDOVER_START, req);
@@ -285,9 +289,10 @@ void handover_start(struct handover_out_req *req)
 
 	OSMO_ASSERT(req && req->old_lchan && req->old_lchan->conn);
 	struct gsm_subscriber_connection *conn = req->old_lchan->conn;
+	const struct neighbor_ident_key *search_for = &req->target_nik;
 	struct handover *ho = &conn->ho;
-	struct gsm_bts *bts;
-	const struct gsm0808_cell_id_list2 *cil;
+	struct gsm_bts *local_target_cell = NULL;
+	const struct gsm0808_cell_id_list2 *remote_target_cell = NULL;
 
 	if (conn->ho.fi) {
 		LOG_HO(conn, LOGL_ERROR, "Handover requested while another handover is ongoing; Ignore\n");
@@ -295,6 +300,9 @@ void handover_start(struct handover_out_req *req)
 	}
 	handover_reset(conn);
 
+	/* When handover_start() is invoked by the gscon, it expects a HANDOVER_END event. The best way to ensure this
+	 * is to always create a handover_fsm instance, even if the target cell is not resolved yet. Any failure should
+	 * then call handover_end(), which ensures that the conn snaps back to a valid state. */
 	handover_fsm_alloc(conn);
 
 	ho->from_hodec_id = req->from_hodec_id;
@@ -302,21 +310,25 @@ void handover_start(struct handover_out_req *req)
 		req->old_lchan->type : req->new_lchan_type;
 	ho->target_cell = req->target_nik;
 
-	bts = bts_by_neighbor_ident(conn->network, &req->target_nik);
-	if (bts) {
-		ho->new_bts = bts;
+	if (find_handover_target_cell(&local_target_cell, &remote_target_cell,
+				      conn, search_for, true))
+		goto no_handover;
+
+	if (local_target_cell) {
+		ho->new_bts = local_target_cell;
 		handover_start_intra_bsc(conn);
 		return;
 	}
 
-	cil = neighbor_ident_get(conn->network->neighbor_bss_cells, &req->target_nik);
-	if (cil) {
-		handover_start_inter_bsc_out(conn, cil);
+	if (remote_target_cell) {
+		handover_start_inter_bsc_out(conn, remote_target_cell);
 		return;
 	}
 
-	LOG_HO(conn, LOGL_ERROR, "Cannot handover %s: neighbor unknown\n",
-	       neighbor_ident_key_name(&req->target_nik));
+	/* should never reach this, because find_handover_target_cell() would have returned error. */
+	OSMO_ASSERT(false);
+
+no_handover:
 	handover_end(conn, HO_RESULT_FAIL_NO_CHANNEL);
 }
 
diff --git a/src/osmo-bsc/handover_logic.c b/src/osmo-bsc/handover_logic.c
index 5725213ef..5be838340 100644
--- a/src/osmo-bsc/handover_logic.c
+++ b/src/osmo-bsc/handover_logic.c
@@ -125,42 +125,205 @@ int bts_handover_count(struct gsm_bts *bts, int ho_scopes)
 	return count;
 }
 
-struct gsm_bts *bts_by_neighbor_ident(const struct gsm_network *net,
-				      const struct neighbor_ident_key *search_for)
+/* Find out a handover target cell for the given neighbor_ident_key,
+ * and make sure there are no ambiguous matches.
+ * Given a source BTS and a target ARFCN+BSIC, find which cell is the right handover target.
+ * ARFCN+BSIC may be re-used within and/or across BSS, so make sure that only those cells that are explicitly
+ * listed as neighbor of the source cell are viable handover targets.
+ * The (legacy) default configuration is that, when no explicit neighbors are listed, that all local cells are
+ * neighbors, in which case each ARFCN+BSIC must exist at most once.
+ * If there is more than one viable handover target cell found for the given ARFCN+BSIC, that constitutes a
+ * configuration error and should not result in handover, so that the system's misconfiguration is more likely
+ * to be found.
+ */
+int find_handover_target_cell(struct gsm_bts **local_target_cell_p,
+			      const struct gsm0808_cell_id_list2 **remote_target_cell_p,
+			      struct gsm_subscriber_connection *conn, const struct neighbor_ident_key *search_for,
+			      bool log_errors)
 {
-	struct gsm_bts *found = NULL;
-	struct gsm_bts *bts;
-	struct gsm_bts *wildcard_match = NULL;
-
-	llist_for_each_entry(bts, &net->bts_list, list) {
-		struct neighbor_ident_key entry = {
-			.from_bts = NEIGHBOR_IDENT_KEY_ANY_BTS,
-			.arfcn = bts->c0->arfcn,
-			.bsic = bts->bsic,
-		};
-		if (neighbor_ident_key_match(&entry, search_for, true)) {
-			if (found) {
-				LOGP(DHO, LOGL_ERROR, "CONFIG ERROR: Multiple BTS match %s: %d and %d\n",
-				     neighbor_ident_key_name(search_for),
-				     found->nr, bts->nr);
-				return found;
+	struct gsm_network *net = conn->network;
+	struct gsm_bts *from_bts;
+	struct gsm_bts *local_target_cell = NULL;
+	const struct gsm0808_cell_id_list2 *remote_target_cell = NULL;
+	struct gsm_bts_ref *neigh;
+	bool ho_active;
+	bool as_active;
+
+	if (local_target_cell_p)
+		*local_target_cell_p = NULL;
+	if (remote_target_cell_p)
+		*remote_target_cell_p = NULL;
+
+	if (!search_for) {
+		if (log_errors)
+			LOG_HO(conn, LOGL_ERROR, "Handover without target cell\n");
+		return -EINVAL;
+	}
+
+	from_bts = gsm_bts_num(net, search_for->from_bts);
+	if (!from_bts) {
+		if (log_errors)
+			LOG_HO(conn, LOGL_ERROR, "Handover without source cell\n");
+		return -EINVAL;
+	}
+
+	ho_active = ho_get_ho_active(from_bts->ho);
+	as_active = (ho_get_algorithm(from_bts->ho) == 2)
+		&& ho_get_hodec2_as_active(from_bts->ho);
+	if (!ho_active && !as_active) {
+		if (log_errors)
+			LOG_HO(conn, LOGL_ERROR, "Cannot start Handover: Handover and Assignment disabled for this source cell (%s)\n",
+			       neighbor_ident_key_name(search_for));
+		return -EINVAL;
+	}
+
+	if (llist_empty(&from_bts->local_neighbors)
+	    && !neighbor_ident_bts_entry_exists(from_bts->nr)) {
+		/* No explicit neighbor entries exist for this BTS. Hence apply the legacy default behavior that all
+		 * local cells are neighbors. */
+		struct gsm_bts *bts;
+		struct gsm_bts *wildcard_match = NULL;
+
+		LOG_HO(conn, LOGL_DEBUG, "No explicit neighbors, regarding all local cells as neighbors\n");
+
+		llist_for_each_entry(bts, &net->bts_list, list) {
+			struct neighbor_ident_key bts_key = *bts_ident_key(bts);
+			if (neighbor_ident_key_match(&bts_key, search_for, true)) {
+				if (local_target_cell) {
+					if (log_errors)
+						LOG_HO(conn, LOGL_ERROR,
+						       "NEIGHBOR CONFIGURATION ERROR: Multiple local cells match %s"
+						       " (BTS %d and BTS %d)."
+						       " Aborting Handover because of ambiguous network topology.\n",
+						       neighbor_ident_key_name(search_for),
+						       local_target_cell->nr, bts->nr);
+					return -EINVAL;
+				}
+				local_target_cell = bts;
 			}
-			found = bts;
+			if (neighbor_ident_key_match(&bts_key, search_for, false))
+				wildcard_match = bts;
+		}
+
+		if (!local_target_cell)
+			local_target_cell = wildcard_match;
+
+		if (!local_target_cell) {
+			if (log_errors)
+				LOG_HO(conn, LOGL_ERROR, "Cannot Handover, no cell matches %s\n",
+				       neighbor_ident_key_name(search_for));
+			return -EINVAL;
+		}
+
+		if (local_target_cell == from_bts && !as_active) {
+			if (log_errors)
+				LOG_HO(conn, LOGL_ERROR,
+				       "Cannot start re-assignment, Assignment disabled for this cell (%s)\n",
+				       neighbor_ident_key_name(search_for));
+			return -EINVAL;
+		}
+		if (local_target_cell != from_bts && !ho_active) {
+			if (log_errors)
+				LOG_HO(conn, LOGL_ERROR,
+				       "Cannot start Handover, Handover disabled for this cell (%s)\n",
+				       neighbor_ident_key_name(search_for));
+			return -EINVAL;
+		}
+
+		if (local_target_cell_p)
+			*local_target_cell_p = local_target_cell;
+		return 0;
+	}
+
+	/* One or more local- or remote-BSS cell neighbors are configured. Find a match among those, but also detect
+	 * ambiguous matches (if multiple cells match, it is a configuration error). */
+
+	LOG_HO(conn, LOGL_DEBUG, "There are explicit neighbors configured for this cell\n");
+
+	/* Iterate explicit local neighbor cells */
+	llist_for_each_entry(neigh, &from_bts->local_neighbors, entry) {
+		struct gsm_bts *neigh_bts = neigh->bts;
+		struct neighbor_ident_key neigh_bts_key = *bts_ident_key(neigh_bts);
+		neigh_bts_key.from_bts = from_bts->nr;
+
+		LOG_HO(conn, LOGL_DEBUG, "Local neighbor %s\n", neighbor_ident_key_name(&neigh_bts_key));
+
+		if (!neighbor_ident_key_match(&neigh_bts_key, search_for, true)) {
+			LOG_HO(conn, LOGL_DEBUG, "Doesn't match %s\n", neighbor_ident_key_name(search_for));
+			continue;
 		}
-		if (neighbor_ident_key_match(&entry, search_for, false))
-			wildcard_match = bts;
+
+		if (local_target_cell) {
+			if (log_errors)
+				LOG_HO(conn, LOGL_ERROR,
+				       "NEIGHBOR CONFIGURATION ERROR: Multiple BTS match %s (BTS %d and BTS %d)."
+				       " Aborting Handover because of ambiguous network topology.\n",
+				       neighbor_ident_key_name(search_for), local_target_cell->nr, neigh_bts->nr);
+			return -EINVAL;
+		}
+
+		local_target_cell = neigh_bts;
+	}
+
+	/* Any matching remote-BSS neighbor cell? */
+	remote_target_cell = neighbor_ident_get(net->neighbor_bss_cells, search_for);
+
+	if (remote_target_cell)
+		LOG_HO(conn, LOGL_DEBUG, "Found remote target cell %s\n",
+		       gsm0808_cell_id_list_name(remote_target_cell));
+
+	if (local_target_cell && remote_target_cell) {
+		if (log_errors)
+			LOG_HO(conn, LOGL_ERROR, "NEIGHBOR CONFIGURATION ERROR: Both a local and a remote-BSS cell match %s"
+			       " (BTS %d and remote %s)."
+			       " Aborting Handover because of ambiguous network topology.\n",
+			       neighbor_ident_key_name(search_for), local_target_cell->nr,
+			       gsm0808_cell_id_list_name(remote_target_cell));
+		return -EINVAL;
+	}
+
+	if (local_target_cell == from_bts && !as_active) {
+		if (log_errors)
+			LOG_HO(conn, LOGL_ERROR,
+			       "Cannot start re-assignment, Assignment disabled for this cell (%s)\n",
+			       neighbor_ident_key_name(search_for));
+		return -EINVAL;
+	}
+
+	if (((local_target_cell && local_target_cell != from_bts)
+	     || remote_target_cell)
+	    && !ho_active) {
+		if (log_errors)
+			LOG_HO(conn, LOGL_ERROR,
+			       "Cannot start Handover, Handover disabled for this cell (%s)\n",
+			       neighbor_ident_key_name(search_for));
+		return -EINVAL;
+	}
+
+	if (local_target_cell) {
+		if (local_target_cell_p)
+			*local_target_cell_p = local_target_cell;
+		return 0;
+	}
+
+	if (remote_target_cell) {
+		if (remote_target_cell_p)
+			*remote_target_cell_p = remote_target_cell;
+		return 0;
 	}
 
-	if (found)
-		return found;
+	if (log_errors)
+		LOG_HO(conn, LOGL_ERROR, "Cannot handover %s: neighbor unknown\n",
+		       neighbor_ident_key_name(search_for));
 
-	return wildcard_match;
+	return -ENODEV;
 }
 
 struct neighbor_ident_key *bts_ident_key(const struct gsm_bts *bts)
 {
 	static struct neighbor_ident_key key;
 	key = (struct neighbor_ident_key){
+		.from_bts = NEIGHBOR_IDENT_KEY_ANY_BTS,
 		.arfcn = bts->c0->arfcn,
 		.bsic = bts->bsic,
 	};
diff --git a/src/osmo-bsc/neighbor_ident_vty.c b/src/osmo-bsc/neighbor_ident_vty.c
index 2b8cd7e2b..f158e7f4e 100644
--- a/src/osmo-bsc/neighbor_ident_vty.c
+++ b/src/osmo-bsc/neighbor_ident_vty.c
@@ -389,6 +389,15 @@ static bool nil_match_bts(const struct neighbor_ident_key *key,
 	return true;
 }
 
+bool neighbor_ident_bts_entry_exists(uint8_t from_bts)
+{
+	struct nil_match_bts_data d = {
+		.bts_nr = from_bts,
+	};
+	neighbor_ident_iter(g_neighbor_cells, nil_match_bts, &d);
+	return (bool)d.found;
+}
+
 static int neighbor_del_all(struct vty *vty)
 {
 	int rc;
diff --git a/tests/bsc/bsc_test.c b/tests/bsc/bsc_test.c
index 492f0c5a1..103e0bbb8 100644
--- a/tests/bsc/bsc_test.c
+++ b/tests/bsc/bsc_test.c
@@ -250,3 +250,5 @@ void gscon_submit_rsl_dtap(struct gsm_subscriber_connection *conn,
 			   struct msgb *msg, int link_id, int allow_sacch) {}
 void ts_fsm_alloc(struct gsm_bts_trx_ts *ts) {}
 void lchan_activate(struct gsm_lchan *lchan, void *info) {}
+bool neighbor_ident_bts_entry_exists(uint8_t from_bts) { return false; }
+const char *handover_status(struct gsm_subscriber_connection *conn) { return "x"; }