[PATCH 0/4] Add support for CPHS customer service profile

[PATCH 0/4] Add support for CPHS customer service profile

[Aki Niemi]

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

Hi all,

This series adds support for CPHS CSP PLMN mode into the netreg atom.

Cheers,
Aki

[PATCH 1/4] simutil: Add EFcsp file and service group IDs

[Aki Niemi]

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

---
 src/simutil.h |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/src/simutil.h b/src/simutil.h
index 463540b..a5a683b 100644
--- a/src/simutil.h
+++ b/src/simutil.h
@@ -37,6 +37,7 @@ enum sim_fileid {
 	SIM_EFIMSI_FILEID =			0x6F07,
 	SIM_EF_CPHS_MWIS_FILEID =		0x6F11,
 	SIM_EF_CPHS_CFF_FILEID =		0x6F13,
+	SIM_EF_CPHS_CSP_FILEID =		0x6F15,
 	SIM_EF_CPHS_INFORMATION_FILEID =	0x6F16,
 	SIM_EF_CPHS_MBDN_FILEID =		0x6F17,
 	SIM_EFUST_FILEID =			0x6F38,
@@ -238,6 +239,21 @@ enum sim_sst_service {
 	SIM_SST_SERVICE_PROVIDER_DISPLAY_INFO =		55
 };
 
+/* CPHS 4.2, Section B4.7 CSP Service Group Codes */
+enum sim_csp_entry {
+	SIM_CSP_ENTRY_CALL_OFFERING =		0x01,
+	SIM_CSP_ENTRY_CALL_RESTRICTION =	0x02,
+	SIM_CSP_ENTRY_OTHER_SUPP_SERVICES =	0x03,
+	SIM_CSP_ENTRY_CALL_COMPLETION =		0x04,
+	SIM_CSP_ENTRY_TELESERVICES =		0x05,
+	SIM_CSP_ENTRY_CPHS_TELESERVICES =	0x06,
+	SIM_CSP_ENTRY_CPHS_FEATURES =		0x07,
+	SIM_CSP_ENTRY_NUMBER_IDENTIFICATION =	0x08,
+	SIM_CSP_ENTRY_PHASE_2GPLUS_SERVICES =	0x09,
+	SIM_CSP_ENTRY_VALUE_ADDED_SERVICES =	0xC0,
+	SIM_CSP_ENTRY_INFORMATION_NUMBERS =	0xD5,
+};
+
 enum ber_tlv_data_type {
 	BER_TLV_DATA_TYPE_UNIVERSAL =		0,
 	BER_TLV_DATA_TYPE_APPLICATION =		1,
-- 
1.7.1

Re: [PATCH 1/4] simutil: Add EFcsp file and service group IDs

[Marcel Holtmann]

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

Hi Aki,

>  src/simutil.h |   16 ++++++++++++++++
>  1 files changed, 16 insertions(+), 0 deletions(-)

patch has been applied. Thanks.

Regards

Marcel

[PATCH 2/4] netreg: Add CPHS CSP PLMN mode implementation

[Aki Niemi]

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

---
 src/network.c |   75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 72 insertions(+), 3 deletions(-)

diff --git a/src/network.c b/src/network.c
index b5450ee..64734d0 100644
--- a/src/network.c
+++ b/src/network.c
@@ -64,6 +64,7 @@ struct ofono_netreg {
 	int cellid;
 	int technology;
 	int mode;
+	gboolean forced_auto;
 	char *base_station;
 	struct network_operator_data *current_operator;
 	GSList *operator_list;
@@ -93,8 +94,11 @@ struct network_operator_data {
 	struct ofono_netreg *netreg;
 };
 
-static const char *registration_mode_to_string(int mode)
+static const char *registration_mode_to_string(int mode, gboolean forced_auto)
 {
+	if (forced_auto)
+		return "auto-only";
+
 	switch (mode) {
 	case NETWORK_REGISTRATION_MODE_AUTO:
 		return "auto";
@@ -160,7 +164,28 @@ static void set_registration_mode(struct ofono_netreg *netreg, int mode)
 		storage_sync(netreg->imsi, SETTINGS_STORE, netreg->settings);
 	}
 
-	strmode = registration_mode_to_string(mode);
+	strmode = registration_mode_to_string(mode, netreg->forced_auto);
+
+	conn = ofono_dbus_get_connection();
+	path = __ofono_atom_get_path(netreg->atom);
+
+	ofono_dbus_signal_property_changed(conn, path,
+					OFONO_NETWORK_REGISTRATION_INTERFACE,
+					"Mode", DBUS_TYPE_STRING, &strmode);
+}
+
+static void set_registration_forced_auto(struct ofono_netreg *netreg, gboolean value)
+{
+	DBusConnection *conn;
+	const char *strmode;
+	const char *path;
+
+	if (netreg->forced_auto == value)
+		return;
+
+	netreg->forced_auto = value;
+
+	strmode = registration_mode_to_string(netreg->mode, value);
 
 	conn = ofono_dbus_get_connection();
 	path = __ofono_atom_get_path(netreg->atom);
@@ -584,6 +609,9 @@ static DBusMessage *network_operator_register(DBusConnection *conn,
 	struct network_operator_data *opd = data;
 	struct ofono_netreg *netreg = opd->netreg;
 
+	if (netreg->forced_auto)
+		return __ofono_error_access_denied(msg);
+
 	if (netreg->pending)
 		return __ofono_error_busy(msg);
 
@@ -753,7 +781,8 @@ static DBusMessage *network_get_properties(DBusConnection *conn,
 
 	const char *status = registration_status_to_string(netreg->status);
 	const char *operator;
-	const char *mode = registration_mode_to_string(netreg->mode);
+	const char *mode = registration_mode_to_string(netreg->mode,
+							netreg->forced_auto);
 
 	reply = dbus_message_new_method_return(msg);
 	if (reply == NULL)
@@ -1572,6 +1601,43 @@ static void sim_spn_read_cb(int ok, int length, int record,
 	}
 }
 
+static void sim_csp_read_cb(int ok, int length, int record,
+				const unsigned char *data,
+				int record_length, void *user_data)
+{
+	struct ofono_netreg *netreg = user_data;
+	int i;
+
+	if (!ok)
+		return;
+
+	if (length < 18 || record_length < 18 || length < record_length)
+		return;
+
+	/*
+	 * According to CPHS 4.2, EFcsp is an array of two-byte service
+	 * entries, each consisting of a one byte service group
+	 * identifier followed by 8 bits; each bit is indicating
+	 * availability of a specific service or feature.
+	 *
+	 * The PLMN mode bit, if present, indicates whether manual
+	 * operator selection should be disabled or enabled. When
+	 * unset, the device is forced to automatic mode; when set,
+	 * manual selection is to be enabled. The latter is also the
+	 * default.
+	 */
+	for (i = 0; i < record_length / 2; i++) {
+		gboolean forced_auto;
+
+		if (data[i * 2] != SIM_CSP_ENTRY_VALUE_ADDED_SERVICES)
+			continue;
+
+		forced_auto = (data[i * 2 + 1] & 0x80) == 0;
+
+		set_registration_forced_auto(netreg, forced_auto);
+	}
+}
+
 int ofono_netreg_get_location(struct ofono_netreg *netreg)
 {
 	if (netreg == NULL)
@@ -1819,6 +1885,9 @@ void ofono_netreg_register(struct ofono_netreg *netreg)
 		ofono_sim_read(netreg->sim, SIM_EFSPN_FILEID,
 				OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 				sim_spn_read_cb, netreg);
+		ofono_sim_read(netreg->sim, SIM_EF_CPHS_CSP_FILEID,
+				OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
+				sim_csp_read_cb, netreg);
 	}
 
 	__ofono_atom_register(netreg->atom, netreg_unregister);
-- 
1.7.1

Re: [PATCH 2/4] netreg: Add CPHS CSP PLMN mode implementation

[Marcel Holtmann]

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

Hi Aki,

>  src/network.c |   75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 72 insertions(+), 3 deletions(-)
> 
> diff --git a/src/network.c b/src/network.c
> index b5450ee..64734d0 100644
> --- a/src/network.c
> +++ b/src/network.c
> @@ -64,6 +64,7 @@ struct ofono_netreg {
>  	int cellid;
>  	int technology;
>  	int mode;
> +	gboolean forced_auto;

do we wanna keep it named forced_auto and not better keep this in sync
with auto_only as we use in the D-Bus API. Or if manual_disabled.

>  	char *base_station;
>  	struct network_operator_data *current_operator;
>  	GSList *operator_list;
> @@ -93,8 +94,11 @@ struct network_operator_data {
>  	struct ofono_netreg *netreg;
>  };
>  
> -static const char *registration_mode_to_string(int mode)
> +static const char *registration_mode_to_string(int mode, gboolean forced_auto)
>  {
> +	if (forced_auto)
> +		return "auto-only";
> +

I see the benefit of keeping this in this function, but I would not have
done it like this actually. I would have kept it next to the function
needing that string and let it do the auto-only check by itself.

Another possibility might be to convert this into

	get_registration_mode_string(struct ofono_netreg *netreg)

Since I am not mistaken, then the netreg->mode is always set before you
are calling this, right?

Regards

Marcel

Re: [PATCH 2/4] netreg: Add CPHS CSP PLMN mode implementation

[Aki Niemi]

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

Hi Marcel,

2011/1/26 Marcel Holtmann <marcel@holtmann.org>:
>> diff --git a/src/network.c b/src/network.c
>> index b5450ee..64734d0 100644
>> --- a/src/network.c
>> +++ b/src/network.c
>> @@ -64,6 +64,7 @@ struct ofono_netreg {
>>       int cellid;
>>       int technology;
>>       int mode;
>> +     gboolean forced_auto;
>
> do we wanna keep it named forced_auto and not better keep this in sync
> with auto_only as we use in the D-Bus API. Or if manual_disabled.

I can rename this auto_only.

>> -static const char *registration_mode_to_string(int mode)
>> +static const char *registration_mode_to_string(int mode, gboolean forced_auto)
>>  {
>> +     if (forced_auto)
>> +             return "auto-only";
>> +
>
> I see the benefit of keeping this in this function, but I would not have
> done it like this actually. I would have kept it next to the function
> needing that string and let it do the auto-only check by itself.

That is how I originally had it, but this ends up being a bit less
code. Also, I'd like to localize all of these constant strings into a
single function and not sprinkle them across the code.

> Another possibility might be to convert this into
>
>        get_registration_mode_string(struct ofono_netreg *netreg)
>
> Since I am not mistaken, then the netreg->mode is always set before you
> are calling this, right?

No, you call it with the new value. The function will then compare the
old and the new, and based on this decide on sending a
PropertyChanged.

Cheers,
Aki

Re: [PATCH 2/4] netreg: Add CPHS CSP PLMN mode implementation

[Marcel Holtmann]

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

Hi Aki,

> >> -static const char *registration_mode_to_string(int mode)
> >> +static const char *registration_mode_to_string(int mode, gboolean forced_auto)
> >>  {
> >> +     if (forced_auto)
> >> +             return "auto-only";
> >> +
> >
> > I see the benefit of keeping this in this function, but I would not have
> > done it like this actually. I would have kept it next to the function
> > needing that string and let it do the auto-only check by itself.
> 
> That is how I originally had it, but this ends up being a bit less
> code. Also, I'd like to localize all of these constant strings into a
> single function and not sprinkle them across the code.

so I am counting two callers to registration_mode_to_string.

> > Another possibility might be to convert this into
> >
> >        get_registration_mode_string(struct ofono_netreg *netreg)
> >
> > Since I am not mistaken, then the netreg->mode is always set before you
> > are calling this, right?
> 
> No, you call it with the new value. The function will then compare the
> old and the new, and based on this decide on sending a
> PropertyChanged.

In the current code base it is always checked and set before:

        if (netreg->mode == mode)
                return;

        netreg->mode = mode;

	...

	strmode = registration_mode_to_string(mode);

Just on a different thought here. The NETWORK_REGISTRATION_MODE_* is
local to src/network.c. We could just add the AUTO_ONLY mode there.

The only downside I is see the already nasty init_registration_status
check, but that might be acceptable compared to crippling the string
conversion function.

However at least the check would be clearly to just checking mode and
not mode + forced_mode.

Regards

Marcel

Re: [PATCH 2/4] netreg: Add CPHS CSP PLMN mode implementation

[Aki Niemi]

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

Hi Marcel,

2011/1/26 Marcel Holtmann <marcel@holtmann.org>:
>> That is how I originally had it, but this ends up being a bit less
>> code. Also, I'd like to localize all of these constant strings into a
>> single function and not sprinkle them across the code.
>
> so I am counting two callers to registration_mode_to_string.

Yup, that's two times "auto-only". ;)

> In the current code base it is always checked and set before:
>
>        if (netreg->mode == mode)
>                return;
>
>        netreg->mode = mode;
>
>        ...
>
>        strmode = registration_mode_to_string(mode);

Ah, right. I was thinking one call up the stack.

> Just on a different thought here. The NETWORK_REGISTRATION_MODE_* is
> local to src/network.c. We could just add the AUTO_ONLY mode there.
>
> The only downside I is see the already nasty init_registration_status
> check, but that might be acceptable compared to crippling the string
> conversion function.
>
> However at least the check would be clearly to just checking mode and
> not mode + forced_mode.

I thought about this, but then we'd need more logic to when the user
calls NetworkRegistration.Register() for automatic mode. That
obviously can't change the mode. But perhaps this is now more
manageable, since we got rid of Deregister(), which means there is
just this one case to worry about.

Cheers,
Aki

Re: [PATCH 2/4] netreg: Add CPHS CSP PLMN mode implementation

[Marcel Holtmann]

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

Hi Aki,

> > Just on a different thought here. The NETWORK_REGISTRATION_MODE_* is
> > local to src/network.c. We could just add the AUTO_ONLY mode there.
> >
> > The only downside I is see the already nasty init_registration_status
> > check, but that might be acceptable compared to crippling the string
> > conversion function.
> >
> > However at least the check would be clearly to just checking mode and
> > not mode + forced_mode.
> 
> I thought about this, but then we'd need more logic to when the user
> calls NetworkRegistration.Register() for automatic mode. That
> obviously can't change the mode. But perhaps this is now more
> manageable, since we got rid of Deregister(), which means there is
> just this one case to worry about.

can you have a stab at this and check how a patch would look like if we
just add another mode. If I am not mistaken we only have the extra code
in the init_registration_status. I could be wrong here since I have not
looked through that code in a long time.

Denis, any comments?

Regards

Marcel

Re: [PATCH 2/4] netreg: Add CPHS CSP PLMN mode implementation

[Aki Niemi]

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

Hi Marcel,

2011/1/26 Marcel Holtmann <marcel@holtmann.org>:
> can you have a stab at this and check how a patch would look like if we
> just add another mode. If I am not mistaken we only have the extra code
> in the init_registration_status. I could be wrong here since I have not
> looked through that code in a long time.

Sure thing.

Cheers,
Aki

[PATCH 3/4] doc: Add documentation for 'auto-only' mode

[Aki Niemi]

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

---
 doc/network-api.txt |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/doc/network-api.txt b/doc/network-api.txt
index 75187cd..2f13a9c 100644
--- a/doc/network-api.txt
+++ b/doc/network-api.txt
@@ -69,18 +69,21 @@ Properties	string Mode [readonly]
 			method of an operator is called.
 
 			The possible values are:
-				"auto"     Network registration is performed
-				           automatically.
-				"manual"   Network operator is selected
-				           manually. If the operator is
-				           currently not selected, registration
-				           is not attempted
+				"auto"       Network registration is performed
+				             automatically.
+				"auto-only"  Network registration is performed
+					     automatically, and manual
+				             selection is disabled.
+				"manual"     Network operator is selected
+				             manually. If the operator is
+				             currently not selected,
+				             registration is not attempted.
 
 		string Status [readonly]
 
 			The current registration status of a modem.
 
-			The possible values are: 
+			The possible values are:
 				"unregistered"  Not registered to any network
 				"registered"    Registered to home network
 				"searching"     Not registered, but searching
-- 
1.7.1

Re: [PATCH 3/4] doc: Add documentation for 'auto-only' mode

[Marcel Holtmann]

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

Hi Aki,

>  doc/network-api.txt |   17 ++++++++++-------
>  1 files changed, 10 insertions(+), 7 deletions(-)

patch has been applied. I did fix a minor indentation style issue on top
of it.

Regards

Marcel

[PATCH 4/4] TODO: Remove completed CPHS CSP task

[Aki Niemi]

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

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

diff --git a/TODO b/TODO
index 41f1f7f..57e0bab 100644
--- a/TODO
+++ b/TODO
@@ -97,19 +97,6 @@ SIM / SIM File system
   Complexity: C1
   Owner: Marit Henriksen <marit.henriksen@stericsson.com>
 
-- Add support for CPHS Customer Service Profile (CSP).  This task adds support
-  for reading the EFcsp file and in particular, interpreting the PLMN mode bit
-  within the 'Value Added Services' service group.
-
-  Based on the PLMN mode value, manual network selection needs to either be
-  enabled or disabled in the netreg atom.  Also to accommodate this, there
-  probably needs to be a new 'forced' mode to indicate that manual network
-  selection is disabled.
-
-  Priority: Medium
-  Complexity: C1
-  Owner: Aki Niemi <aki.niemi@nokia.com>
-
 - Support SIM authentication: SIM and AKA suites.
 
   Priority: Medium
-- 
1.7.1

Re: [PATCH 0/4] Add support for CPHS customer service profile

[Marcel Holtmann]

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

Hi Aki,

> This series adds support for CPHS CSP PLMN mode into the netreg atom.

one open question here is still if we also have to disallow scanning
when this is set?

So we might wanna update the Scan() and Register() method with the
proper error codes and put a note in there for mode = "auto-only".

Regards

Marcel

Re: [PATCH 0/4] Add support for CPHS customer service profile

[Aki Niemi]

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

Hi Marcel,

2011/1/26 Marcel Holtmann <marcel@holtmann.org>:
> one open question here is still if we also have to disallow scanning
> when this is set?
>
> So we might wanna update the Scan() and Register() method with the
> proper error codes and put a note in there for mode = "auto-only".

Good point, it's also missing from the Scan() implementation. I meant
to add it for but forgot. I'll send a v2 shortly.

The other open item was whether to create a new error code for this or
reuse the existing access denied error (my preference).

Cheers,
Aki

Re: [PATCH 0/4] Add support for CPHS customer service profile

[Marcel Holtmann]

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

Hi Aki,

> > one open question here is still if we also have to disallow scanning
> > when this is set?
> >
> > So we might wanna update the Scan() and Register() method with the
> > proper error codes and put a note in there for mode = "auto-only".
> 
> Good point, it's also missing from the Scan() implementation. I meant
> to add it for but forgot. I'll send a v2 shortly.
> 
> The other open item was whether to create a new error code for this or
> reuse the existing access denied error (my preference).

good question. I can see the benefit of having a proper error code, but
I have no preference here right now.

Regards

Marcel

Re: [PATCH 0/4] Add support for CPHS customer service profile

[Aki Niemi]

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

Hi Marcel,

2011/1/26 Marcel Holtmann <marcel@holtmann.org>:
>> > one open question here is still if we also have to disallow scanning
>> > when this is set?
>> >
>> > So we might wanna update the Scan() and Register() method with the
>> > proper error codes and put a note in there for mode = "auto-only".
>>
>> Good point, it's also missing from the Scan() implementation. I meant
>> to add it for but forgot. I'll send a v2 shortly.
>>
>> The other open item was whether to create a new error code for this or
>> reuse the existing access denied error (my preference).
>
> good question. I can see the benefit of having a proper error code, but
> I have no preference here right now.

I really would prefer to use the existing one, which I think came with
the lockdown property. It fits the bill here as well, and since access
denied won't ever occur for these methods for any other reason, the
code will look exactly the same on the client side.

Cheers,
Aki