[PATCH 0/4] LTE default bearer APN (QMI)

[PATCH 0/4] LTE default bearer APN (QMI)

[Jonas Bonn]

[-- Attachment #1: Type: text/plain, Size: 1415 bytes --]

When an LTE default bearer is established, an APN is provided to the
gateway from some default settings in the modem.  This APN may be an
empty string or something meaningful depending on how the modem has
been configured, but nonetheless _something_ is used.

In order for connman to be able to handle transitions between LTE and
non-LTE networks, there can only be a single internet context; therefore,
the LTE and non-LTE APN settings really need to be consistent: relying
on a connection with an empty-string APN does not work.

This patch series provides the ability for QMI modems to set up the
default APN for LTE.  With that, it removes the string 'automatic' that
was used as an APN when we previously couldn't determine what the modem
was using.  With this, connman functions much better across network
technology transitions.

Jonas Bonn (4):
  qmi: add LTE atom driver
  qmimodem: initialize LTE driver
  gobi: add LTE atom
  qmimodem: get LTE default bearer APN from modem

 Makefile.am                 |   1 +
 drivers/qmimodem/gprs.c     |  90 ++++++++++++++++++++--
 drivers/qmimodem/lte.c      | 182 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/qmimodem/qmimodem.c |   2 +
 drivers/qmimodem/qmimodem.h |   3 +
 plugins/gobi.c              |   3 +
 6 files changed, 273 insertions(+), 8 deletions(-)
 create mode 100644 drivers/qmimodem/lte.c

-- 
2.14.1

[PATCH 1/4] qmi: add LTE atom driver

[Jonas Bonn]

[-- Attachment #1: Type: text/plain, Size: 5688 bytes --]

This patch adds an LTE atom for QMI modems.

This atom sets the APN that the LTE default bearer should use when
establishing its PDP context.  For the Quectel EC21 that I'm testing
with, this is effected by setting the APN on profile ID 1 from which the
default settings appear to be taken.  As I don't have any other QMI
modem to test with, I can not say for sure that all modems work in this
manner.

Once configured, the default profile settings are used when the
modem connects to the network; for this reason, the LTE atom needs
to be instantiated in post_sim, before the modem is set online.
---
 Makefile.am            |   1 +
 drivers/qmimodem/lte.c | 182 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 183 insertions(+)
 create mode 100644 drivers/qmimodem/lte.c

diff --git a/Makefile.am b/Makefile.am
index d790a215..a921bd2b 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -234,6 +234,7 @@ builtin_sources += $(qmi_sources) \
 			drivers/qmimodem/ussd.c \
 			drivers/qmimodem/gprs.c \
 			drivers/qmimodem/gprs-context.c \
+			drivers/qmimodem/lte.c \
 			drivers/qmimodem/radio-settings.c \
 			drivers/qmimodem/location-reporting.c \
 			drivers/qmimodem/netmon.c
diff --git a/drivers/qmimodem/lte.c b/drivers/qmimodem/lte.c
new file mode 100644
index 00000000..f410c608
--- /dev/null
+++ b/drivers/qmimodem/lte.c
@@ -0,0 +1,182 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2018  Jonas Bonn. All rights reserved.
+ *  Copyright (C) 2018  Norrbonn AB. All rights reserved.
+ *  Copyright (C) 2018  Data Respons ASA. All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#define _GNU_SOURCE
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <errno.h>
+
+#include <glib.h>
+
+#include <ofono/modem.h>
+#include <ofono/gprs-context.h>
+#include <ofono/log.h>
+#include <ofono/lte.h>
+
+#include "qmi.h"
+#include "wds.h"
+
+#include "qmimodem.h"
+
+struct lte_data {
+	struct qmi_service *wds;
+	char* apn;
+};
+
+static void modify_profile_cb(struct qmi_result *result, void *user_data)
+{
+	struct cb_data *cbd = user_data;
+	ofono_lte_cb_t cb = cbd->cb;
+	uint16_t error;
+
+	DBG("");
+
+	if (qmi_result_set_error(result, &error)) {
+		DBG("Failed to modify profile: %d", error);
+		CALLBACK_WITH_FAILURE(cb, cbd->data);
+		return;
+	}
+
+	CALLBACK_WITH_SUCCESS(cb, cbd->data);
+}
+
+static void qmimodem_lte_set_default_attach_info(const struct ofono_lte *lte,
+			const struct ofono_lte_default_attach_info *info,
+			ofono_lte_cb_t cb, void *data)
+{
+	struct lte_data *ldd = ofono_lte_get_data(lte);
+	struct cb_data *cbd = cb_data_new(cb, data);
+	struct qmi_param* param;
+	struct {
+		uint8_t type;
+		uint8_t index;
+	} __attribute__((packed)) p = {
+		.type = 0, /* 3GPP */
+		.index = 1,
+	};
+
+	DBG("LTE config with APN: %s", info->apn);
+
+	g_free(ldd->apn);
+
+	if (strlen(info->apn) > 0)
+		ldd->apn = g_strdup(info->apn);
+	else
+		ldd->apn = NULL;
+
+	param = qmi_param_new();
+	if (!param)
+		goto error;
+
+	/* Profile selector */
+	qmi_param_append(param, 0x01, sizeof(p), &p);
+
+	/* WDS APN Name */
+	qmi_param_append(param, QMI_WDS_PARAM_APN,
+				strlen(info->apn), info->apn);
+
+	/* Modify profile */
+	if (qmi_service_send(ldd->wds, 0x28, param,
+					modify_profile_cb, cbd, g_free) > 0)
+		return;
+
+	qmi_param_free(param);
+
+error:	
+	CALLBACK_WITH_FAILURE(cb, data);
+}
+
+static void create_wds_cb(struct qmi_service *service, void *user_data)
+{
+	struct ofono_lte *lte = user_data;
+	struct lte_data *data = ofono_lte_get_data(lte);
+
+	DBG("");
+
+	if (!service) {
+		ofono_error("Failed to request WDS service");
+		ofono_lte_remove(lte);
+		return;
+	}
+
+	data->wds = qmi_service_ref(service);
+
+	ofono_lte_register(lte);
+}
+
+static int qmimodem_lte_probe(struct ofono_lte *lte, void *data)
+{
+	struct qmi_device *device = data;
+	struct lte_data *ldd;
+
+	DBG("qmimodem lte probe");
+
+	ldd = g_try_new0(struct lte_data, 1);
+	if (!ldd)
+		return -ENOMEM;
+
+	ofono_lte_set_data(lte, ldd);
+
+	qmi_service_create_shared(device, QMI_SERVICE_WDS,
+					create_wds_cb, lte, NULL);
+
+	return 0;
+}
+
+static void qmimodem_lte_remove(struct ofono_lte *lte)
+{
+	struct lte_data *ldd = ofono_lte_get_data(lte);
+
+	DBG("");
+
+	ofono_lte_set_data(lte, NULL);
+
+	qmi_service_unregister_all(ldd->wds);
+
+	qmi_service_unref(ldd->wds);
+
+	g_free(ldd->apn);
+
+	g_free(ldd);
+}
+
+static struct ofono_lte_driver driver = {
+	.name				= "qmimodem",
+	.probe				= qmimodem_lte_probe,
+	.remove				= qmimodem_lte_remove,
+	.set_default_attach_info	= qmimodem_lte_set_default_attach_info,
+};
+
+void qmi_lte_init(void)
+{
+	ofono_lte_driver_register(&driver);
+}
+
+void qmi_lte_exit(void)
+{
+	ofono_lte_driver_unregister(&driver);
+}
-- 
2.14.1

[PATCH 2/4] qmimodem: initialize LTE driver

[Jonas Bonn]

[-- Attachment #1: Type: text/plain, Size: 1234 bytes --]

---
 drivers/qmimodem/qmimodem.c | 2 ++
 drivers/qmimodem/qmimodem.h | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/drivers/qmimodem/qmimodem.c b/drivers/qmimodem/qmimodem.c
index b10ce28c..11e68f2e 100644
--- a/drivers/qmimodem/qmimodem.c
+++ b/drivers/qmimodem/qmimodem.c
@@ -39,6 +39,7 @@ static int qmimodem_init(void)
 	qmi_ussd_init();
 	qmi_gprs_init();
 	qmi_gprs_context_init();
+	qmi_lte_init();
 	qmi_radio_settings_init();
 	qmi_location_reporting_init();
 	qmi_netmon_init();
@@ -51,6 +52,7 @@ static void qmimodem_exit(void)
 	qmi_netmon_exit();
 	qmi_location_reporting_exit();
 	qmi_radio_settings_exit();
+	qmi_lte_exit();
 	qmi_gprs_context_exit();
 	qmi_gprs_exit();
 	qmi_ussd_exit();
diff --git a/drivers/qmimodem/qmimodem.h b/drivers/qmimodem/qmimodem.h
index 4b0fad3f..eeb1375a 100644
--- a/drivers/qmimodem/qmimodem.h
+++ b/drivers/qmimodem/qmimodem.h
@@ -48,6 +48,9 @@ extern void qmi_gprs_exit(void);
 extern void qmi_gprs_context_init(void);
 extern void qmi_gprs_context_exit(void);
 
+extern void qmi_lte_init(void);
+extern void qmi_lte_exit(void);
+
 extern void qmi_radio_settings_init(void);
 extern void qmi_radio_settings_exit(void);
 
-- 
2.14.1

[PATCH 3/4] gobi: add LTE atom

[Jonas Bonn]

[-- Attachment #1: Type: text/plain, Size: 899 bytes --]

This atom needs to be created in post_sim so that the APN can be
written to the default profile before the modem attempts to use the
setting to connect to the network.
---
 plugins/gobi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/plugins/gobi.c b/plugins/gobi.c
index 17900010..e7cf1598 100644
--- a/plugins/gobi.c
+++ b/plugins/gobi.c
@@ -43,6 +43,7 @@
 #include <ofono/ussd.h>
 #include <ofono/gprs.h>
 #include <ofono/gprs-context.h>
+#include <ofono/lte.h>
 #include <ofono/radio-settings.h>
 #include <ofono/location-reporting.h>
 #include <ofono/log.h>
@@ -483,6 +484,8 @@ static void gobi_post_sim(struct ofono_modem *modem)
 
 	DBG("%p", modem);
 
+	ofono_lte_create(modem, "qmimodem", data->device);
+
 	if (data->features & GOBI_CAT)
 		ofono_stk_create(modem, 0, "qmimodem", data->device);
 	else if (data->features & GOBI_CAT_OLD)
-- 
2.14.1

[PATCH 4/4] qmimodem: get LTE default bearer APN from modem

[Jonas Bonn]

[-- Attachment #1: Type: text/plain, Size: 5622 bytes --]

When an LTE modem registers with the network, a default bearer is
automatically established.  The APN used for this bearer is taken from
whatever default settings the modem has.

In order to properly set up the ofono context, we need to find out what
APN the modem has used when negotiating this bearer.  Ideally, we would
want to query the 'current settings', but those aren't available until
start_net has been called and that's done in the gprs-context atom.  As
such, we query the 'default settings' here under the assumption that
that's what the modem will have used.

If we can't get the APN, we do what the AT driver does:  pretend the
bearer wasn't established.  This is a reasonable fallback, currently,
because connman can't handle zero-length APN's anyway; the previous
approach of setting the APN to 'automatic' breaks connman badly when it
needs to switch between LTE and non-LTE networks.
---
 drivers/qmimodem/gprs.c | 90 ++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 82 insertions(+), 8 deletions(-)

diff --git a/drivers/qmimodem/gprs.c b/drivers/qmimodem/gprs.c
index a80d55fe..567e8925 100644
--- a/drivers/qmimodem/gprs.c
+++ b/drivers/qmimodem/gprs.c
@@ -29,12 +29,16 @@
 
 #include "qmi.h"
 #include "nas.h"
+#include "wds.h"
 
 #include "src/common.h"
 #include "qmimodem.h"
 
 struct gprs_data {
+	struct qmi_device* dev;
 	struct qmi_service *nas;
+	struct qmi_service *wds;
+	unsigned int last_auto_context_id;
 };
 
 static bool extract_ss_info(struct qmi_result *result, int *status, int *tech)
@@ -64,8 +68,57 @@ static bool extract_ss_info(struct qmi_result *result, int *status, int *tech)
 	return true;
 }
 
+static void get_default_settings_cb(struct qmi_result *result, void *user_data)
+{
+	struct ofono_gprs* gprs = user_data;
+	struct gprs_data *data = ofono_gprs_get_data(gprs);
+	char* apn = NULL;
+	uint16_t error;
+
+	DBG("");
+
+	if (qmi_result_set_error(result, &error)) {
+		ofono_error("Failed to query default settings: %hd", error);
+		goto error;
+	}
+
+	apn = qmi_result_get_string(result, QMI_WDS_RESULT_APN);
+	if (!apn) {
+		DBG("Default profile has no APN setting");
+	}
+
+error:
+	if (apn) {
+		if (!data->last_auto_context_id) {
+			data->last_auto_context_id = 1;
+			ofono_gprs_cid_activated(gprs, 1, apn);
+		}
+	} else {
+		ofono_warn("LTE context activated but APN missing");
+	}
+}
+
+/*
+ * When an LTE default bearer is established, we need to callback into the
+ * ofono core to let it know; at this point, we are interested in the
+ * finding out what APN is in use so we try to query that first
+ */
+static void get_cid_and_apn(struct ofono_gprs* gprs)
+{
+	struct gprs_data *data = ofono_gprs_get_data(gprs);
+
+	DBG("");
+
+	if (qmi_service_send(data->wds, 0x2c, NULL,
+				get_default_settings_cb, gprs, NULL) > 0)
+		return;
+
+	ofono_warn("Unable to query LTE APN... will not activate context");
+}
+
 static int handle_ss_info(struct qmi_result *result, struct ofono_gprs *gprs)
 {
+	struct gprs_data *data = ofono_gprs_get_data(gprs);
 	int status;
 	int tech;
 
@@ -74,17 +127,17 @@ static int handle_ss_info(struct qmi_result *result, struct ofono_gprs *gprs)
 	if (!extract_ss_info(result, &status, &tech))
 		return -1;
 
-	if (status == NETWORK_REGISTRATION_STATUS_REGISTERED)
+	if (status == NETWORK_REGISTRATION_STATUS_REGISTERED) {
 		if (tech == ACCESS_TECHNOLOGY_EUTRAN) {
 			/* On LTE we are effectively always attached; and
 			 * the default bearer is established as soon as the
 			 * network is joined.
 			 */
-			/* FIXME: query default profile number and APN
-			 * instead of assuming profile 1 and ""
-			 */
-			ofono_gprs_cid_activated(gprs, 1 , "automatic");
+			get_cid_and_apn(gprs);
 		}
+	} else {
+		data->last_auto_context_id = 0;
+	}
 
 	return status;
 }
@@ -198,7 +251,7 @@ static void qmi_attached_status(struct ofono_gprs *gprs,
 	g_free(cbd);
 }
 
-static void create_nas_cb(struct qmi_service *service, void *user_data)
+static void create_wds_cb(struct qmi_service *service, void *user_data)
 {
 	struct ofono_gprs *gprs = user_data;
 	struct gprs_data *data = ofono_gprs_get_data(gprs);
@@ -206,12 +259,12 @@ static void create_nas_cb(struct qmi_service *service, void *user_data)
 	DBG("");
 
 	if (!service) {
-		ofono_error("Failed to request NAS service");
+		ofono_error("Failed to request WDS service");
 		ofono_gprs_remove(gprs);
 		return;
 	}
 
-	data->nas = qmi_service_ref(service);
+	data->wds = qmi_service_ref(service);
 
 	/*
 	 * First get the SS info - the modem may already be connected,
@@ -228,6 +281,25 @@ static void create_nas_cb(struct qmi_service *service, void *user_data)
 	ofono_gprs_register(gprs);
 }
 
+static void create_nas_cb(struct qmi_service *service, void *user_data)
+{
+	struct ofono_gprs *gprs = user_data;
+	struct gprs_data *data = ofono_gprs_get_data(gprs);
+
+	DBG("");
+
+	if (!service) {
+		ofono_error("Failed to request NAS service");
+		ofono_gprs_remove(gprs);
+		return;
+	}
+
+	data->nas = qmi_service_ref(service);
+
+	qmi_service_create_shared(data->dev, QMI_SERVICE_WDS,
+						create_wds_cb, gprs, NULL);
+}
+
 static int qmi_gprs_probe(struct ofono_gprs *gprs,
 				unsigned int vendor, void *user_data)
 {
@@ -240,6 +312,8 @@ static int qmi_gprs_probe(struct ofono_gprs *gprs,
 
 	ofono_gprs_set_data(gprs, data);
 
+	data->dev = device;
+
 	qmi_service_create_shared(device, QMI_SERVICE_NAS,
 						create_nas_cb, gprs, NULL);
 
-- 
2.14.1

Re: [PATCH 1/4] qmi: add LTE atom driver

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1212 bytes --]

Hi Jonas,

On 03/01/2018 04:48 AM, Jonas Bonn wrote:
> This patch adds an LTE atom for QMI modems.
> 
> This atom sets the APN that the LTE default bearer should use when
> establishing its PDP context.  For the Quectel EC21 that I'm testing
> with, this is effected by setting the APN on profile ID 1 from which the
> default settings appear to be taken.  As I don't have any other QMI
> modem to test with, I can not say for sure that all modems work in this
> manner.
> 
> Once configured, the default profile settings are used when the
> modem connects to the network; for this reason, the LTE atom needs
> to be instantiated in post_sim, before the modem is set online.
> ---
>   Makefile.am            |   1 +
>   drivers/qmimodem/lte.c | 182 +++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 183 insertions(+)
>   create mode 100644 drivers/qmimodem/lte.c
> 

Please merge patch 1 & 2 together, that's how we usually do it anyway.

> +struct lte_data {
> +	struct qmi_service *wds;
> +	char* apn;
> +};

Why do you need the apn?  I don't see it used anywhere.  Also not our 
style, should be char *apn.

The rest looks fine to me.

Regards,
-Denis

Re: [PATCH 1/4] qmi: add LTE atom driver

[Jonas Bonn]

[-- Attachment #1: Type: text/plain, Size: 1628 bytes --]

On 03/01/2018 03:50 PM, Denis Kenzior wrote:
> Hi Jonas,
> 
> On 03/01/2018 04:48 AM, Jonas Bonn wrote:
>> This patch adds an LTE atom for QMI modems.
>>
>> This atom sets the APN that the LTE default bearer should use when
>> establishing its PDP context.  For the Quectel EC21 that I'm testing
>> with, this is effected by setting the APN on profile ID 1 from which the
>> default settings appear to be taken.  As I don't have any other QMI
>> modem to test with, I can not say for sure that all modems work in this
>> manner.
>>
>> Once configured, the default profile settings are used when the
>> modem connects to the network; for this reason, the LTE atom needs
>> to be instantiated in post_sim, before the modem is set online.
>> ---
>>   Makefile.am            |   1 +
>>   drivers/qmimodem/lte.c | 182 
>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 183 insertions(+)
>>   create mode 100644 drivers/qmimodem/lte.c
>>
> 
> Please merge patch 1 & 2 together, that's how we usually do it anyway.

OK.  I broke those up because I thought you were super-pedantic about 
breaking out patches so they were per-file... what's the rule here? 
Per-directory?

> 
>> +struct lte_data {
>> +    struct qmi_service *wds;
>> +    char* apn;
>> +};
> 
> Why do you need the apn?  I don't see it used anywhere.  Also not our 
> style, should be char *apn.

Ah, right you are... that's a remnant of an earlier approach.  Good catch.

I'll resubmit this shortly.

/Jonas

> 
> The rest looks fine to me.
> 
> Regards,
> -Denis

Re: [PATCH 4/4] qmimodem: get LTE default bearer APN from modem

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 3129 bytes --]

Hi Jonas,

On 03/01/2018 04:48 AM, Jonas Bonn wrote:
> When an LTE modem registers with the network, a default bearer is
> automatically established.  The APN used for this bearer is taken from
> whatever default settings the modem has.
> 
> In order to properly set up the ofono context, we need to find out what
> APN the modem has used when negotiating this bearer.  Ideally, we would
> want to query the 'current settings', but those aren't available until
> start_net has been called and that's done in the gprs-context atom.  As
> such, we query the 'default settings' here under the assumption that
> that's what the modem will have used.

I don't recall exactly, but from what I remember the default attach APN 
is not guaranteed to be honored by the operator.  So the default attach 
settings used for the actual active context might be different.  It 
might be better to query those instead.

> 
> If we can't get the APN, we do what the AT driver does:  pretend the
> bearer wasn't established.  This is a reasonable fallback, currently,
> because connman can't handle zero-length APN's anyway; the previous
> approach of setting the APN to 'automatic' breaks connman badly when it
> needs to switch between LTE and non-LTE networks.
> ---
>   drivers/qmimodem/gprs.c | 90 ++++++++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 82 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/qmimodem/gprs.c b/drivers/qmimodem/gprs.c
> index a80d55fe..567e8925 100644
> --- a/drivers/qmimodem/gprs.c
> +++ b/drivers/qmimodem/gprs.c
> @@ -29,12 +29,16 @@
>   
>   #include "qmi.h"
>   #include "nas.h"
> +#include "wds.h"
>   
>   #include "src/common.h"
>   #include "qmimodem.h"
>   
>   struct gprs_data {
> +	struct qmi_device* dev;
>   	struct qmi_service *nas;
> +	struct qmi_service *wds;
> +	unsigned int last_auto_context_id;

Just a nitpick, but why do you have the '*' in different places here?

>   };
>   
>   static bool extract_ss_info(struct qmi_result *result, int *status, int *tech)
> @@ -64,8 +68,57 @@ static bool extract_ss_info(struct qmi_result *result, int *status, int *tech)
>   	return true;
>   }
>   
> +static void get_default_settings_cb(struct qmi_result *result, void *user_data)
> +{
> +	struct ofono_gprs* gprs = user_data;
> +	struct gprs_data *data = ofono_gprs_get_data(gprs);
> +	char* apn = NULL;
> +	uint16_t error;
> +
> +	DBG("");
> +
> +	if (qmi_result_set_error(result, &error)) {
> +		ofono_error("Failed to query default settings: %hd", error);
> +		goto error;
> +	}
> +
> +	apn = qmi_result_get_string(result, QMI_WDS_RESULT_APN);
> +	if (!apn) {
> +		DBG("Default profile has no APN setting");
> +	}
> +
> +error:
> +	if (apn) {
> +		if (!data->last_auto_context_id) {
> +			data->last_auto_context_id = 1;
> +			ofono_gprs_cid_activated(gprs, 1, apn);
> +		}

Why is a success path under an error: label?  This really should be 
reworked to be more readable.

> +	} else {
> +		ofono_warn("LTE context activated but APN missing");
> +	}
> +}
> +

Regards,
-Denis

Re: [PATCH 1/4] qmi: add LTE atom driver

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 649 bytes --]

Hi Jonas,

>> Please merge patch 1 & 2 together, that's how we usually do it anyway.
> 
> OK.  I broke those up because I thought you were super-pedantic about 
> breaking out patches so they were per-file... what's the rule here? 
> Per-directory?

It is per-directory, with the build system being the exception.  E.g. if 
you're adding foo/bar/baz.c to Makefile.am, then Makefile.am changes 
should go with the baz.c changes.  Any files touched that are directly 
related and in the same directory can be grouped together.

If in doubt, keep things separate.  I can easily squash them together 
before pushing.

Regards,
-Denis

Re: [PATCH 4/4] qmimodem: get LTE default bearer APN from modem

[Jonas Bonn]

[-- Attachment #1: Type: text/plain, Size: 2332 bytes --]

On 03/01/2018 04:02 PM, Denis Kenzior wrote:
> Hi Jonas,
> 
> On 03/01/2018 04:48 AM, Jonas Bonn wrote:
>> When an LTE modem registers with the network, a default bearer is
>> automatically established.  The APN used for this bearer is taken from
>> whatever default settings the modem has.
>>
>> In order to properly set up the ofono context, we need to find out what
>> APN the modem has used when negotiating this bearer.  Ideally, we would
>> want to query the 'current settings', but those aren't available until
>> start_net has been called and that's done in the gprs-context atom.  As
>> such, we query the 'default settings' here under the assumption that
>> that's what the modem will have used.
> 
> I don't recall exactly, but from what I remember the default attach APN 
> is not guaranteed to be honored by the operator.  So the default attach 
> settings used for the actual active context might be different.  It 
> might be better to query those instead.

So, this becomes quite messy:

Getting "current settings" does not succeed until after we have called 
start_net.  This is done in the gprs-context atom as part of 
read_settings() for LTE and shares a bunch of code with 
activate_primary() that also calls start_net.

We can move the call of start_net to the gprs atom if we must, but we 
end up duplicating a whole bunch of code.  That's why I wanted to just 
grab the APN from the default settings in the GPRS atom, call 
cid_activated(), and then wait for read_settings() to be called before 
invoking start_net and then getting the rest of the settings.


>> +    struct qmi_device* dev;
>>       struct qmi_service *nas;
>> +    struct qmi_service *wds;
>> +    unsigned int last_auto_context_id;
> 
> Just a nitpick, but why do you have the '*' in different places here?

Because I"m terribly inconsistent with my placement of the '*'...

>> +error:
>> +    if (apn) {
>> +        if (!data->last_auto_context_id) {
>> +            data->last_auto_context_id = 1;
>> +            ofono_gprs_cid_activated(gprs, 1, apn);
>> +        }
> 
> Why is a success path under an error: label?  This really should be 
> reworked to be more readable.

Yeah, I can see how that looks odd...

/Jonas

Re: [PATCH 4/4] qmimodem: get LTE default bearer APN from modem

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2165 bytes --]

Hi Jonas,

On 03/01/2018 09:20 AM, Jonas Bonn wrote:
> On 03/01/2018 04:02 PM, Denis Kenzior wrote:
>> Hi Jonas,
>>
>> On 03/01/2018 04:48 AM, Jonas Bonn wrote:
>>> When an LTE modem registers with the network, a default bearer is
>>> automatically established.  The APN used for this bearer is taken from
>>> whatever default settings the modem has.
>>>
>>> In order to properly set up the ofono context, we need to find out what
>>> APN the modem has used when negotiating this bearer.  Ideally, we would
>>> want to query the 'current settings', but those aren't available until
>>> start_net has been called and that's done in the gprs-context atom.  As
>>> such, we query the 'default settings' here under the assumption that
>>> that's what the modem will have used.
>>
>> I don't recall exactly, but from what I remember the default attach 
>> APN is not guaranteed to be honored by the operator.  So the default 
>> attach settings used for the actual active context might be 
>> different.  It might be better to query those instead.
> 
> So, this becomes quite messy:
> 
> Getting "current settings" does not succeed until after we have called 
> start_net.  This is done in the gprs-context atom as part of 
> read_settings() for LTE and shares a bunch of code with 
> activate_primary() that also calls start_net.
> 
> We can move the call of start_net to the gprs atom if we must, but we 
> end up duplicating a whole bunch of code.  That's why I wanted to just 
> grab the APN from the default settings in the GPRS atom, call 
> cid_activated(), and then wait for read_settings() to be called before 
> invoking start_net and then getting the rest of the settings.
> 

3GPP extended +CGDCONT specifically for this case.  Does QMI have 
anything similar?

> 
>>> +    struct qmi_device* dev;
>>>       struct qmi_service *nas;
>>> +    struct qmi_service *wds;
>>> +    unsigned int last_auto_context_id;
>>
>> Just a nitpick, but why do you have the '*' in different places here?
> 
> Because I"m terribly inconsistent with my placement of the '*'...
> 

:)

Regards,
-Denis

Re: [PATCH 4/4] qmimodem: get LTE default bearer APN from modem

[Jonas Bonn]

[-- Attachment #1: Type: text/plain, Size: 2506 bytes --]

On 03/01/2018 04:27 PM, Denis Kenzior wrote:
> Hi Jonas,
> 
> On 03/01/2018 09:20 AM, Jonas Bonn wrote:
>> On 03/01/2018 04:02 PM, Denis Kenzior wrote:
>>> Hi Jonas,
>>>
>>> On 03/01/2018 04:48 AM, Jonas Bonn wrote:
>>>> When an LTE modem registers with the network, a default bearer is
>>>> automatically established.  The APN used for this bearer is taken from
>>>> whatever default settings the modem has.
>>>>
>>>> In order to properly set up the ofono context, we need to find out what
>>>> APN the modem has used when negotiating this bearer.  Ideally, we would
>>>> want to query the 'current settings', but those aren't available until
>>>> start_net has been called and that's done in the gprs-context atom.  As
>>>> such, we query the 'default settings' here under the assumption that
>>>> that's what the modem will have used.
>>>
>>> I don't recall exactly, but from what I remember the default attach 
>>> APN is not guaranteed to be honored by the operator.  So the default 
>>> attach settings used for the actual active context might be 
>>> different.  It might be better to query those instead.
>>
>> So, this becomes quite messy:
>>
>> Getting "current settings" does not succeed until after we have called 
>> start_net.  This is done in the gprs-context atom as part of 
>> read_settings() for LTE and shares a bunch of code with 
>> activate_primary() that also calls start_net.
>>
>> We can move the call of start_net to the gprs atom if we must, but we 
>> end up duplicating a whole bunch of code.  That's why I wanted to just 
>> grab the APN from the default settings in the GPRS atom, call 
>> cid_activated(), and then wait for read_settings() to be called before 
>> invoking start_net and then getting the rest of the settings.
>>
> 
> 3GPP extended +CGDCONT specifically for this case.  Does QMI have 
> anything similar?

Not that I am able to ascertain.  I'm hoping somebody here can tell me 
what that would be...

For +CGDCONT you effectively have an event saying that a 'transient' 
context has been activated.  There's nothing in the QMI protocol that 
resembles this.  We jsut get "network registered" and then we need to 
understand that there's an underlying bearer, call start_net (for 
whatever reason, despite the net effectively being started already), and 
then query the settings.  The heuristic we have is (network_registered 
&& tech==LTE)... if there something better, I'd love to know!

/Jonas

Re: [PATCH 4/4] qmimodem: get LTE default bearer APN from modem

[Reinhard Speyerer]

[-- Attachment #1: Type: text/plain, Size: 2991 bytes --]

On Thu, Mar 01, 2018 at 04:43:22PM +0100, Jonas Bonn wrote:
> On 03/01/2018 04:27 PM, Denis Kenzior wrote:
> > Hi Jonas,
> > 
> > On 03/01/2018 09:20 AM, Jonas Bonn wrote:
> > > On 03/01/2018 04:02 PM, Denis Kenzior wrote:
> > > > Hi Jonas,
> > > > 
> > > > On 03/01/2018 04:48 AM, Jonas Bonn wrote:
> > > > > When an LTE modem registers with the network, a default bearer is
> > > > > automatically established.  The APN used for this bearer is taken from
> > > > > whatever default settings the modem has.
> > > > > 
> > > > > In order to properly set up the ofono context, we need to find out what
> > > > > APN the modem has used when negotiating this bearer.  Ideally, we would
> > > > > want to query the 'current settings', but those aren't available until
> > > > > start_net has been called and that's done in the gprs-context atom.  As
> > > > > such, we query the 'default settings' here under the assumption that
> > > > > that's what the modem will have used.
> > > > 
> > > > I don't recall exactly, but from what I remember the default
> > > > attach APN is not guaranteed to be honored by the operator.  So
> > > > the default attach settings used for the actual active context
> > > > might be different.  It might be better to query those instead.
> > > 
> > > So, this becomes quite messy:
> > > 
> > > Getting "current settings" does not succeed until after we have
> > > called start_net.  This is done in the gprs-context atom as part of
> > > read_settings() for LTE and shares a bunch of code with
> > > activate_primary() that also calls start_net.
> > > 
> > > We can move the call of start_net to the gprs atom if we must, but
> > > we end up duplicating a whole bunch of code.  That's why I wanted to
> > > just grab the APN from the default settings in the GPRS atom, call
> > > cid_activated(), and then wait for read_settings() to be called
> > > before invoking start_net and then getting the rest of the settings.
> > > 
> > 
> > 3GPP extended +CGDCONT specifically for this case.  Does QMI have
> > anything similar?
> 
> Not that I am able to ascertain.  I'm hoping somebody here can tell me what
> that would be...
> 
> For +CGDCONT you effectively have an event saying that a 'transient' context
> has been activated.  There's nothing in the QMI protocol that resembles
> this.  We jsut get "network registered" and then we need to understand that
> there's an underlying bearer, call start_net (for whatever reason, despite
> the net effectively being started already), and then query the settings.
> The heuristic we have is (network_registered && tech==LTE)... if there
> something better, I'd love to know!

Hi Jonas,

if you only need to know the APN and the IP version assigned by the network
for the default bearer you could try to use WDSGetLTEAttachParameters
(WDS msgid 0x0085) instead of using WDSStartNetworkInterface followed by
WDSGetCurrentSettings.

Regards,
Reinhard

Re: [PATCH 4/4] qmimodem: get LTE default bearer APN from modem

[Jonas Bonn]

[-- Attachment #1: Type: text/plain, Size: 693 bytes --]

On 03/01/2018 11:30 PM, Reinhard Speyerer wrote:
> 
> Hi Jonas,
> 
> if you only need to know the APN and the IP version assigned by the network
> for the default bearer you could try to use WDSGetLTEAttachParameters
> (WDS msgid 0x0085) instead of using WDSStartNetworkInterface followed by
> WDSGetCurrentSettings.

Thanks, Reinhard!

That information was invaluable.  I've been looking for that for ages!

I've reworked my patches using your suggestion and things work _much_ 
better.

/Jonas

> 
> Regards,
> Reinhard
> _______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> https://lists.ofono.org/mailman/listinfo/ofono
>