[MERGED] osmocom-bb[master]: mobile: Instead of putting semantic in a comment, use an enum

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/.

Holger Freyther gerrit-no-reply at lists.osmocom.org
Tue Nov 28 06:26:03 UTC 2017


Holger Freyther has submitted this change and it was merged.

Change subject: mobile: Instead of putting semantic in a comment, use an enum
......................................................................


mobile: Instead of putting semantic in a comment, use an enum

The enum was created to understand the different states during
the shutdown and find places where it is used. The normal
transitions are like.

	Idle -> Imsi Detach -> L1 Reset -> Done
	Idle -> L1 Reset -> Done

The shutdown can get stuck in case:

* Out of memory situation while handling IMSI detach (timeout)
* Never receiving l1 reset acknnowledgment.

The code could benefit from the move to osmo fsm to deal with
proper timeouts.

Change-Id: Iee1140e4848923c7270495c381bf87b7e3fddee1
---
M src/host/layer23/include/osmocom/bb/common/osmocom_data.h
M src/host/layer23/src/mobile/app_mobile.c
M src/host/layer23/src/mobile/gsm411_sms.c
M src/host/layer23/src/mobile/gsm480_ss.c
M src/host/layer23/src/mobile/gsm48_cc.c
M src/host/layer23/src/mobile/gsm48_mm.c
M src/host/layer23/src/mobile/vty_interface.c
7 files changed, 34 insertions(+), 27 deletions(-)

Approvals:
  Harald Welte: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/src/host/layer23/include/osmocom/bb/common/osmocom_data.h b/src/host/layer23/include/osmocom/bb/common/osmocom_data.h
index 21b2880..7a935f9 100644
--- a/src/host/layer23/include/osmocom/bb/common/osmocom_data.h
+++ b/src/host/layer23/include/osmocom/bb/common/osmocom_data.h
@@ -54,6 +54,13 @@
 	int16_t s, rl_fail;
 };
 
+enum {
+	MS_SHUTDOWN_NONE = 0,
+	MS_SHUTDOWN_IMSI_DETACH = 1,
+	MS_SHUTDOWN_WAIT_RESET = 2,
+	MS_SHUTDOWN_COMPL = 3,
+};
+
 /* One Mobilestation for osmocom */
 struct osmocom_ms {
 	struct llist_head entity;
diff --git a/src/host/layer23/src/mobile/app_mobile.c b/src/host/layer23/src/mobile/app_mobile.c
index 1905010..d28af00 100644
--- a/src/host/layer23/src/mobile/app_mobile.c
+++ b/src/host/layer23/src/mobile/app_mobile.c
@@ -96,9 +96,9 @@
 		set = &ms->settings;
 
 		/* waiting for reset after shutdown */
-		if (ms->shutdown == 2) {
+		if (ms->shutdown == MS_SHUTDOWN_WAIT_RESET) {
 			LOGP(DMOB, LOGL_NOTICE, "MS '%s' has been resetted\n", ms->name);
-			ms->shutdown = 3;
+			ms->shutdown = MS_SHUTDOWN_COMPL;
 			break;
 		}
 
@@ -142,13 +142,13 @@
 	struct gsm48_mmlayer *mm = &ms->mmlayer;
 
 	/* if shutdown is already performed */
-	if (ms->shutdown >= 2)
+	if (ms->shutdown >= MS_SHUTDOWN_WAIT_RESET)
 		return 0;
 
 	if (!force && ms->started) {
 		struct msgb *nmsg;
 
-		ms->shutdown = 1; /* going down */
+		ms->shutdown = MS_SHUTDOWN_IMSI_DETACH;
 		nmsg = gsm48_mmevent_msgb_alloc(GSM48_MM_EVENT_IMSI_DETACH);
 		if (!nmsg)
 			return -ENOMEM;
@@ -168,10 +168,10 @@
 	lapdm_channel_exit(&ms->lapdm_channel);
 
 	if (ms->started) {
-		ms->shutdown = 2; /* being down, wait for reset */
+		ms->shutdown = MS_SHUTDOWN_WAIT_RESET; /* being down, wait for reset */
 		l1ctl_tx_reset_req(ms, L1CTL_RES_T_FULL);
 	} else {
-		ms->shutdown = 3; /* being down */
+		ms->shutdown = MS_SHUTDOWN_COMPL; /* being down */
 	}
 	vty_notify(ms, NULL);
 	vty_notify(ms, "Power off!\n");
@@ -230,7 +230,7 @@
 
 	gsm_random_imei(&ms->settings);
 
-	ms->shutdown = 0;
+	ms->shutdown = MS_SHUTDOWN_NONE;
 	ms->started = false;
 
 	if (!strcmp(ms->settings.imei, "000000000000000")) {
@@ -268,7 +268,7 @@
 	gsm_support_init(ms);
 	gsm_settings_init(ms);
 
-	ms->shutdown = 3; /* being down */
+	ms->shutdown = MS_SHUTDOWN_COMPL;
 
 	if (mncc_recv_app) {
 		mncc_name = talloc_asprintf(ms, "/tmp/ms_mncc_%s", ms->name);
@@ -298,7 +298,7 @@
 		ms->mncc_entity.sock_state = NULL;
 	}
 
-	if (ms->shutdown == 0 || (ms->shutdown == 1 && force)) {
+	if (ms->shutdown == MS_SHUTDOWN_NONE || (ms->shutdown == MS_SHUTDOWN_IMSI_DETACH && force)) {
 		rc = mobile_exit(ms, force);
 		if (rc < 0)
 			return rc;
@@ -339,9 +339,9 @@
 	int work = 0;
 
 	llist_for_each_entry_safe(ms, ms2, &ms_list, entity) {
-		if (ms->shutdown != 3)
+		if (ms->shutdown != MS_SHUTDOWN_COMPL)
 			work |= mobile_work(ms);
-		if (ms->shutdown == 3) {
+		if (ms->shutdown == MS_SHUTDOWN_COMPL) {
 			if (ms->l2_wq.bfd.fd > -1) {
 				layer2_close(ms);
 				ms->l2_wq.bfd.fd = -1;
diff --git a/src/host/layer23/src/mobile/gsm411_sms.c b/src/host/layer23/src/mobile/gsm411_sms.c
index 22db859..1b10262 100644
--- a/src/host/layer23/src/mobile/gsm411_sms.c
+++ b/src/host/layer23/src/mobile/gsm411_sms.c
@@ -633,7 +633,7 @@
 	LOGP(DLSMS, LOGL_INFO, "..._sms_submit()\n");
 
 	/* no running, no transaction */
-	if (!ms->started || ms->shutdown) {
+	if (!ms->started || ms->shutdown != MS_SHUTDOWN_COMPL) {
 		LOGP(DLSMS, LOGL_ERROR, "Phone is down\n");
 		gsm411_sms_report(ms, sms, GSM411_RP_CAUSE_MO_TEMP_FAIL);
 		sms_free(sms);
diff --git a/src/host/layer23/src/mobile/gsm480_ss.c b/src/host/layer23/src/mobile/gsm480_ss.c
index ff90faa..ee2c943 100644
--- a/src/host/layer23/src/mobile/gsm480_ss.c
+++ b/src/host/layer23/src/mobile/gsm480_ss.c
@@ -603,7 +603,7 @@
 	}
 
 	/* no running, no transaction */
-	if (!ms->started || ms->shutdown) {
+	if (!ms->started || ms->shutdown != MS_SHUTDOWN_NONE) {
 		gsm480_ss_result(ms, "<phone is down>", 0);
 		return -EIO;
 	}
diff --git a/src/host/layer23/src/mobile/gsm48_cc.c b/src/host/layer23/src/mobile/gsm48_cc.c
index d398c76..f1e8109 100644
--- a/src/host/layer23/src/mobile/gsm48_cc.c
+++ b/src/host/layer23/src/mobile/gsm48_cc.c
@@ -1923,7 +1923,7 @@
 	struct gsm_trans *trans;
 	int i, rc;
 
-	if (!ms->started || ms->shutdown) {
+	if (!ms->started || ms->shutdown != MS_SHUTDOWN_NONE) {
 		LOGP(DCC, LOGL_NOTICE, "Phone is down!\n");
 		if (ms->mncc_entity.mncc_recv && msg_type != MNCC_REL_REQ) {
 			struct gsm_mncc rel;
diff --git a/src/host/layer23/src/mobile/gsm48_mm.c b/src/host/layer23/src/mobile/gsm48_mm.c
index 4b1db1e..f32d57a 100644
--- a/src/host/layer23/src/mobile/gsm48_mm.c
+++ b/src/host/layer23/src/mobile/gsm48_mm.c
@@ -1849,7 +1849,7 @@
 	subscr->sim_valid = 0;
 
 	/* wait for RR idle and then power off when IMSI is detached */
-	if (ms->shutdown) {
+	if (ms->shutdown != MS_SHUTDOWN_NONE) {
 		if (mm->state == GSM48_MM_ST_MM_IDLE) {
 			mobile_exit(ms, 1);
 			return 0;
@@ -1944,7 +1944,7 @@
 		new_mm_state(mm, GSM48_MM_ST_WAIT_NETWORK_CMD, 0);
 
 		/* power off */
-		if (ms->shutdown) {
+		if (ms->shutdown != MS_SHUTDOWN_NONE) {
 			mobile_exit(ms, 1);
 			return 0;
 		}
diff --git a/src/host/layer23/src/mobile/vty_interface.c b/src/host/layer23/src/mobile/vty_interface.c
index 3703ac5..d11f625 100644
--- a/src/host/layer23/src/mobile/vty_interface.c
+++ b/src/host/layer23/src/mobile/vty_interface.c
@@ -123,7 +123,7 @@
 {
 	if (vty_reading)
 		return;
-	if (ms->shutdown != 0)
+	if (ms->shutdown != MS_SHUTDOWN_NONE)
 		return;
 	vty_out(vty, "You must restart MS '%s' ('shutdown / no shutdown') for "
 		"change to take effect!%s", ms->name, VTY_NEWLINE);
@@ -142,7 +142,7 @@
 
 	llist_for_each_entry(ms, &ms_list, entity) {
 		if (!strcmp(ms->name, name)) {
-			if (ms->shutdown) {
+			if (ms->shutdown != MS_SHUTDOWN_NONE) {
 				vty_out(vty, "MS '%s' is admin down.%s", name,
 					VTY_NEWLINE);
 				return NULL;
@@ -189,9 +189,9 @@
 		service = ", MM connection active";
 
 	vty_out(vty, "MS '%s' is %s%s%s%s", ms->name,
-		(ms->shutdown) ? "administratively " : "",
-		(ms->shutdown || !ms->started) ? "down" : "up",
-		(!ms->shutdown) ? service : "",
+		(ms->shutdown != MS_SHUTDOWN_NONE) ? "administratively " : "",
+		(ms->shutdown != MS_SHUTDOWN_NONE || !ms->started) ? "down" : "up",
+		(ms->shutdown == MS_SHUTDOWN_NONE) ? service : "",
 		VTY_NEWLINE);
 	vty_out(vty, "  IMEI: %s%s", set->imei, VTY_NEWLINE);
 	vty_out(vty, "     IMEISV: %s%s", set->imeisv, VTY_NEWLINE);
@@ -201,7 +201,7 @@
 	else
 		vty_out(vty, "     IMEI generation: fixed%s", VTY_NEWLINE);
 
-	if (ms->shutdown)
+	if (ms->shutdown != MS_SHUTDOWN_NONE)
 		return;
 
 	if (set->plmn_mode == PLMN_MODE_AUTO)
@@ -303,7 +303,7 @@
 		gsm_subscr_dump(&ms->subscr, print_vty, vty);
 	} else {
 		llist_for_each_entry(ms, &ms_list, entity) {
-			if (!ms->shutdown) {
+			if (ms->shutdown == MS_SHUTDOWN_NONE) {
 				gsm_subscr_dump(&ms->subscr, print_vty, vty);
 				vty_out(vty, "%s", VTY_NEWLINE);
 			}
@@ -1529,7 +1529,7 @@
 			(set->test_always) ? "everywhere" : "foreign-country",
 			VTY_NEWLINE);
 	/* no shutdown must be written to config, because shutdown is default */
-	vty_out(vty, " %sshutdown%s", (ms->shutdown) ? "" : "no ",
+	vty_out(vty, " %sshutdown%s", (ms->shutdown != MS_SHUTDOWN_NONE) ? "" : "no ",
 		VTY_NEWLINE);
 	vty_out(vty, "!%s", VTY_NEWLINE);
 }
@@ -2699,7 +2699,7 @@
 	struct osmocom_ms *ms = vty->index, *tmp;
 	int rc;
 
-	if (ms->shutdown != 3)
+	if (ms->shutdown != MS_SHUTDOWN_COMPL)
 		return CMD_SUCCESS;
 
 	llist_for_each_entry(tmp, &ms_list, entity) {
@@ -2738,7 +2738,7 @@
 {
 	struct osmocom_ms *ms = vty->index;
 
-	if (ms->shutdown == 0)
+	if (ms->shutdown == MS_SHUTDOWN_NONE)
 		mobile_exit(ms, 0);
 
 	return CMD_SUCCESS;
@@ -2749,7 +2749,7 @@
 {
 	struct osmocom_ms *ms = vty->index;
 
-	if (ms->shutdown <= 1)
+	if (ms->shutdown <= MS_SHUTDOWN_IMSI_DETACH)
 		mobile_exit(ms, 1);
 
 	return CMD_SUCCESS;

-- 
To view, visit https://gerrit.osmocom.org/4909
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Iee1140e4848923c7270495c381bf87b7e3fddee1
Gerrit-PatchSet: 3
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Owner: Holger Freyther <holger at freyther.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Holger Freyther <holger at freyther.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>



More information about the gerrit-log mailing list