[PATCH] rilmodem: Fix GPRS feature inavailable issue

[PATCH] rilmodem: Fix GPRS feature inavailable issue

[caiwen.zhang]

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

From: Caiwen Zhang <caiwen.zhang@intel.com>

When query max cid if rild radio status isn't RADIO_STATUS_ON, it may
fail, gprs atom will be removed, gprs feature will always be inavailable.
---
 drivers/rilmodem/gprs.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/rilmodem/gprs.c b/drivers/rilmodem/gprs.c
index 0ec9d5f..f3bdc86 100644
--- a/drivers/rilmodem/gprs.c
+++ b/drivers/rilmodem/gprs.c
@@ -54,8 +54,11 @@ struct ril_gprs_data {
 	gboolean ofono_attached;
 	int rild_status;
 	int pending_deact_req;
+	gboolean registered;
 };
 
+static void query_max_cids(struct ofono_gprs *gprs);
+
 /*
  * This module is the ofono_gprs_driver implementation for rilmodem.
  *
@@ -300,6 +303,34 @@ static void ril_gprs_registration_status(struct ofono_gprs *gprs,
 	}
 }
 
+static void ril_radio_state_changed(struct ril_msg *message,
+							gpointer user_data)
+{
+	struct ofono_gprs *gprs = user_data;
+	struct ril_gprs_data *gd = ofono_gprs_get_data(gprs);
+	struct parcel rilp;
+	int radio_state;
+
+	if (gd->registered)
+		return;
+
+	g_ril_init_parcel(message, &rilp);
+	radio_state = parcel_r_int32(&rilp);
+
+	if (rilp.malformed) {
+		ofono_error("%s: malformed parcel received", __func__);
+		return;
+	}
+
+	g_ril_append_print_buf(gd->ril, "(state: %s)",
+				ril_radio_state_to_string(radio_state));
+	g_ril_print_unsol(gd->ril, message);
+
+	if (radio_state == RADIO_STATE_ON) {
+		query_max_cids(gprs);
+	}
+}
+
 static void query_max_cids_cb(struct ril_msg *message, gpointer user_data)
 {
 	struct ofono_gprs *gprs = user_data;
@@ -315,7 +346,14 @@ static void query_max_cids_cb(struct ril_msg *message, gpointer user_data)
 		ofono_error("%s: DATA_REGISTRATION_STATE reply failure: %s",
 				__func__,
 				ril_error_to_string(message->error));
-		goto error;
+
+		if (message->error != RIL_E_RADIO_NOT_AVAILABLE)
+			goto error;
+		else {
+		  g_ril_register(gd->ril, RIL_UNSOL_RESPONSE_RADIO_STATE_CHANGED,
+					ril_radio_state_changed, gprs);
+			return;
+		}
 	}
 
 	g_ril_init_parcel(message, &rilp);
@@ -341,6 +379,7 @@ reg_atom:
 	g_strfreev(strv);
 	ofono_gprs_set_cid_range(gprs, 1, max_calls);
 	ofono_gprs_register(gprs);
+	gd->registered = TRUE;
 	return;
 
 error_free:
@@ -380,6 +419,7 @@ static void query_max_cids(struct ofono_gprs *gprs)
 	if (g_ril_vendor(gd->ril) == OFONO_RIL_VENDOR_MTK) {
 		ofono_gprs_set_cid_range(gprs, 1, 3);
 		ofono_gprs_register(gprs);
+		gd->registered = TRUE;
 		return;
 	}
 
@@ -495,6 +535,7 @@ static int ril_gprs_probe(struct ofono_gprs *gprs, unsigned int vendor,
 	gd->ril = g_ril_clone(ril);
 	gd->ofono_attached = FALSE;
 	gd->rild_status = -1;
+	gd->registered = FALSE;
 
 	ofono_gprs_set_data(gprs, gd);
 
-- 
1.9.1

Re: [PATCH] rilmodem: Fix GPRS feature inavailable issue

[Tony Espy]

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

On 01/11/2016 10:42 PM, caiwen.zhang(a)intel.com wrote:
> From: Caiwen Zhang <caiwen.zhang@intel.com>
>
> When query max cid if rild radio status isn't RADIO_STATUS_ON, it may
> fail, gprs atom will be removed, gprs feature will always be inavailable.
> ---
>   drivers/rilmodem/gprs.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 42 insertions(+), 1 deletion(-)

Thanks for the patch Caiwen!

That said instead of extending the gprs atom to listen to radio state 
changes ( which may not reliably work on all rild-based devices ), it 
probably makes more sense to defer the creation of the gprs atom in the 
ril device plugin till the post_online phase ( vs. post_sim ).

Regards,
/tony


> diff --git a/drivers/rilmodem/gprs.c b/drivers/rilmodem/gprs.c
> index 0ec9d5f..f3bdc86 100644
> --- a/drivers/rilmodem/gprs.c
> +++ b/drivers/rilmodem/gprs.c
> @@ -54,8 +54,11 @@ struct ril_gprs_data {
>   	gboolean ofono_attached;
>   	int rild_status;
>   	int pending_deact_req;
> +	gboolean registered;
>   };
>
> +static void query_max_cids(struct ofono_gprs *gprs);
> +
>   /*
>    * This module is the ofono_gprs_driver implementation for rilmodem.
>    *
> @@ -300,6 +303,34 @@ static void ril_gprs_registration_status(struct ofono_gprs *gprs,
>   	}
>   }
>
> +static void ril_radio_state_changed(struct ril_msg *message,
> +							gpointer user_data)
> +{
> +	struct ofono_gprs *gprs = user_data;
> +	struct ril_gprs_data *gd = ofono_gprs_get_data(gprs);
> +	struct parcel rilp;
> +	int radio_state;
> +
> +	if (gd->registered)
> +		return;
> +
> +	g_ril_init_parcel(message, &rilp);
> +	radio_state = parcel_r_int32(&rilp);
> +
> +	if (rilp.malformed) {
> +		ofono_error("%s: malformed parcel received", __func__);
> +		return;
> +	}
> +
> +	g_ril_append_print_buf(gd->ril, "(state: %s)",
> +				ril_radio_state_to_string(radio_state));
> +	g_ril_print_unsol(gd->ril, message);
> +
> +	if (radio_state == RADIO_STATE_ON) {
> +		query_max_cids(gprs);
> +	}
> +}
> +
>   static void query_max_cids_cb(struct ril_msg *message, gpointer user_data)
>   {
>   	struct ofono_gprs *gprs = user_data;
> @@ -315,7 +346,14 @@ static void query_max_cids_cb(struct ril_msg *message, gpointer user_data)
>   		ofono_error("%s: DATA_REGISTRATION_STATE reply failure: %s",
>   				__func__,
>   				ril_error_to_string(message->error));
> -		goto error;
> +
> +		if (message->error != RIL_E_RADIO_NOT_AVAILABLE)
> +			goto error;
> +		else {
> +		  g_ril_register(gd->ril, RIL_UNSOL_RESPONSE_RADIO_STATE_CHANGED,
> +					ril_radio_state_changed, gprs);
> +			return;
> +		}
>   	}
>
>   	g_ril_init_parcel(message, &rilp);
> @@ -341,6 +379,7 @@ reg_atom:
>   	g_strfreev(strv);
>   	ofono_gprs_set_cid_range(gprs, 1, max_calls);
>   	ofono_gprs_register(gprs);
> +	gd->registered = TRUE;
>   	return;
>
>   error_free:
> @@ -380,6 +419,7 @@ static void query_max_cids(struct ofono_gprs *gprs)
>   	if (g_ril_vendor(gd->ril) == OFONO_RIL_VENDOR_MTK) {
>   		ofono_gprs_set_cid_range(gprs, 1, 3);
>   		ofono_gprs_register(gprs);
> +		gd->registered = TRUE;
>   		return;
>   	}
>
> @@ -495,6 +535,7 @@ static int ril_gprs_probe(struct ofono_gprs *gprs, unsigned int vendor,
>   	gd->ril = g_ril_clone(ril);
>   	gd->ofono_attached = FALSE;
>   	gd->rild_status = -1;
> +	gd->registered = FALSE;
>
>   	ofono_gprs_set_data(gprs, gd);
>
>

RE: [PATCH] rilmodem: Fix GPRS feature inavailable issue

[Zhang, Caiwen]

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

Hi tony,

Thanks for your reply. 

In fact I have ever considered the way you mentioned. But l found in most plugins, atoms (gprs, gprs_context, sms, 
call_forwarding ...) are created at post_sim phrase. It seems post_online phrase is more reasonable. There should
be some reason that the atom creation is brought forward. Does anyone know the reason?

Thanks,
Caiwen

> -----Original Message-----
> From: ofono [mailto:ofono-bounces(a)ofono.org] On Behalf Of Tony Espy
> Sent: Thursday, January 14, 2016 5:11 AM
> To: ofono(a)ofono.org
> Subject: Re: [PATCH] rilmodem: Fix GPRS feature inavailable issue
> 
> On 01/11/2016 10:42 PM, caiwen.zhang(a)intel.com wrote:
> > From: Caiwen Zhang <caiwen.zhang@intel.com>
> >
> > When query max cid if rild radio status isn't RADIO_STATUS_ON, it may
> > fail, gprs atom will be removed, gprs feature will always be inavailable.
> > ---
> >   drivers/rilmodem/gprs.c | 43
> ++++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 42 insertions(+), 1 deletion(-)
> 
> Thanks for the patch Caiwen!
> 
> That said instead of extending the gprs atom to listen to radio state
> changes ( which may not reliably work on all rild-based devices ), it
> probably makes more sense to defer the creation of the gprs atom in the
> ril device plugin till the post_online phase ( vs. post_sim ).
> 
> Regards,
> /tony
> 
> 
> > diff --git a/drivers/rilmodem/gprs.c b/drivers/rilmodem/gprs.c index
> > 0ec9d5f..f3bdc86 100644
> > --- a/drivers/rilmodem/gprs.c
> > +++ b/drivers/rilmodem/gprs.c
> > @@ -54,8 +54,11 @@ struct ril_gprs_data {
> >   	gboolean ofono_attached;
> >   	int rild_status;
> >   	int pending_deact_req;
> > +	gboolean registered;
> >   };
> >
> > +static void query_max_cids(struct ofono_gprs *gprs);
> > +
> >   /*
> >    * This module is the ofono_gprs_driver implementation for
> rilmodem.
> >    *
> > @@ -300,6 +303,34 @@ static void ril_gprs_registration_status(struct
> ofono_gprs *gprs,
> >   	}
> >   }
> >
> > +static void ril_radio_state_changed(struct ril_msg *message,
> > +							gpointer user_data)
> > +{
> > +	struct ofono_gprs *gprs = user_data;
> > +	struct ril_gprs_data *gd = ofono_gprs_get_data(gprs);
> > +	struct parcel rilp;
> > +	int radio_state;
> > +
> > +	if (gd->registered)
> > +		return;
> > +
> > +	g_ril_init_parcel(message, &rilp);
> > +	radio_state = parcel_r_int32(&rilp);
> > +
> > +	if (rilp.malformed) {
> > +		ofono_error("%s: malformed parcel received", __func__);
> > +		return;
> > +	}
> > +
> > +	g_ril_append_print_buf(gd->ril, "(state: %s)",
> > +				ril_radio_state_to_string(radio_state));
> > +	g_ril_print_unsol(gd->ril, message);
> > +
> > +	if (radio_state == RADIO_STATE_ON) {
> > +		query_max_cids(gprs);
> > +	}
> > +}
> > +
> >   static void query_max_cids_cb(struct ril_msg *message, gpointer
> user_data)
> >   {
> >   	struct ofono_gprs *gprs = user_data; @@ -315,7 +346,14 @@
> static
> > void query_max_cids_cb(struct ril_msg *message, gpointer user_data)
> >   		ofono_error("%s: DATA_REGISTRATION_STATE reply
> failure: %s",
> >   				__func__,
> >   				ril_error_to_string(message->error));
> > -		goto error;
> > +
> > +		if (message->error != RIL_E_RADIO_NOT_AVAILABLE)
> > +			goto error;
> > +		else {
> > +		  g_ril_register(gd->ril,
> RIL_UNSOL_RESPONSE_RADIO_STATE_CHANGED,
> > +					ril_radio_state_changed, gprs);
> > +			return;
> > +		}
> >   	}
> >
> >   	g_ril_init_parcel(message, &rilp);
> > @@ -341,6 +379,7 @@ reg_atom:
> >   	g_strfreev(strv);
> >   	ofono_gprs_set_cid_range(gprs, 1, max_calls);
> >   	ofono_gprs_register(gprs);
> > +	gd->registered = TRUE;
> >   	return;
> >
> >   error_free:
> > @@ -380,6 +419,7 @@ static void query_max_cids(struct ofono_gprs
> *gprs)
> >   	if (g_ril_vendor(gd->ril) == OFONO_RIL_VENDOR_MTK) {
> >   		ofono_gprs_set_cid_range(gprs, 1, 3);
> >   		ofono_gprs_register(gprs);
> > +		gd->registered = TRUE;
> >   		return;
> >   	}
> >
> > @@ -495,6 +535,7 @@ static int ril_gprs_probe(struct ofono_gprs
> *gprs, unsigned int vendor,
> >   	gd->ril = g_ril_clone(ril);
> >   	gd->ofono_attached = FALSE;
> >   	gd->rild_status = -1;
> > +	gd->registered = FALSE;
> >
> >   	ofono_gprs_set_data(gprs, gd);
> >
> >
> 
> _______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> https://lists.ofono.org/mailman/listinfo/ofono

Re: [PATCH] rilmodem: Fix GPRS feature inavailable issue

[Denis Kenzior]

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

Hi Caiwen,

Please don't top-post on this list.  Thanks for your understanding.

Now as to your question:

 >
> In fact I have ever considered the way you mentioned. But l found in most plugins, atoms (gprs, gprs_context, sms,
> call_forwarding ...) are created at post_sim phrase. It seems post_online phrase is more reasonable. There should
> be some reason that the atom creation is brought forward. Does anyone know the reason?
>

It really depends on what the particular atom supports and what the 
hardware supports.  The modem driver has some leeway to place atoms in 
the appropriate state callback (.pre_sim, .post_sim, .post_online).

Generally, anything that accesses the SIM can safely go into .post_sim 
if the hardware supports this mode properly, e.g. sim on, rx/tx radio 
circuits off.  sim and gprs atoms also support being in post_sim.  This 
is done so that settings can be tweaked while offline.

In the case of the rilmodem gprs driver, the querying of max_cids is 
needed in order to tell oFono core the id range.  This operation needs 
to be done once before the atom is registered.  If this operation is not 
available on the RIL in 'post_sim' (not online) state, then it is  safer 
to move the gprs atom into post_online callback than trying to follow 
radio state changes and (repeatedly) query max_cids.

If you really need gprs to be in post_sim, then we can discuss some 
alternative solutions.

Regards,
-Denis

Re: [PATCH] rilmodem: Fix GPRS feature inavailable issue

[Tony Espy]

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

On 01/14/2016 01:14 AM, Zhang, Caiwen wrote:
> Hi tony,
>
> Thanks for your reply.
>
> In fact I have ever considered the way you mentioned. But l found in most plugins, atoms (gprs, gprs_context, sms,
> call_forwarding ...) are created at post_sim phrase. It seems post_online phrase is more reasonable. There should
> be some reason that the atom creation is brought forward. Does anyone know the reason?

Two other potential solutions...

1) Fix the modem/fw and/or rild implementation to allow querying of 
max_cids even when the radio is off.

2) It's doubtful that the number of cids available actually changes over 
time, so it might be acceptable to add a vendor quirk and just hard-code 
the value for this modem as is done with MTK modems.  A slightly more 
conservative approach would be to do this in the query_cids callback if 
the operation fails.

Regards,
/tony