[PATCH 1/4] ofono: Add generic method to convert a list of strings to flags

[PATCH 1/4] ofono: Add generic method to convert a list of strings to flags

[Denis Kenzior]

oFono D-Bus APIs are string based, with several string list based
properties that convey information that might require tracking.  For
example:
	- Interfaces -> list of interfaces supported
	- Features -> list of features supported
	- Capabilities -> modem capabilities

Introduce a new convenience method that can be used to convert such
string lists to a set of flags (bitmap).  Make extract_interfaces()
use it.  While here, tighten up error checking and do not attempt to
parse non-string list signatures.
---
 plugins/ofono.c | 49 ++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 40 insertions(+), 9 deletions(-)

diff --git a/plugins/ofono.c b/plugins/ofono.c
index 65a722fd85d6..1906843335cf 100644
--- a/plugins/ofono.c
+++ b/plugins/ofono.c
@@ -685,29 +685,60 @@ static bool has_interface(uint8_t interfaces,
 	return false;
 }
 
-static uint8_t extract_interfaces(DBusMessageIter *array)
+struct flag_map {
+	const char *value;
+	uint32_t flag;
+};
+
+static int string_list_to_flags(DBusMessageIter *array,
+				const struct flag_map *map, size_t map_len,
+				uint32_t *out_flags)
 {
 	DBusMessageIter entry;
-	uint8_t interfaces = 0;
+	uint32_t flags = 0;
+
+	if (dbus_message_iter_get_arg_type(array) != DBUS_TYPE_ARRAY)
+		return -EINVAL;
+
+	if (dbus_message_iter_get_element_type(array) != DBUS_TYPE_STRING)
+		return -EINVAL;
 
 	dbus_message_iter_recurse(array, &entry);
 
 	while (dbus_message_iter_get_arg_type(&entry) == DBUS_TYPE_STRING) {
+		size_t i;
 		const char *name;
 
 		dbus_message_iter_get_basic(&entry, &name);
 
-		if (g_str_equal(name, OFONO_SIM_INTERFACE))
-			interfaces |= OFONO_API_SIM;
-		else if (g_str_equal(name, OFONO_NETREG_INTERFACE))
-			interfaces |= OFONO_API_NETREG;
-		else if (g_str_equal(name, OFONO_CM_INTERFACE))
-			interfaces |= OFONO_API_CM;
+		for (i = 0; i < map_len; i++)
+			if (!strcmp(name, map[i].value))
+				flags |= map[i].flag;
 
 		dbus_message_iter_next(&entry);
 	}
 
-	return interfaces;
+	if (out_flags)
+		*out_flags = flags;
+
+	return 0;
+}
+
+static uint8_t extract_interfaces(DBusMessageIter *array)
+{
+	static const struct flag_map interfaces_map[] = {
+		{ .value = OFONO_SIM_INTERFACE, .flag = OFONO_API_SIM },
+		{ .value = OFONO_NETREG_INTERFACE, .flag = OFONO_API_NETREG },
+		{ .value = OFONO_CM_INTERFACE, .flag = OFONO_API_CM },
+	};
+	uint32_t flags;
+
+	if (string_list_to_flags(array, interfaces_map,
+					G_N_ELEMENTS(interfaces_map),
+					&flags) < 0)
+		return 0;
+
+	return flags;
 }
 
 static char *extract_nameservers(DBusMessageIter *array)
-- 
2.43.0

[PATCH 2/4] ofono: Set modem->interfaces earlier

[Denis Kenzior]

Update modem->interfaces earlier so that handlers invoked on an
interface being added or removed can check the current interface list.
---
 plugins/ofono.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/plugins/ofono.c b/plugins/ofono.c
index 1906843335cf..fd8b442a674c 100644
--- a/plugins/ofono.c
+++ b/plugins/ofono.c
@@ -2018,18 +2018,15 @@ static gboolean modem_changed(DBusConnection *conn, DBusMessage *message,
 
 		connman_device_set_powered(modem->device, modem->online);
 	} else if (g_str_equal(key, "Interfaces")) {
-		uint8_t interfaces;
+		uint8_t new_interfaces = extract_interfaces(&value);
+		uint8_t old_interfaces = modem->interfaces;
 
-		interfaces = extract_interfaces(&value);
-
-		if (interfaces == modem->interfaces)
+		if (new_interfaces == old_interfaces)
 			return TRUE;
 
-		DBG("%s Interfaces 0x%02x", modem->path, interfaces);
-
-		modem_update_interfaces(modem, modem->interfaces, interfaces);
-
-		modem->interfaces = interfaces;
+		DBG("%s Interfaces 0x%02x", modem->path, new_interfaces);
+		modem->interfaces = new_interfaces;
+		modem_update_interfaces(modem, old_interfaces, new_interfaces);
 	} else if (g_str_equal(key, "Serial")) {
 		char *serial;
 
-- 
2.43.0

[PATCH 3/4] ofono: combine create_device and ready_to_create_device

[Denis Kenzior]

These two functions are called one after the other, repeating the same
comment in multiple places.  Combine them into a single function that
takes care of checking whether the device can be created, and if so,
creating it.  Move the repeated comment to the new function.

While here, refactor handling of 'ident' which removes the need for the
'out' label.  Also, simplify some logic left over from CDMA support that
was checking whether modem->imsi exists.
---
 plugins/ofono.c | 75 ++++++++++++++++++-------------------------------
 1 file changed, 28 insertions(+), 47 deletions(-)

diff --git a/plugins/ofono.c b/plugins/ofono.c
index fd8b442a674c..ad85c55d3df3 100644
--- a/plugins/ofono.c
+++ b/plugins/ofono.c
@@ -957,57 +957,56 @@ out:
 
 /*
  * This functions tests if we have the necessary information gathered
- * before we are able to create a device.
+ * in order to create the device.
  */
-static bool ready_to_create_device(struct modem_data *modem)
+static bool try_create_device(struct modem_data *modem)
 {
+	struct connman_device *device;
+	char *ident;
+
+	DBG("%s", modem->path);
+
 	if (modem->device)
 		return false;
 
 	if (!modem->imsi)
 		return false;
 
-	return true;
-}
-
-static void create_device(struct modem_data *modem)
-{
-	struct connman_device *device;
-	char *ident = NULL;
 
-	DBG("%s", modem->path);
+	/*
+	 * Create the device and register it at the core. Enabling (setting
+	 * it online is done through the modem_enable() callback.
+	 */
+	device = connman_device_create("ofono", CONNMAN_DEVICE_TYPE_CELLULAR);
+	if (!device) {
+		connman_error("Failed to create device for modem on path: %s",
+				modem->path);
+		return false;
+	}
 
-	if (modem->imsi)
-		ident = modem->imsi;
+	DBG("created device %p", device);
 
-	if (!connman_dbus_validate_ident(ident))
-		ident = connman_dbus_encode_string(ident);
+	if (!connman_dbus_validate_ident(modem->imsi))
+		ident = connman_dbus_encode_string(modem->imsi);
 	else
-		ident = g_strdup(ident);
-
-	device = connman_device_create("ofono", CONNMAN_DEVICE_TYPE_CELLULAR);
-	if (!device)
-		goto out;
-
-	DBG("device %p", device);
+		ident = g_strdup(modem->imsi);
 
 	connman_device_set_ident(device, ident);
+	g_free(ident);
 
 	connman_device_set_string(device, "Path", modem->path);
-
 	connman_device_set_data(device, modem);
 
 	if (connman_device_register(device) < 0) {
-		connman_error("Failed to register cellular device");
+		connman_error("Failed to register cellular device %p", device);
 		connman_device_unref(device);
-		goto out;
+		return false;
 	}
 
 	modem->device = device;
-
 	connman_device_set_powered(modem->device, modem->online);
-out:
-	g_free(ident);
+
+	return true;
 }
 
 static void destroy_device(struct modem_data *modem)
@@ -1835,17 +1834,7 @@ static gboolean sim_changed(DBusConnection *conn, DBusMessage *message,
 
 	if (g_str_equal(key, "SubscriberIdentity")) {
 		sim_update_imsi(modem, &value);
-
-		if (!ready_to_create_device(modem))
-			return TRUE;
-
-		/*
-		 * This is a GSM modem. Create the device and
-		 * register it at the core. Enabling (setting
-		 * it online is done through the
-		 * modem_enable() callback.
-		 */
-		create_device(modem);
+		try_create_device(modem);
 	}
 
 	return TRUE;
@@ -1869,17 +1858,9 @@ static void sim_properties_reply(struct modem_data *modem,
 		if (g_str_equal(key, "SubscriberIdentity")) {
 			sim_update_imsi(modem, &value);
 
-			if (!ready_to_create_device(modem))
+			if (!try_create_device(modem))
 				return;
 
-			/*
-			 * This is a GSM modem. Create the device and
-			 * register it at the core. Enabling (setting
-			 * it online is done through the
-			 * modem_enable() callback.
-			 */
-			create_device(modem);
-
 			if (!modem->online)
 				return;
 
-- 
2.43.0

[PATCH 4/4] ofono: delay device creation until lte settings are provisioned

[Denis Kenzior]

For LTE capable devices, successful network registration typically
requires the default attach APN to be configured.  Some carriers will
allow the device to register to the network even if this setting is
empty or missing.  Sometimes devices will utilize carrier specific
settings present on the device itself (such as profiles in modem NVRAM,
etc).  However, there may be situations where missing default
attach APN settings preclude successful registration, or registration is
performed with an incorrect profile.

Currently ConnMan does not take oFono's LTE support into consideration
when setting the modem 'Online'.  This can result in the modem
attempting registration too early, when the default attach settings have
not been applied.

oFono has recently introduced Modem.Capabilities property which can
provide an early hint whether the modem is LTE capable.  If the modem
is LTE capable, then ConnMan should wait until oFono LongTermEvolution
interface is available (and thus default attach settings have been
applied) prior to setting the device 'Online'.
---
 plugins/ofono.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/plugins/ofono.c b/plugins/ofono.c
index ad85c55d3df3..062905c8a3a6 100644
--- a/plugins/ofono.c
+++ b/plugins/ofono.c
@@ -52,6 +52,7 @@
 #define OFONO_NETREG_INTERFACE		OFONO_SERVICE ".NetworkRegistration"
 #define OFONO_CM_INTERFACE		OFONO_SERVICE ".ConnectionManager"
 #define OFONO_CONTEXT_INTERFACE		OFONO_SERVICE ".ConnectionContext"
+#define OFONO_LTE_INTERFACE		OFONO_SERVICE ".LongTermEvolution"
 
 #define MODEM_ADDED			"ModemAdded"
 #define MODEM_REMOVED			"ModemRemoved"
@@ -70,6 +71,11 @@ enum ofono_api {
 	OFONO_API_SIM =		0x1,
 	OFONO_API_NETREG =	0x2,
 	OFONO_API_CM =		0x4,
+	OFONO_API_LTE =		0x8,
+};
+
+enum capabilities {
+	LTE_CAPABLE = 0x1,
 };
 
 /*
@@ -137,6 +143,7 @@ struct modem_data {
 	bool powered;
 	bool online;
 	uint8_t interfaces;
+	uint8_t capabilities;
 	bool ignore;
 
 	bool set_powered;
@@ -169,6 +176,8 @@ static const char *api2string(enum ofono_api api)
 		return "netreg";
 	case OFONO_API_CM:
 		return "cm";
+	case OFONO_API_LTE:
+		return "lte";
 	}
 
 	return "unknown";
@@ -730,6 +739,7 @@ static uint8_t extract_interfaces(DBusMessageIter *array)
 		{ .value = OFONO_SIM_INTERFACE, .flag = OFONO_API_SIM },
 		{ .value = OFONO_NETREG_INTERFACE, .flag = OFONO_API_NETREG },
 		{ .value = OFONO_CM_INTERFACE, .flag = OFONO_API_CM },
+		{ .value = OFONO_LTE_INTERFACE, .flag = OFONO_API_LTE },
 	};
 	uint32_t flags;
 
@@ -741,6 +751,21 @@ static uint8_t extract_interfaces(DBusMessageIter *array)
 	return flags;
 }
 
+static uint8_t extract_capabilities(DBusMessageIter *array)
+{
+	static const struct flag_map capabilities_map[] = {
+		{ .value = "lte", .flag = LTE_CAPABLE },
+	};
+	uint32_t flags;
+
+	if (string_list_to_flags(array, capabilities_map,
+					G_N_ELEMENTS(capabilities_map),
+					&flags) < 0)
+		return 0;
+
+	return flags;
+}
+
 static char *extract_nameservers(DBusMessageIter *array)
 {
 	DBusMessageIter entry;
@@ -972,6 +997,9 @@ static bool try_create_device(struct modem_data *modem)
 	if (!modem->imsi)
 		return false;
 
+	if ((modem->capabilities & LTE_CAPABLE) &&
+			!has_interface(modem->interfaces, OFONO_API_LTE))
+		return false;
 
 	/*
 	 * Create the device and register it at the core. Enabling (setting
@@ -1940,6 +1968,9 @@ static void modem_update_interfaces(struct modem_data *modem,
 			netreg_get_properties(modem);
 	}
 
+	if (api_added(old_ifaces, new_ifaces, OFONO_API_LTE))
+		try_create_device(modem);
+
 	if (api_removed(old_ifaces, new_ifaces, OFONO_API_CM)) {
 		if (modem->call_get_contexts) {
 			DBG("cancelling pending GetContexts call");
@@ -2017,6 +2048,9 @@ static gboolean modem_changed(DBusConnection *conn, DBusMessage *message,
 		modem->serial = g_strdup(serial);
 
 		DBG("%s Serial %s", modem->path, modem->serial);
+	} else if (g_str_equal(key, "Capabilities")) {
+		modem->capabilities = extract_capabilities(&value);
+		return TRUE;
 	}
 
 	return TRUE;
@@ -2092,6 +2126,12 @@ static void add_modem(const char *path, DBusMessageIter *prop)
 				DBG("%s Ignore this modem", modem->path);
 				modem->ignore = true;
 			}
+		} else if (g_str_equal(key, "Capabilities")) {
+			modem->capabilities = extract_capabilities(&value);
+
+			DBG("lte capable: %s",
+					modem->capabilities & LTE_CAPABLE ?
+					"yes" : "no");
 		}
 
 		dbus_message_iter_next(prop);
-- 
2.43.0

Re: [PATCH 1/4] ofono: Add generic method to convert a list of strings to flags

[patchwork-bot+connman]

Hello:

This series was applied to connman.git (master)
by Denis Kenzior <denkenz@gmail.com>:

On Wed, 27 Mar 2024 14:43:32 -0500 you wrote:
> oFono D-Bus APIs are string based, with several string list based
> properties that convey information that might require tracking.  For
> example:
> 	- Interfaces -> list of interfaces supported
> 	- Features -> list of features supported
> 	- Capabilities -> modem capabilities
> 
> [...]

Here is the summary with links:
  - [1/4] ofono: Add generic method to convert a list of strings to flags
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=7de3a65b33ad
  - [2/4] ofono: Set modem->interfaces earlier
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=43031e8c7b03
  - [3/4] ofono: combine create_device and ready_to_create_device
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=09d819414f46
  - [4/4] ofono: delay device creation until lte settings are provisioned
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=30f1ac870732

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html