[PATCHv2 0/3] CPHS SPN, short-SPN support

[PATCHv2 0/3] CPHS SPN, short-SPN support

[Oleg Zhurakivskyy]

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

Hello,

Please find the reworked version of the CPHS SPN and short-SPN TODO items.

Changes from v1:

- Removed some in place code to emit the operator display name,
  netreg_emit_operator_display_name() is used everywhere.
- Taking advantage of the CPHS Information Service Table.
  CPHS 4.2 specifies active / allocated bits for SPN short only.
  So, if CPHS SPN short bits are on, the logic is to check
  first for CPHS SPN with a fallback to CPHS SPN short.
- Separate callbacks for SPN variants in order to make the logic simpler.
- Refresh cases for CPHS SPN variants are taken into account.

What's missing:

- changes to the SPN callback in src/grps.c in order not to duplicate the logic for SPN, CPHS SPN and short SPN in both files.

Regards,
Oleg

Oleg Zhurakivskyy (3):
  network: Use netreg_emit_operator_display_name()
  network: Add CPHS SPN, short-SPN fallbacks
  TODO: Remove completed CPHS SPN and short-SPN tasks

 TODO          |   14 ---
 src/network.c |  254 ++++++++++++++++++++++++++++++++++++++-------------------
 2 files changed, 170 insertions(+), 98 deletions(-)

-- 
1.7.5.4

[PATCHv2 1/3] network: Use netreg_emit_operator_display_name()

[Oleg Zhurakivskyy]

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

Redundant in place code removed, netreg_emit_operator_display_name()
is now used consistently everywhere in network.c
---
 src/network.c |   76 ++++++++++++++++----------------------------------------
 1 files changed, 22 insertions(+), 54 deletions(-)

diff --git a/src/network.c b/src/network.c
index e190389..cc708a2 100644
--- a/src/network.c
+++ b/src/network.c
@@ -419,13 +419,22 @@ static char *get_operator_display_name(struct ofono_netreg *netreg)
 	return name;
 }
 
+static void netreg_emit_operator_display_name(struct ofono_netreg *netreg)
+{
+	const char *operator = get_operator_display_name(netreg);
+
+	ofono_dbus_signal_property_changed(ofono_dbus_get_connection(),
+					__ofono_atom_get_path(netreg->atom),
+					OFONO_NETWORK_REGISTRATION_INTERFACE,
+					"Name", DBUS_TYPE_STRING, &operator);
+}
+
 static void set_network_operator_name(struct network_operator_data *opd,
 					const char *name)
 {
 	DBusConnection *conn = ofono_dbus_get_connection();
 	struct ofono_netreg *netreg = opd->netreg;
 	const char *path;
-	const char *operator;
 
 	if (name[0] == '\0')
 		return;
@@ -443,16 +452,8 @@ static void set_network_operator_name(struct network_operator_data *opd,
 	if (opd->eons_info && opd->eons_info->longname)
 		return;
 
-	if (opd == netreg->current_operator) {
-		const char *path = __ofono_atom_get_path(netreg->atom);
-
-		operator = get_operator_display_name(netreg);
-
-		ofono_dbus_signal_property_changed(conn, path,
-					OFONO_NETWORK_REGISTRATION_INTERFACE,
-					"Name", DBUS_TYPE_STRING,
-					&operator);
-	}
+	if (opd == netreg->current_operator)
+		netreg_emit_operator_display_name(netreg);
 
 	/* Don't emit when only operator name is reported */
 	if (opd->mcc[0] == '\0' && opd->mnc[0] == '\0')
@@ -498,16 +499,8 @@ static void set_network_operator_eons_info(struct network_operator_data *opd,
 					OFONO_NETWORK_OPERATOR_INTERFACE,
 					"Name", DBUS_TYPE_STRING, &newname);
 
-		if (opd == netreg->current_operator) {
-			const char *npath = __ofono_atom_get_path(netreg->atom);
-			const char *operator =
-				get_operator_display_name(netreg);
-
-			ofono_dbus_signal_property_changed(conn, npath,
-					OFONO_NETWORK_REGISTRATION_INTERFACE,
-					"Name", DBUS_TYPE_STRING,
-					&operator);
-		}
+		if (opd == netreg->current_operator)
+			netreg_emit_operator_display_name(netreg);
 	}
 
 	if (old_eons_info && old_eons_info->info)
@@ -1209,7 +1202,6 @@ static void current_operator_callback(const struct ofono_error *error,
 	struct ofono_netreg *netreg = data;
 	const char *path = __ofono_atom_get_path(netreg->atom);
 	GSList *op = NULL;
-	const char *operator;
 
 	DBG("%p, %p", netreg, netreg->current_operator);
 
@@ -1279,12 +1271,7 @@ static void current_operator_callback(const struct ofono_error *error,
 	}
 
 emit:
-	operator = get_operator_display_name(netreg);
-
-	ofono_dbus_signal_property_changed(conn, path,
-					OFONO_NETWORK_REGISTRATION_INTERFACE,
-					"Name", DBUS_TYPE_STRING,
-					&operator);
+	netreg_emit_operator_display_name(netreg);
 
 	if (netreg->current_operator) {
 		if (netreg->current_operator->mcc[0] != '\0') {
@@ -1628,42 +1615,23 @@ static void sim_spdi_read_cb(int ok, int length, int record,
 				int record_length, void *user_data)
 {
 	struct ofono_netreg *netreg = user_data;
-	struct network_operator_data *current = netreg->current_operator;
 
 	if (!ok)
 		return;
 
 	netreg->spdi = sim_spdi_new(data, length);
 
-	if (current == NULL)
+	if (netreg->current_operator == NULL)
 		return;
 
-	if (netreg->status == NETWORK_REGISTRATION_STATUS_ROAMING) {
-		DBusConnection *conn = ofono_dbus_get_connection();
-		const char *path = __ofono_atom_get_path(netreg->atom);
-		const char *operator;
-
-		if (!sim_spdi_lookup(netreg->spdi,
-					current->mcc, current->mnc))
-			return;
-
-		operator = get_operator_display_name(netreg);
-
-		ofono_dbus_signal_property_changed(conn, path,
-					OFONO_NETWORK_REGISTRATION_INTERFACE,
-					"Name", DBUS_TYPE_STRING,
-					&operator);
-	}
-}
+	if (netreg->status != NETWORK_REGISTRATION_STATUS_ROAMING)
+		return;
 
-static void netreg_emit_operator_display_name(struct ofono_netreg *netreg)
-{
-	const char *operator = get_operator_display_name(netreg);
+	if (!sim_spdi_lookup(netreg->spdi, netreg->current_operator->mcc,
+				netreg->current_operator->mnc))
+		return;
 
-	ofono_dbus_signal_property_changed(ofono_dbus_get_connection(),
-					__ofono_atom_get_path(netreg->atom),
-					OFONO_NETWORK_REGISTRATION_INTERFACE,
-					"Name", DBUS_TYPE_STRING, &operator);
+	netreg_emit_operator_display_name(netreg);
 }
 
 static void sim_spn_display_condition_parse(struct ofono_netreg *netreg,
-- 
1.7.5.4

[PATCHv2 2/3] network: Add CPHS SPN, short-SPN fallbacks

[Oleg Zhurakivskyy]

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

---
 src/network.c |  178 +++++++++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 148 insertions(+), 30 deletions(-)

diff --git a/src/network.c b/src/network.c
index cc708a2..5b2dc9a 100644
--- a/src/network.c
+++ b/src/network.c
@@ -57,6 +57,12 @@ enum operator_status {
 	OPERATOR_STATUS_FORBIDDEN =	3,
 };
 
+enum spn_flags {
+	SPN_3GPP =		0x1,
+	SPN_CPHS =		0x2,
+	SPN_CPHS_SHORT =	0x4,
+};
+
 struct ofono_netreg {
 	int status;
 	int location;
@@ -70,7 +76,6 @@ struct ofono_netreg {
 	int flags;
 	DBusMessage *pending;
 	int signal_strength;
-	char *spn;
 	struct sim_spdi *spdi;
 	struct sim_eons *eons;
 	struct ofono_sim *sim;
@@ -82,6 +87,8 @@ struct ofono_netreg {
 	void *driver_data;
 	struct ofono_atom *atom;
 	unsigned int hfp_watch;
+	char *spn;
+	guint8 spn_flags;
 };
 
 struct network_operator_data {
@@ -1679,27 +1686,150 @@ static gboolean sim_spn_parse(const void *data, int length, char **dst)
 	return TRUE;
 }
 
+static inline gboolean sim_cphs_spn_enabled(struct ofono_sim *sim)
+{
+	const guint8 *st;
+
+	if (ofono_sim_get_cphs_phase(sim) != OFONO_SIM_CPHS_PHASE_2G)
+		return FALSE;
+
+	st = ofono_sim_get_cphs_service_table(sim);
+
+	/*
+	 * CPHS short SPN bits are located in byte 1, bits 7 and 8
+	 */
+	return (st && (st[0] >> 6 == 3));
+}
+
+static void ofono_netreg_spn_free(struct ofono_netreg *netreg)
+{
+	gboolean had_spn = netreg->spn != NULL && strlen(netreg->spn) > 0;
+
+	g_free(netreg->spn);
+	netreg->spn = NULL;
+	netreg->spn_flags = 0;
+
+	/*
+	 * We can't determine whether the property really changed
+	 * without checking the name, before and after.  Instead we use a
+	 * simple heuristic, which will not always be correct
+	 */
+	if (had_spn && netreg->current_operator)
+		netreg_emit_operator_display_name(netreg);
+}
+
+static void sim_cphs_spn_short_read_cb(int ok, int length, int record,
+					const unsigned char *data,
+					int record_length, void *user_data)
+{
+	struct ofono_netreg *netreg = user_data;
+
+	if (netreg->spn_flags & (SPN_3GPP | SPN_CPHS) || !ok)
+		return;
+
+	if (!sim_spn_parse(data, length, &netreg->spn))
+		return;
+
+	netreg->spn_flags = SPN_CPHS_SHORT;
+
+	if (netreg->current_operator)
+		netreg_emit_operator_display_name(netreg);
+}
+
+static void sim_cphs_spn_read_cb(int ok, int length, int record,
+					const unsigned char *data,
+					int record_length, void *user_data)
+{
+	struct ofono_netreg *netreg = user_data;
+
+	if (netreg->spn_flags & SPN_3GPP)
+		return;
+
+	if (!ok) {
+		if (sim_cphs_spn_enabled(netreg->sim))
+			ofono_sim_read(netreg->sim_context,
+					SIM_EF_CPHS_SPN_SHORT_FILEID,
+					OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
+					sim_cphs_spn_short_read_cb, netreg);
+
+		return;
+	}
+
+	if (!sim_spn_parse(data, length, &netreg->spn))
+		return;
+
+	netreg->spn_flags = SPN_CPHS;
+
+	if (netreg->current_operator)
+		netreg_emit_operator_display_name(netreg);
+}
+
 static void sim_spn_read_cb(int ok, int length, int record,
 				const unsigned char *data,
 				int record_length, void *user_data)
 {
 	struct ofono_netreg *netreg = user_data;
-	unsigned char dcbyte;
 
-	if (!ok)
-		return;
+	if (!ok) {
+		if (sim_cphs_spn_enabled(netreg->sim))
+			ofono_sim_read(netreg->sim_context,
+					SIM_EF_CPHS_SPN_FILEID,
+					OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
+					sim_cphs_spn_read_cb, netreg);
 
-	dcbyte = data[0];
+		return;
+	}
 
 	if (!sim_spn_parse(data + 1, length - 1, &netreg->spn))
 		return;
 
-	sim_spn_display_condition_parse(netreg, dcbyte);
+	netreg->spn_flags = SPN_3GPP;
+
+	sim_spn_display_condition_parse(netreg, data[0]);
 
 	if (netreg->current_operator)
 		netreg_emit_operator_display_name(netreg);
 }
 
+static void sim_cphs_spn_short_changed(int id, void *userdata)
+{
+	struct ofono_netreg *netreg = userdata;
+
+	if (netreg->spn_flags & (SPN_3GPP | SPN_CPHS))
+		return;
+
+	ofono_netreg_spn_free(netreg);
+
+	ofono_sim_read(netreg->sim_context, SIM_EFSPN_FILEID,
+			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
+			sim_cphs_spn_short_read_cb, netreg);
+}
+
+static void sim_cphs_spn_changed(int id, void *userdata)
+{
+	struct ofono_netreg *netreg = userdata;
+
+	if (netreg->spn_flags & SPN_3GPP)
+		return;
+
+	ofono_netreg_spn_free(netreg);
+
+	ofono_sim_read(netreg->sim_context, SIM_EFSPN_FILEID,
+			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
+			sim_cphs_spn_read_cb, netreg);
+}
+
+static void sim_spn_changed(int id, void *userdata)
+{
+	struct ofono_netreg *netreg = userdata;
+
+	ofono_netreg_spn_free(netreg);
+
+	ofono_sim_read(netreg->sim_context, SIM_EFSPN_FILEID,
+			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
+			sim_spn_read_cb, netreg);
+}
+
 int ofono_netreg_get_location(struct ofono_netreg *netreg)
 {
 	if (netreg == NULL)
@@ -1989,30 +2119,6 @@ static void sim_pnn_opl_changed(int id, void *userdata)
 			sim_pnn_read_cb, netreg);
 }
 
-static void sim_spn_changed(int id, void *userdata)
-{
-	struct ofono_netreg *netreg = userdata;
-	gboolean had_spn = netreg->spn != NULL && strlen(netreg->spn) > 0;
-
-	netreg->flags &= ~(NETWORK_REGISTRATION_FLAG_HOME_SHOW_PLMN |
-			NETWORK_REGISTRATION_FLAG_ROAMING_SHOW_SPN);
-
-	g_free(netreg->spn);
-	netreg->spn = NULL;
-
-	/*
-	 * We can't determine whether the property really changed
-	 * without checking the name, before and after.  Instead we use a
-	 * simple heuristic, which will not always be correct
-	 */
-	if (netreg->current_operator && had_spn)
-		netreg_emit_operator_display_name(netreg);
-
-	ofono_sim_read(netreg->sim_context, SIM_EFSPN_FILEID,
-			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
-			sim_spn_read_cb, netreg);
-}
-
 static void sim_spdi_changed(int id, void *userdata)
 {
 	struct ofono_netreg *netreg = userdata;
@@ -2143,6 +2249,18 @@ void ofono_netreg_register(struct ofono_netreg *netreg)
 						sim_spn_changed, netreg,
 						NULL);
 
+		if (sim_cphs_spn_enabled(netreg->sim)) {
+			ofono_sim_add_file_watch(netreg->sim_context,
+						SIM_EF_CPHS_SPN_SHORT_FILEID,
+						sim_cphs_spn_short_changed,
+						netreg, NULL);
+
+			ofono_sim_add_file_watch(netreg->sim_context,
+						SIM_EF_CPHS_SPN_FILEID,
+						sim_cphs_spn_changed, netreg,
+						NULL);
+		}
+
 		if (__ofono_sim_service_available(netreg->sim,
 				SIM_UST_SERVICE_PROVIDER_DISPLAY_INFO,
 				SIM_SST_SERVICE_PROVIDER_DISPLAY_INFO)) {
-- 
1.7.5.4

[PATCHv2 3/3] TODO: Remove completed CPHS SPN and short-SPN tasks

[Oleg Zhurakivskyy]

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

---
 TODO |   14 --------------
 1 files changed, 0 insertions(+), 14 deletions(-)

diff --git a/TODO b/TODO
index 46dadce..484f829 100644
--- a/TODO
+++ b/TODO
@@ -102,20 +102,6 @@ SIM / SIM File system
   Priority: Low
   Complexity: C4
 
-- CPHS SPN support
-
-  Support reading of the CPHS-defined SPN field from SIMs
-
-  Priority: High
-  Complexity: C1
-
-- CPHS Short SPN support
-
-  Support reading of the CPHS-defined short SPN field from SIMs
-
-  Priority: Medium
-  Complexity: C1
-
 
 Bluetooth HFP
 =============
-- 
1.7.5.4

Re: [PATCHv2 1/3] network: Use netreg_emit_operator_display_name()

[Denis Kenzior]

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

Hi Oleg,

On 12/13/2011 05:36 AM, Oleg Zhurakivskyy wrote:
> Redundant in place code removed, netreg_emit_operator_display_name()
> is now used consistently everywhere in network.c
> ---
>  src/network.c |   76 ++++++++++++++++----------------------------------------
>  1 files changed, 22 insertions(+), 54 deletions(-)
> 

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCHv2 2/3] network: Add CPHS SPN, short-SPN fallbacks

[Denis Kenzior]

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

Hi Oleg,

On 12/13/2011 05:36 AM, Oleg Zhurakivskyy wrote:
> ---
>  src/network.c |  178 +++++++++++++++++++++++++++++++++++++++++++++++----------
>  1 files changed, 148 insertions(+), 30 deletions(-)

I went ahead and applied this patch, but modified it heavily afterward.
 Can you please check the changes starting at 7f18695f and make sure
that things are still working as intended and I didn't screw anything up.

Regards,
-Denis

Re: [PATCHv2 3/3] TODO: Remove completed CPHS SPN and short-SPN tasks

[Denis Kenzior]

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

Hi Oleg,

On 12/13/2011 05:36 AM, Oleg Zhurakivskyy wrote:
> ---
>  TODO |   14 --------------
>  1 files changed, 0 insertions(+), 14 deletions(-)
> 

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCHv2 2/3] network: Add CPHS SPN, short-SPN fallbacks

[Oleg Zhurakivskyy]

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

Hello Denis,

On 12/17/2011 02:54 AM, Denis Kenzior wrote:
> I went ahead and applied this patch, but modified it heavily afterward.
>   Can you please check the changes starting at 7f18695f and make sure
> that things are still working as intended and I didn't screw anything up.

Everything works as expected, thanks for the help!

I just wonder now about the case when the SPN update would happen while the 
NETWORK_REGISTRATION_FLAG_READING_SPN is on.

diff --git a/src/network.c b/src/network.c
struct ofono_netreg {
[...]
         char *spn;
+       unsigned int spn_update;
  };
[...]

+static gboolean spn_update_cb(gpointer userdata)
+{
+       struct ofono_netreg *netreg = userdata;
+
+       if (netreg->flags & NETWORK_REGISTRATION_FLAG_READING_SPN)
+               return TRUE;
+
+       netreg->spn_update = 0;
+
+       sim_spn_changed(0, netreg);
+
+       return FALSE;
+}
+
  static void sim_spn_changed(int id, void *userdata)
  {
         struct ofono_netreg *netreg = userdata;
         gboolean had_spn;

-       if (netreg->flags & NETWORK_REGISTRATION_FLAG_READING_SPN)
+       if (netreg->flags & NETWORK_REGISTRATION_FLAG_READING_SPN) {
+               if (netreg->spn_update == 0)
+                       netreg->spn_update = g_timeout_add(1000,
+                                                       spn_update_cb, netreg);
+
                 return;
+       }
[...]

What do you think?

I will be sending SPN changes for src/gprs.c soon.

Regards,
Oleg
-- 
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki

Re: [PATCHv2 2/3] network: Add CPHS SPN, short-SPN fallbacks

[Denis Kenzior]

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

Hi Oleg,

On 12/19/2011 06:58 AM, Oleg Zhurakivskyy wrote:
> Hello Denis,
> 
> On 12/17/2011 02:54 AM, Denis Kenzior wrote:
>> I went ahead and applied this patch, but modified it heavily afterward.
>>   Can you please check the changes starting at 7f18695f and make sure
>> that things are still working as intended and I didn't screw anything up.
> 
> Everything works as expected, thanks for the help!
> 
> I just wonder now about the case when the SPN update would happen while
> the NETWORK_REGISTRATION_FLAG_READING_SPN is on.
> 

This is possible if extremely unlikely.  However, the nasty part is that
we probably would need to take care of other cases besides SPN...

> diff --git a/src/network.c b/src/network.c
> struct ofono_netreg {
> [...]
>         char *spn;
> +       unsigned int spn_update;
>  };
> [...]
> 
> +static gboolean spn_update_cb(gpointer userdata)
> +{
> +       struct ofono_netreg *netreg = userdata;
> +
> +       if (netreg->flags & NETWORK_REGISTRATION_FLAG_READING_SPN)
> +               return TRUE;
> +
> +       netreg->spn_update = 0;
> +
> +       sim_spn_changed(0, netreg);
> +
> +       return FALSE;
> +}
> +
>  static void sim_spn_changed(int id, void *userdata)
>  {
>         struct ofono_netreg *netreg = userdata;
>         gboolean had_spn;
> 
> -       if (netreg->flags & NETWORK_REGISTRATION_FLAG_READING_SPN)
> +       if (netreg->flags & NETWORK_REGISTRATION_FLAG_READING_SPN) {
> +               if (netreg->spn_update == 0)
> +                       netreg->spn_update = g_timeout_add(1000,
> +                                                       spn_update_cb,
> netreg);
> +
>                 return;
> +       }
> [...]
> 
> What do you think?

You have to be careful here since you can't really guarantee that the
operation will succeed in your time allotted.  If you are unlucky the
timer will fire before the entire transaction succeeds and you might be
running the old transaction and the new transaction in parallel.  You
would have to check if the current code can handle this, but probably
not.  And of course you never know what timer value is large enough.

A better approach might be to set another flag in this case, e.g.
RECHECK_SPN, and re-do the transaction.  However, you'd also need to
make sure the sim file cache is flushed as well.

> 
> I will be sending SPN changes for src/gprs.c soon.

Before you do this, a sanity check on whether any MVNOs actually use the
CPHS SPN field might be in order.  It may be that this complexity is not
needed in the gprs.c provisioning logic; and that the EFspn field is
sufficient.

Regards,
-Denis

Re: [PATCHv2 2/3] network: Add CPHS SPN, short-SPN fallbacks

[Oleg Zhurakivskyy]

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

Hello Denis,

On 12/20/2011 08:34 AM, Denis Kenzior wrote:
> You have to be careful here since you can't really guarantee that the
> operation will succeed in your time allotted.  If you are unlucky the
> timer will fire before the entire transaction succeeds and you might be
> running the old transaction and the new transaction in parallel.  You
> would have to check if the current code can handle this, but probably
> not.  And of course you never know what timer value is large enough.

Just a little more explanation here. The g_timeout_add() is periodical, in 
unlucky case it will run a few times checking whether the SPN reading flag is 
cleared and only after that will trigger the new read operation and unregister 
itself. Since we clear the flag in all SPN read callbacks in all cases, this 
should be safe.

> A better approach might be to set another flag in this case, e.g.
> RECHECK_SPN, and re-do the transaction.  However, you'd also need to
> make sure the sim file cache is flushed as well.

This might be more efficient, let me check this.

>> I will be sending SPN changes for src/gprs.c soon.
>
> Before you do this, a sanity check on whether any MVNOs actually use the
> CPHS SPN field might be in order.  It may be that this complexity is not
> needed in the gprs.c provisioning logic; and that the EFspn field is
> sufficient.

Sure, that makes sense. I haven't found anything on usage of CPHS SPN by MVNOs. 
However, that doesn't mean nobody is using it. Anyway, that change won't touch 
the provisioning logic, might this be useful, the patch is almost ready, just 
decide when you see it.

Regards,
Oleg
-- 
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki

Re: [PATCHv2 2/3] network: Add CPHS SPN, short-SPN fallbacks

[Denis Kenzior]

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

Hi Oleg,

On 12/20/2011 07:19 AM, Oleg Zhurakivskyy wrote:
> Hello Denis,
> 
> On 12/20/2011 08:34 AM, Denis Kenzior wrote:
>> You have to be careful here since you can't really guarantee that the
>> operation will succeed in your time allotted.  If you are unlucky the
>> timer will fire before the entire transaction succeeds and you might be
>> running the old transaction and the new transaction in parallel.  You
>> would have to check if the current code can handle this, but probably
>> not.  And of course you never know what timer value is large enough.
> 
> Just a little more explanation here. The g_timeout_add() is periodical,
> in unlucky case it will run a few times checking whether the SPN reading
> flag is cleared and only after that will trigger the new read operation
> and unregister itself. Since we clear the flag in all SPN read callbacks
> in all cases, this should be safe.
> 

Yep, you're right I missed this in the original review.  Your proposed
approach should work just fine, but I'd still prefer something without
timeouts if possible.

>> A better approach might be to set another flag in this case, e.g.
>> RECHECK_SPN, and re-do the transaction.  However, you'd also need to
>> make sure the sim file cache is flushed as well.
> 
> This might be more efficient, let me check this.
> 

Another, third approach that might work is for the atom to keep track of
any transactions in progress and cancel them when the refresh is
detected.  They can then immediately re-initiate the new transaction
without waiting.

Regardless of which solution we pick, the trickiest part here is to make
this automatic and easy for all the EFs that we read; and there are
quite a bit of those.

>>> I will be sending SPN changes for src/gprs.c soon.
>>
>> Before you do this, a sanity check on whether any MVNOs actually use the
>> CPHS SPN field might be in order.  It may be that this complexity is not
>> needed in the gprs.c provisioning logic; and that the EFspn field is
>> sufficient.
> 
> Sure, that makes sense. I haven't found anything on usage of CPHS SPN by
> MVNOs. However, that doesn't mean nobody is using it. Anyway, that
> change won't touch the provisioning logic, might this be useful, the
> patch is almost ready, just decide when you see it.
> 

Sounds good.

Regards,
-Denis

Re: [PATCHv2 2/3] network: Add CPHS SPN, short-SPN fallbacks

[Oleg Zhurakivskyy]

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

Hello Denis,

On 12/20/2011 05:32 PM, Denis Kenzior wrote:
> Yep, you're right I missed this in the original review.  Your proposed
> approach should work just fine, but I'd still prefer something without
> timeouts if possible.

OK.

> Another, third approach that might work is for the atom to keep track of
> any transactions in progress and cancel them when the refresh is
> detected.  They can then immediately re-initiate the new transaction
> without waiting.
>
> Regardless of which solution we pick, the trickiest part here is to make
> this automatic and easy for all the EFs that we read; and there are
> quite a bit of those.

Thanks for the ideas here! The third approach sounds good. Let me see how to fit it.

Regards,
Oleg
-- 
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki