Change in osmo-bsc[master]: lchan and assignment FSMs: make Channel Mode Modify more sane

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

neels gerrit-no-reply at lists.osmocom.org
Sat May 22 02:39:26 UTC 2021


neels has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/23943 )

Change subject: lchan and assignment FSMs: make Channel Mode Modify more sane
......................................................................

lchan and assignment FSMs: make Channel Mode Modify more sane

The Channel Mode Modify procedure is currently implemented for changing
a TCH lchan from signalling to voice mode. For that, however, it is
re-using (abusing) the channel activation structs and state transitions,
and thus always implies activating a voice stream when the mode
modification is done.

I will add a Channel Mode Modify to enable VAMOS mode soon, so I require
separate structs and state transitions which also work on an lchan that
already has a voice stream established: a struct lchan_modify_info and
LCHAN_EV_REQUEST_MODE_MODIFY, and dedicated assignment FSM state
ASSIGNMENT_ST_WAIT_LCHAN_MODIFIED.

For the part where a Channel Mode Modify enables a voice stream after
switching from signalling to speech mode, still use the channel
activation code path, but only once the mode modification is done.

General improvements:
- To ask for a mode modification, emit an FSM event that ensures a mode
  modify only happens when the lchan state allows it.
- The new lchan_modify_info struct reflects only those parts that have
  an effect during a mode modification (before the lchan_activate_info
  was fully populated, many values not having an effect).
- More accurate logging, indicating "Mode Modify" instead of "Channel
  Activation"

A TTCN3 test for the Channel Mode Modify procedure is added in
Idf4efaed986de0bbd2b663313e837352cc139f0f, and the test passes both
before and after this patch is applied.

Related: SYS#4895
Change-Id: I4986844f839b1c9672c61d916eb3d33d0042d747
---
M doc/assignment-fsm.dot
M include/osmocom/bsc/assignment_fsm.h
M include/osmocom/bsc/gsm_data.h
M include/osmocom/bsc/lchan_fsm.h
M src/osmo-bsc/assignment_fsm.c
M src/osmo-bsc/gsm_data.c
M src/osmo-bsc/lchan_fsm.c
7 files changed, 224 insertions(+), 70 deletions(-)

Approvals:
  neels: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/doc/assignment-fsm.dot b/doc/assignment-fsm.dot
index c218153..4eb8d98 100644
--- a/doc/assignment-fsm.dot
+++ b/doc/assignment-fsm.dot
@@ -12,6 +12,7 @@
 	gscon2 [label="conn FSM",shape=box3d]
 	lchan [label="lchan FSM\n(new lchan)",shape=box3d]
 	old_lchan [label="old lchan",shape=box3d]
+	lchan2 [label="lchan FSM",shape=box3d]
 
 	bssap [label="osmo_bsc_bssap.c",shape=box]
 
@@ -22,8 +23,7 @@
 	bssap -> gscon [label="GSCON_EV_ASSIGNMENT_START\ndata=struct assignment_request",style=dotted]
 
 	gscon -> WAIT_LCHAN_ACTIVE [label="assignment_fsm_start()",style=dotted]
-	gscon -> WAIT_LCHAN_ESTABLISHED [label="assignment_fsm_start()\n(mode modify)",style=dotted]
-        WAIT_LCHAN_ACTIVE -> lchan [label="lchan_activate()\nFOR_ASSIGNMENT",style=dotted]
+	WAIT_LCHAN_ACTIVE -> lchan [label="lchan_activate()\nFOR_ASSIGNMENT",style=dotted]
 	lchan -> WAIT_LCHAN_ACTIVE [label="ASSIGNMENT_EV_\nLCHAN_\nACTIVE,ERROR",style=dotted]
 	lchan -> WAIT_LCHAN_ESTABLISHED [label="ASSIGNMENT_EV_\nLCHAN_\nESTABLISHED,ERROR",style=dotted]
 
@@ -40,4 +40,10 @@
 	WAIT_MGW_ENDPOINT_TO_MSC -> gscon2 [label="gscon_connect_\nmgw_to_msc()",style=dotted]
 	gscon2 -> WAIT_MGW_ENDPOINT_TO_MSC [label="ASSIGNMENT_EV_\nMSC_MGW_OK",style=dotted]
 	terminate -> gscon2 [label="GSCON_EV_\nASSIGNMENT_END",style=dotted]
+
+	WAIT_LCHAN_ACTIVE -> WAIT_LCHAN_MODIFIED [label="assignment_fsm_start()\n(mode modify)"]
+	WAIT_LCHAN_MODIFIED -> lchan2 [label="lchan_mode_modify()\nMODIFY_FOR_ASSIGNMENT",style=dotted]
+	lchan2 -> WAIT_LCHAN_MODIFIED [label="ASSIGNMENT_EV_\nLCHAN_\nMODIFIED,ERROR",style=dotted]
+	WAIT_LCHAN_MODIFIED -> WAIT_MGW_ENDPOINT_TO_MSC [label="needs\nvoice\nstream"]
+	WAIT_LCHAN_MODIFIED -> terminate [label="no change\nin voice\nstream"]
 }
diff --git a/include/osmocom/bsc/assignment_fsm.h b/include/osmocom/bsc/assignment_fsm.h
index 156da42..b4af335 100644
--- a/include/osmocom/bsc/assignment_fsm.h
+++ b/include/osmocom/bsc/assignment_fsm.h
@@ -24,11 +24,13 @@
 	ASSIGNMENT_ST_WAIT_RR_ASS_COMPLETE,
 	ASSIGNMENT_ST_WAIT_LCHAN_ESTABLISHED,
 	ASSIGNMENT_ST_WAIT_MGW_ENDPOINT_TO_MSC,
+	ASSIGNMENT_ST_WAIT_LCHAN_MODIFIED,
 };
 
 enum assignment_fsm_event {
 	ASSIGNMENT_EV_LCHAN_ACTIVE,
 	ASSIGNMENT_EV_LCHAN_ESTABLISHED,
+	ASSIGNMENT_EV_LCHAN_MODIFIED,
 	ASSIGNMENT_EV_LCHAN_ERROR,
 	ASSIGNMENT_EV_MSC_MGW_OK,
 	ASSIGNMENT_EV_MSC_MGW_FAIL,
diff --git a/include/osmocom/bsc/gsm_data.h b/include/osmocom/bsc/gsm_data.h
index bd83ea0..5a202bf 100644
--- a/include/osmocom/bsc/gsm_data.h
+++ b/include/osmocom/bsc/gsm_data.h
@@ -564,6 +564,7 @@
 	ACTIVATE_FOR_ASSIGNMENT,
 	ACTIVATE_FOR_HANDOVER,
 	ACTIVATE_FOR_VTY,
+	ACTIVATE_FOR_MODE_MODIFY_RTP,
 };
 
 extern const struct value_string lchan_activate_mode_names[];
@@ -589,6 +590,25 @@
 	uint8_t ta;
 };
 
+enum lchan_modify_for {
+	MODIFY_FOR_NONE,
+	MODIFY_FOR_ASSIGNMENT,
+	MODIFY_FOR_VTY,
+};
+
+extern const struct value_string lchan_modify_for_names[];
+static inline const char *lchan_modify_for_name(enum lchan_modify_for modify_for)
+{ return get_value_string(lchan_modify_for_names, modify_for); }
+
+struct lchan_modify_info {
+	enum lchan_modify_for modify_for;
+	enum gsm48_chan_mode chan_mode;
+	bool requires_voice_stream;
+	uint16_t msc_assigned_cic;
+	/* AMR config */
+	uint16_t s15_s0;
+};
+
 struct gsm_lchan {
 	/* The TS that we're part of */
 	struct gsm_bts_trx_ts *ts;
@@ -614,6 +634,11 @@
 	} activate;
 
 	struct {
+		struct lchan_modify_info info;
+		bool concluded;
+	} modify;
+
+	struct {
 		/* If an event to release the lchan comes in while still waiting for responses, just mark this
 		 * flag, so that the lchan will gracefully release at the next sensible junction. */
 		bool requested;
diff --git a/include/osmocom/bsc/lchan_fsm.h b/include/osmocom/bsc/lchan_fsm.h
index 99964a4..74e2a96 100644
--- a/include/osmocom/bsc/lchan_fsm.h
+++ b/include/osmocom/bsc/lchan_fsm.h
@@ -65,6 +65,7 @@
 
 void lchan_activate(struct gsm_lchan *lchan, struct lchan_activate_info *info);
 void lchan_ready_to_switch_rtp(struct gsm_lchan *lchan);
+void lchan_mode_modify(struct gsm_lchan *lchan, struct lchan_modify_info *info);
 
 static inline const char *lchan_state_name(struct gsm_lchan *lchan)
 {
diff --git a/src/osmo-bsc/assignment_fsm.c b/src/osmo-bsc/assignment_fsm.c
index 3a17396..228cb1f 100644
--- a/src/osmo-bsc/assignment_fsm.c
+++ b/src/osmo-bsc/assignment_fsm.c
@@ -241,20 +241,23 @@
 static void assignment_success(struct gsm_subscriber_connection *conn)
 {
 	struct gsm_bts *bts;
+	bool lchan_changed = (conn->assignment.new_lchan != NULL);
 
-	/* Take on the new lchan */
-	gscon_change_primary_lchan(conn, conn->assignment.new_lchan);
-	conn->assignment.new_lchan = NULL;
+	/* Take on the new lchan. If there only was a Channel Mode Modify, then there is no new lchan to take on. */
+	if (lchan_changed) {
+		gscon_change_primary_lchan(conn, conn->assignment.new_lchan);
 
-	OSMO_ASSERT((bts = conn_get_bts(conn)) != NULL);
-	if (is_siemens_bts(bts) && ts_is_tch(conn->lchan->ts)) {
-		/* HACK: store the actual Classmark 2 LV from the subscriber and use it here! */
-		uint8_t cm2_lv[] = { 0x02, 0x00, 0x00 };
-		send_siemens_mrpci(conn->lchan, cm2_lv);
+		OSMO_ASSERT((bts = conn_get_bts(conn)) != NULL);
+		if (is_siemens_bts(bts) && ts_is_tch(conn->lchan->ts)) {
+			/* HACK: store the actual Classmark 2 LV from the subscriber and use it here! */
+			uint8_t cm2_lv[] = { 0x02, 0x00, 0x00 };
+			send_siemens_mrpci(conn->lchan, cm2_lv);
+		}
+
+		/* apply LCLS configuration (if any) */
+		lcls_apply_config(conn);
 	}
-
-	/* apply LCLS configuration (if any) */
-	lcls_apply_config(conn);
+	conn->assignment.new_lchan = NULL;
 
 	send_assignment_complete(conn);
 	/* If something went wrong during send_assignment_complete(), the fi will be gone from
@@ -267,15 +270,17 @@
 		return;
 	}
 
-	/* Rembered this only for error handling: should assignment fail, assignment_reset() will release
-	 * the MGW endpoint right away. If successful, the conn continues to use the endpoint. */
-	conn->assignment.created_ci_for_msc = NULL;
+	if (lchan_changed) {
+		/* Rembered this only for error handling: should assignment fail, assignment_reset() will release
+		 * the MGW endpoint right away. If successful, the conn continues to use the endpoint. */
+		conn->assignment.created_ci_for_msc = NULL;
 
-	/* New RTP information is now accepted */
-	conn->user_plane.msc_assigned_cic = conn->assignment.req.msc_assigned_cic;
-	osmo_strlcpy(conn->user_plane.msc_assigned_rtp_addr, conn->assignment.req.msc_rtp_addr,
-		     sizeof(conn->user_plane.msc_assigned_rtp_addr));
-	conn->user_plane.msc_assigned_rtp_port = conn->assignment.req.msc_rtp_port;
+		/* New RTP information is now accepted */
+		conn->user_plane.msc_assigned_cic = conn->assignment.req.msc_assigned_cic;
+		osmo_strlcpy(conn->user_plane.msc_assigned_rtp_addr, conn->assignment.req.msc_rtp_addr,
+			     sizeof(conn->user_plane.msc_assigned_rtp_addr));
+		conn->user_plane.msc_assigned_rtp_port = conn->assignment.req.msc_rtp_port;
+	}
 
 	LOG_ASSIGNMENT(conn, LOGL_DEBUG, "Assignment successful\n");
 	osmo_fsm_inst_term(conn->assignment.fi, OSMO_FSM_TERM_REGULAR, 0);
@@ -285,7 +290,9 @@
 
 static void assignment_fsm_update_id(struct gsm_subscriber_connection *conn)
 {
-	struct gsm_lchan *new_lchan = conn->assignment.new_lchan;
+	/* Assignment can do a new channel activation, in which case new_lchan points at the new lchan.
+	 * Or assignment can Channel Mode Modify the already used lchan, in which case new_lchan == NULL. */
+	struct gsm_lchan *new_lchan = (conn->assignment.new_lchan ? : conn->lchan);
 	if (!new_lchan) {
 		osmo_fsm_inst_update_id(conn->assignment.fi, conn->fi->id);
 		return;
@@ -435,7 +442,8 @@
 		[CH_RATE_FULL] = "FR",
 	};
 	struct osmo_fsm_inst *fi;
-	struct lchan_activate_info info;
+	struct lchan_activate_info activ_info;
+	struct lchan_modify_info modif_info;
 	int i;
 
 	OSMO_ASSERT(conn);
@@ -465,6 +473,8 @@
 	 * LCHAN_EV_REQUEST_MODE_MODIFY in lchan_fsm.c. To not break the lchan, do not even attempt to re-use an lchan
 	 * that already has an RTP stream set up, rather establish a new lchan (that transition is well implemented). */
 	if (reuse_existing_lchan(conn) && !conn->lchan->fi_rtp) {
+		/* The new lchan is the old lchan, keep new_lchan == NULL. */
+		conn->assignment.new_lchan = NULL;
 
 		/* If the requested mode and the current TCH mode matches up, just send the
 		 * assignment complete directly and be done with the assignment procedure. */
@@ -494,28 +504,17 @@
 			       gsm48_chan_mode_name(conn->lchan->ch_mode_rate.chan_mode),
 			       gsm_lchan_name(conn->lchan));
 
-		info = (struct lchan_activate_info){
-			.activ_for = ACTIVATE_FOR_ASSIGNMENT,
-			.for_conn = conn,
+		modif_info = (struct lchan_modify_info){
+			.modify_for = MODIFY_FOR_ASSIGNMENT,
 			.chan_mode = conn->lchan->ch_mode_rate.chan_mode,
-			.encr = conn->lchan->encr,
-			.s15_s0 = conn->lchan->ch_mode_rate.s15_s0,
 			.requires_voice_stream = conn->assignment.requires_voice_stream,
+			.s15_s0 = conn->lchan->ch_mode_rate.s15_s0,
 			.msc_assigned_cic = req->msc_assigned_cic,
-			.re_use_mgw_endpoint_from_lchan = conn->lchan,
-			.ta = conn->lchan->last_ta,
-			.ta_known = true,
 		};
 
-		osmo_fsm_inst_dispatch(conn->lchan->fi, LCHAN_EV_REQUEST_MODE_MODIFY, &info);
-
-		/* Since we opted not to allocate a new lchan, the new lchan is still the old lchan. */
-		conn->assignment.new_lchan = conn->lchan;
-
-		/* Also we need to skip the RR assignment, so we jump forward and wait for the lchan_fsm until it
-		 * reaches the established state again. */
-		assignment_fsm_state_chg(ASSIGNMENT_ST_WAIT_LCHAN_ESTABLISHED);
-
+		if (assignment_fsm_state_chg(ASSIGNMENT_ST_WAIT_LCHAN_MODIFIED))
+			return;
+		lchan_mode_modify(conn->lchan, &modif_info);
 		return;
 	}
 
@@ -571,7 +570,7 @@
 		       req->use_osmux ? "yes" : "no");
 
 	assignment_fsm_state_chg(ASSIGNMENT_ST_WAIT_LCHAN_ACTIVE);
-	info = (struct lchan_activate_info){
+	activ_info = (struct lchan_activate_info){
 		.activ_for = ACTIVATE_FOR_ASSIGNMENT,
 		.for_conn = conn,
 		.chan_mode = conn->lchan->ch_mode_rate.chan_mode,
@@ -583,7 +582,7 @@
 		.ta = conn->lchan->last_ta,
 		.ta_known = true,
 	};
-	lchan_activate(conn->assignment.new_lchan, &info);
+	lchan_activate(conn->assignment.new_lchan, &activ_info);
 }
 
 static void assignment_fsm_wait_lchan(struct osmo_fsm_inst *fi, uint32_t event, void *data)
@@ -695,8 +694,10 @@
 		       conn->assignment.req.msc_rtp_addr,
 		       conn->assignment.req.msc_rtp_port);
 
+	/* Assignment can do a new channel activation, in which case new_lchan points at the new lchan.
+	 * Or assignment can Channel Mode Modify the already used lchan, in which case new_lchan == NULL. */
 	if (!gscon_connect_mgw_to_msc(conn,
-				      conn->assignment.new_lchan,
+				      conn->assignment.new_lchan ? : conn->lchan,
 				      conn->assignment.req.msc_rtp_addr,
 				      conn->assignment.req.msc_rtp_port,
 				      fi,
@@ -742,6 +743,19 @@
 	}
 }
 
+static void assignment_fsm_wait_lchan_modified(struct osmo_fsm_inst *fi, uint32_t event, void *data)
+{
+	switch (event) {
+
+	case ASSIGNMENT_EV_LCHAN_MODIFIED:
+		assignment_fsm_post_lchan_established(fi);
+		return;
+
+	default:
+		OSMO_ASSERT(false);
+	}
+}
+
 #define S(x)	(1 << (x))
 
 static const struct osmo_fsm_state assignment_fsm_states[] = {
@@ -754,7 +768,7 @@
 		.out_state_mask = 0
 			| S(ASSIGNMENT_ST_WAIT_LCHAN_ACTIVE)
 			| S(ASSIGNMENT_ST_WAIT_RR_ASS_COMPLETE)
-			| S(ASSIGNMENT_ST_WAIT_LCHAN_ESTABLISHED) /* MODE MODIFY */
+			| S(ASSIGNMENT_ST_WAIT_LCHAN_MODIFIED)
 			,
 	},
 	[ASSIGNMENT_ST_WAIT_RR_ASS_COMPLETE] = {
@@ -790,11 +804,22 @@
 			| S(ASSIGNMENT_EV_MSC_MGW_FAIL)
 			,
 	},
+	[ASSIGNMENT_ST_WAIT_LCHAN_MODIFIED] = {
+		.name = "WAIT_LCHAN_MODIFIED",
+		.action = assignment_fsm_wait_lchan_modified,
+		.in_event_mask = 0
+			| S(ASSIGNMENT_EV_LCHAN_MODIFIED)
+			,
+		.out_state_mask = 0
+			| S(ASSIGNMENT_ST_WAIT_MGW_ENDPOINT_TO_MSC)
+			,
+	},
 };
 
 static const struct value_string assignment_fsm_event_names[] = {
 	OSMO_VALUE_STRING(ASSIGNMENT_EV_LCHAN_ACTIVE),
 	OSMO_VALUE_STRING(ASSIGNMENT_EV_LCHAN_ESTABLISHED),
+	OSMO_VALUE_STRING(ASSIGNMENT_EV_LCHAN_MODIFIED),
 	OSMO_VALUE_STRING(ASSIGNMENT_EV_LCHAN_ERROR),
 	OSMO_VALUE_STRING(ASSIGNMENT_EV_MSC_MGW_OK),
 	OSMO_VALUE_STRING(ASSIGNMENT_EV_MSC_MGW_FAIL),
@@ -807,6 +832,11 @@
 void assignment_fsm_allstate_action(struct osmo_fsm_inst *fi, uint32_t event, void *data)
 {
 	struct gsm_subscriber_connection *conn = assignment_fi_conn(fi);
+
+	/* Assignment can do a new channel activation, in which case new_lchan points at the new lchan.
+	 * Or assignment can Channel Mode Modify the already used lchan, in which case new_lchan == NULL. */
+	struct gsm_lchan *new_lchan = conn->assignment.new_lchan ? : conn->lchan;
+
 	switch (event) {
 
 	case ASSIGNMENT_EV_CONN_RELEASING:
@@ -815,10 +845,11 @@
 		return;
 
 	case ASSIGNMENT_EV_LCHAN_ERROR:
-		if (data != conn->assignment.new_lchan)
+		if (data != new_lchan)
 			return;
-		assignment_fail(conn->assignment.new_lchan->activate.gsm0808_error_cause,
-				"Failed to activate lchan %s",
+		assignment_fail(new_lchan->activate.gsm0808_error_cause,
+				"Failed to %s lchan %s",
+				conn->assignment.new_lchan ? "activate" : "modify",
 				gsm_lchan_name(conn->assignment.new_lchan));
 		return;
 
diff --git a/src/osmo-bsc/gsm_data.c b/src/osmo-bsc/gsm_data.c
index d5a9d6b..6c08229 100644
--- a/src/osmo-bsc/gsm_data.c
+++ b/src/osmo-bsc/gsm_data.c
@@ -916,6 +916,13 @@
 	{}
 };
 
+const struct value_string lchan_modify_for_names[] = {
+	OSMO_VALUE_STRING(MODIFY_FOR_NONE),
+	OSMO_VALUE_STRING(MODIFY_FOR_ASSIGNMENT),
+	OSMO_VALUE_STRING(MODIFY_FOR_VTY),
+	{}
+};
+
 /* This may be specific to RR Channel Release, and the mappings were chosen by pure naive guessing without a proper
  * specification available. */
 enum gsm48_rr_cause bsc_gsm48_rr_cause_from_gsm0808_cause(enum gsm0808_cause c)
diff --git a/src/osmo-bsc/lchan_fsm.c b/src/osmo-bsc/lchan_fsm.c
index 83524e0..1735346 100644
--- a/src/osmo-bsc/lchan_fsm.c
+++ b/src/osmo-bsc/lchan_fsm.c
@@ -66,6 +66,53 @@
 	}
 }
 
+static void lchan_on_mode_modify_success(struct gsm_lchan *lchan)
+{
+	lchan->modify.concluded = true;
+
+	switch (lchan->modify.info.modify_for) {
+
+	case MODIFY_FOR_ASSIGNMENT:
+		osmo_fsm_inst_dispatch(lchan->conn->assignment.fi, ASSIGNMENT_EV_LCHAN_MODIFIED, lchan);
+		break;
+
+	default:
+		break;
+	}
+}
+
+#define lchan_on_mode_modify_failure(lchan, modify_for, for_conn) \
+	_lchan_on_mode_modify_failure(lchan, modify_for, for_conn, \
+				     __FILE__, __LINE__)
+static void _lchan_on_mode_modify_failure(struct gsm_lchan *lchan, enum lchan_modify_for modify_for,
+					  struct gsm_subscriber_connection *for_conn,
+					  const char *file, int line)
+{
+	if (lchan->modify.concluded)
+		return;
+	lchan->modify.concluded = true;
+
+	switch (modify_for) {
+
+	case MODIFY_FOR_ASSIGNMENT:
+		LOG_LCHAN(lchan, LOGL_NOTICE, "Signalling Assignment FSM of error (%s)\n",
+			  lchan->last_error ? : "unknown error");
+		_osmo_fsm_inst_dispatch(for_conn->assignment.fi, ASSIGNMENT_EV_LCHAN_ERROR, lchan,
+					file, line);
+		return;
+
+	case MODIFY_FOR_VTY:
+		LOG_LCHAN(lchan, LOGL_ERROR, "VTY user invoked lchan Channel Mode Modify failed (%s)\n",
+			  lchan->last_error ? : "unknown error");
+		break;
+
+	default:
+		LOG_LCHAN(lchan, LOGL_ERROR, "lchan Channel Mode Modify failed (%s)\n",
+			  lchan->last_error ? : "unknown error");
+		break;
+	}
+}
+
 /* The idea here is that we must not require to change any lchan state in order to deny a request. */
 #define lchan_on_activation_failure(lchan, for_conn, activ_for) \
 	_lchan_on_activation_failure(lchan, for_conn, activ_for, \
@@ -122,6 +169,10 @@
 			  lchan->last_error ? : "unknown error");
 		break;
 
+	case ACTIVATE_FOR_MODE_MODIFY_RTP:
+		lchan_on_mode_modify_failure(lchan, lchan->modify.info.modify_for, for_conn);
+		break;
+
 	default:
 		LOG_LCHAN(lchan, LOGL_ERROR, "lchan activation failed (%s)\n",
 			  lchan->last_error ? : "unknown error");
@@ -183,6 +234,10 @@
 		 * we will try to roll back a modified RTP connection. */
 		break;
 
+	case ACTIVATE_FOR_MODE_MODIFY_RTP:
+		lchan_on_mode_modify_success(lchan);
+		break;
+
 	default:
 		LOG_LCHAN(lchan, LOGL_NOTICE, "lchan %s fully established\n",
 			  lchan_activate_mode_name(lchan->activate.info.activ_for));
@@ -323,6 +378,18 @@
 	/* Remain in state UNUSED */
 }
 
+void lchan_mode_modify(struct gsm_lchan *lchan, struct lchan_modify_info *info)
+{
+	OSMO_ASSERT(lchan && info);
+
+	/* To make sure that the lchan is actually allowed to initiate Mode Modify, feed through an FSM event. */
+	if (osmo_fsm_inst_dispatch(lchan->fi, LCHAN_EV_REQUEST_MODE_MODIFY, info)) {
+		LOG_LCHAN(lchan, LOGL_ERROR,
+			  "Channel Mode Modify requested, but cannot dispatch LCHAN_EV_REQUEST_MODE_MODIFY event\n");
+		lchan_on_mode_modify_failure(lchan, info->modify_for, lchan->conn);
+	}
+}
+
 static void lchan_fsm_update_id(struct gsm_lchan *lchan)
 {
 	osmo_fsm_inst_update_id_f(lchan->fi, "%u-%u-%u-%s-%u",
@@ -881,7 +948,7 @@
 static void lchan_fsm_wait_rr_chan_mode_modify_ack_onenter(struct osmo_fsm_inst *fi, uint32_t prev_state)
 {
 	struct gsm_lchan *lchan = lchan_fi_lchan(fi);
-	gsm48_lchan_modify(lchan, lchan->activate.info.chan_mode);
+	gsm48_lchan_modify(lchan, lchan->modify.info.chan_mode);
 }
 
 static void lchan_fsm_wait_rr_chan_mode_modify_ack(struct osmo_fsm_inst *fi, uint32_t event, void *data)
@@ -921,10 +988,27 @@
 	switch (event) {
 
 	case LCHAN_EV_RSL_CHAN_MODE_MODIFY_ACK:
-		if (lchan->activate.info.requires_voice_stream)
+		/* The Channel Mode Modify was ACKed, now the requested values become the accepted and used values. */
+		lchan->tch_mode = lchan->modify.info.chan_mode;
+		lchan->s15_s0 = lchan->modify.info.s15_s0;
+
+		if (lchan->modify.info.requires_voice_stream
+		    && !lchan->fi_rtp) {
+			/* Continue with RTP stream establishing as done in lchan_activate(). Place the requested values in
+			 * lchan->activate.info and continue with voice stream setup. */
+			lchan->activate.info = (struct lchan_activate_info){
+				.activ_for = ACTIVATE_FOR_MODE_MODIFY_RTP,
+				.for_conn = lchan->conn,
+				.s15_s0 = lchan->modify.info.s15_s0,
+				.requires_voice_stream = true,
+				.msc_assigned_cic = lchan->modify.info.msc_assigned_cic,
+			};
+			lchan->activate.concluded = false;
 			lchan_fsm_state_chg(LCHAN_ST_WAIT_RLL_RTP_ESTABLISH);
-		else
+		} else {
 			lchan_fsm_state_chg(LCHAN_ST_ESTABLISHED);
+			lchan_on_mode_modify_success(lchan);
+		}
 		return;
 
 	case LCHAN_EV_RSL_CHAN_MODE_MODIFY_NACK:
@@ -1014,7 +1098,7 @@
 static void lchan_fsm_established(struct osmo_fsm_inst *fi, uint32_t event, void *data)
 {
 	struct gsm_lchan *lchan = lchan_fi_lchan(fi);
-	struct lchan_activate_info *info;
+	struct lchan_modify_info *modif_info;
 	struct osmo_mgcpc_ep_ci *use_mgwep_ci;
 
 	switch (event) {
@@ -1052,35 +1136,29 @@
 			return;
 		}
 
-		info = data;
-		lchan->activate.info = *info;
+		modif_info = data;
+		lchan->modify.info = *modif_info;
+		lchan->modify.concluded = false;
+
 		use_mgwep_ci = lchan_use_mgw_endpoint_ci_bts(lchan);
 
-		if (info->chan_mode == GSM48_CMODE_SPEECH_AMR) {
-			if (lchan_mr_config(lchan, info->s15_s0) < 0) {
+		if (modif_info->chan_mode == GSM48_CMODE_SPEECH_AMR) {
+			if (lchan_mr_config(lchan, modif_info->s15_s0) < 0) {
 				lchan_fail("Can not generate multirate configuration IE\n");
 				return;
 			}
 		}
 
 		LOG_LCHAN(lchan, LOGL_INFO,
-			  "Modification requested: %s voice=%s MGW-ci=%s type=%s tch-mode=%s encr-alg=A5/%u ck=%s\n",
-			  lchan_activate_mode_name(lchan->activate.info.activ_for),
-			  lchan->activate.info.requires_voice_stream ? "yes" : "no",
-			  lchan->activate.info.requires_voice_stream ?
+			  "Modification requested: %s voice=%s MGW-ci=%s type=%s tch-mode=%s\n",
+			  lchan_modify_for_name(lchan->modify.info.modify_for),
+			  lchan->modify.info.requires_voice_stream ? "yes" : "no",
+			  lchan->modify.info.requires_voice_stream ?
 			  (use_mgwep_ci ? osmo_mgcpc_ep_ci_name(use_mgwep_ci) : "new")
 			  : "none",
 			  gsm_lchant_name(lchan->type),
-			  gsm48_chan_mode_name(lchan->tch_mode),
-			  (lchan->activate.info.encr.alg_id ? : 1) - 1,
-			  lchan->activate.info.encr.key_len ? osmo_hexdump_nospc(lchan->activate.info.encr.key,
-										 lchan->activate.info.encr.key_len) : "none");
+			  gsm48_chan_mode_name(lchan->tch_mode));
 
-		/* While the mode is changed the lchan is virtually "not activated", at least
-		 * from the FSM implementations perspective */
-		lchan->activate.concluded = false;
-
-		/* Initiate mode modification, start with the MS side (RR) */
 		lchan_fsm_state_chg(LCHAN_ST_WAIT_RR_CHAN_MODE_MODIFY_ACK);
 		return;
 
@@ -1389,8 +1467,9 @@
 			| S(LCHAN_EV_RR_CHAN_MODE_MODIFY_ERROR)
 			,
 		.out_state_mask = 0
-			| S(LCHAN_ST_BORKEN)
 			| S(LCHAN_ST_WAIT_RSL_CHAN_MODE_MODIFY_ACK)
+			| S(LCHAN_ST_WAIT_RF_RELEASE_ACK)
+			| S(LCHAN_ST_BORKEN)
 			,
 	},
 	[LCHAN_ST_WAIT_RSL_CHAN_MODE_MODIFY_ACK] = {
@@ -1402,8 +1481,10 @@
 			| S(LCHAN_EV_RSL_CHAN_MODE_MODIFY_NACK)
 			,
 		.out_state_mask = 0
-			| S(LCHAN_ST_BORKEN)
+			| S(LCHAN_ST_ESTABLISHED)
 			| S(LCHAN_ST_WAIT_RLL_RTP_ESTABLISH)
+			| S(LCHAN_ST_WAIT_RF_RELEASE_ACK)
+			| S(LCHAN_ST_BORKEN)
 			,
 	},
 	[LCHAN_ST_ESTABLISHED] = {
@@ -1514,6 +1595,7 @@
 	OSMO_VALUE_STRING(LCHAN_EV_RR_CHAN_MODE_MODIFY_ERROR),
 	OSMO_VALUE_STRING(LCHAN_EV_RSL_CHAN_MODE_MODIFY_ACK),
 	OSMO_VALUE_STRING(LCHAN_EV_RSL_CHAN_MODE_MODIFY_NACK),
+	OSMO_VALUE_STRING(LCHAN_EV_REQUEST_MODE_MODIFY),
 	{}
 };
 

-- 
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/23943
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I4986844f839b1c9672c61d916eb3d33d0042d747
Gerrit-Change-Number: 23943
Gerrit-PatchSet: 5
Gerrit-Owner: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy at sysmocom.de>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210522/3ef82c37/attachment.htm>


More information about the gerrit-log mailing list