Re: [RFC_v1 1/2] sierra: Fix memory leak

Re: [RFC_v1 1/2] sierra: Fix memory leak

[Denis Kenzior]

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

Hi Guillaume,

On 12/01/2011 10:48 AM, Guillaume Zajac wrote:
> ---
>  plugins/sierra.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 

Patch has been applied, thanks.

Regards,
-Denis

Re: [RFC_v1 2/2] sierra: Add SIM state polling

[Denis Kenzior]

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

Hi Guillaume,

On 12/01/2011 10:48 AM, Guillaume Zajac wrote:
> It has been reported than for some Sierra modem, the SIM
> initialization process leads to +CME ERROR: 10 sim not
> present. The issue might come from SIM is not ready.

Do you have an AT log of these reports?

> ---
>  plugins/sierra.c |   26 +++++++++++++++++++++++++-
>  1 files changed, 25 insertions(+), 1 deletions(-)
> 
> diff --git a/plugins/sierra.c b/plugins/sierra.c
> index b33382d..45e75c1 100644
> --- a/plugins/sierra.c
> +++ b/plugins/sierra.c
> @@ -47,6 +47,8 @@ static const char *none_prefix[] = { NULL };
>  
>  struct sierra_data {
>  	GAtChat *chat;
> +	gboolean have_sim;
> +	struct at_util_sim_state_query *sim_state_query;
>  };
>  
>  static void sierra_debug(const char *str, void *user_data)
> @@ -82,6 +84,9 @@ static void sierra_remove(struct ofono_modem *modem)
>  	/* Cleanup after hot-unplug */
>  	g_at_chat_unref(data->chat);
>  
> +	/* Cleanup potential SIM state polling */
> +	at_util_sim_state_query_free(data->sim_state_query);
> +
>  	g_free(data);
>  }
>  
> @@ -118,6 +123,21 @@ static GAtChat *open_device(struct ofono_modem *modem,
>  	return chat;
>  }
>  
> +static void sim_state_cb(gboolean present, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct sierra_data *data = ofono_modem_get_data(modem);
> +
> +	at_util_sim_state_query_free(data->sim_state_query);
> +	data->sim_state_query = NULL;
> +
> +	data->have_sim = present;
> +
> +	ofono_modem_set_powered(modem, TRUE);
> +
> +	g_at_chat_send(data->chat, "AT&C0", NULL, NULL, NULL, NULL);

Fair enough, however:

- You are not using data->have_sim anywhere, presumably you want to use
it in sierra_pre_sim to notify the sim inserted status...
- The sierra driver does not instantiate any gprs_context drivers, so it
isn't even capable of GPRS in its current state.  Sending the &C0
command is only needed if you are planning to use PPP and probably
belongs in a separate patch.

> +}
> +
>  static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
>  {
>  	struct ofono_modem *modem = user_data;
> @@ -128,9 +148,13 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
>  	if (!ok) {
>  		g_at_chat_unref(data->chat);
>  		data->chat = NULL;
> +
> +		ofono_modem_set_powered(modem, FALSE);
> +		return;
>  	}
>  
> -	ofono_modem_set_powered(modem, ok);
> +	data->sim_state_query = at_util_sim_state_query_new(data->chat,
> +						2, 20, sim_state_cb, modem);
>  }
>  
>  static int sierra_enable(struct ofono_modem *modem)

Regards,
-Denis

[RFC_v1 0/2] Add SIM polling mechanism for Sierra

[Guillaume Zajac]

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

Hi,

Change log from v0:
	- Separate memory leak fix in another patch
	- Fix issue into polling mechanism

Kind regards,
Guillaume

Guillaume Zajac (2):
  sierra: Fix memory leak
  sierra: Add SIM state polling

 plugins/sierra.c |   29 ++++++++++++++++++++++++++++-
 1 files changed, 28 insertions(+), 1 deletions(-)

[RFC_v1 1/2] sierra: Fix memory leak

[Guillaume Zajac]

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

---
 plugins/sierra.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/plugins/sierra.c b/plugins/sierra.c
index c41e2d1..b33382d 100644
--- a/plugins/sierra.c
+++ b/plugins/sierra.c
@@ -79,6 +79,9 @@ static void sierra_remove(struct ofono_modem *modem)
 
 	ofono_modem_set_data(modem, NULL);
 
+	/* Cleanup after hot-unplug */
+	g_at_chat_unref(data->chat);
+
 	g_free(data);
 }
 
-- 
1.7.1

[RFC_v1 2/2] sierra: Add SIM state polling

[Guillaume Zajac]

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

It has been reported than for some Sierra modem, the SIM
initialization process leads to +CME ERROR: 10 sim not
present. The issue might come from SIM is not ready.
---
 plugins/sierra.c |   26 +++++++++++++++++++++++++-
 1 files changed, 25 insertions(+), 1 deletions(-)

diff --git a/plugins/sierra.c b/plugins/sierra.c
index b33382d..45e75c1 100644
--- a/plugins/sierra.c
+++ b/plugins/sierra.c
@@ -47,6 +47,8 @@ static const char *none_prefix[] = { NULL };
 
 struct sierra_data {
 	GAtChat *chat;
+	gboolean have_sim;
+	struct at_util_sim_state_query *sim_state_query;
 };
 
 static void sierra_debug(const char *str, void *user_data)
@@ -82,6 +84,9 @@ static void sierra_remove(struct ofono_modem *modem)
 	/* Cleanup after hot-unplug */
 	g_at_chat_unref(data->chat);
 
+	/* Cleanup potential SIM state polling */
+	at_util_sim_state_query_free(data->sim_state_query);
+
 	g_free(data);
 }
 
@@ -118,6 +123,21 @@ static GAtChat *open_device(struct ofono_modem *modem,
 	return chat;
 }
 
+static void sim_state_cb(gboolean present, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct sierra_data *data = ofono_modem_get_data(modem);
+
+	at_util_sim_state_query_free(data->sim_state_query);
+	data->sim_state_query = NULL;
+
+	data->have_sim = present;
+
+	ofono_modem_set_powered(modem, TRUE);
+
+	g_at_chat_send(data->chat, "AT&C0", NULL, NULL, NULL, NULL);
+}
+
 static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
 {
 	struct ofono_modem *modem = user_data;
@@ -128,9 +148,13 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
 	if (!ok) {
 		g_at_chat_unref(data->chat);
 		data->chat = NULL;
+
+		ofono_modem_set_powered(modem, FALSE);
+		return;
 	}
 
-	ofono_modem_set_powered(modem, ok);
+	data->sim_state_query = at_util_sim_state_query_new(data->chat,
+						2, 20, sim_state_cb, modem);
 }
 
 static int sierra_enable(struct ofono_modem *modem)
-- 
1.7.1

Re: [RFC_v1 2/2] sierra: Add SIM state polling

[Denis Kenzior]

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

Hi Guillaume,

>> - The sierra driver does not instantiate any gprs_context drivers, so it
>> isn't even capable of GPRS in its current state.  Sending the&C0
>> command is only needed if you are planning to use PPP and probably
>> belongs in a separate patch.
>>
> 
> I didn't see the gprs context was never created into sierra plugin, do
> we have a reason for that?
> 

Probably just an oversight.

Regards,
-Denis

Re: [RFC_v1 2/2] sierra: Add SIM state polling

[Guillaume Zajac]

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

Hi Denis,

On 29/11/2011 18:36, Denis Kenzior wrote:
> Hi Guillaume,
>
> On 12/01/2011 10:48 AM, Guillaume Zajac wrote:
>> It has been reported than for some Sierra modem, the SIM
>> initialization process leads to +CME ERROR: 10 sim not
>> present. The issue might come from SIM is not ready.
> Do you have an AT log of these reports?

Yes I had one but it was given to me through pastebin from IRC.
As soon as I see the guy on #ofono, I will ask him to send the log over 
oFono mailing list.

>> ---
>>   plugins/sierra.c |   26 +++++++++++++++++++++++++-
>>   1 files changed, 25 insertions(+), 1 deletions(-)
>>
>> diff --git a/plugins/sierra.c b/plugins/sierra.c
>> index b33382d..45e75c1 100644
>> --- a/plugins/sierra.c
>> +++ b/plugins/sierra.c
>> @@ -47,6 +47,8 @@ static const char *none_prefix[] = { NULL };
>>
>>   struct sierra_data {
>>   	GAtChat *chat;
>> +	gboolean have_sim;
>> +	struct at_util_sim_state_query *sim_state_query;
>>   };
>>
>>   static void sierra_debug(const char *str, void *user_data)
>> @@ -82,6 +84,9 @@ static void sierra_remove(struct ofono_modem *modem)
>>   	/* Cleanup after hot-unplug */
>>   	g_at_chat_unref(data->chat);
>>
>> +	/* Cleanup potential SIM state polling */
>> +	at_util_sim_state_query_free(data->sim_state_query);
>> +
>>   	g_free(data);
>>   }
>>
>> @@ -118,6 +123,21 @@ static GAtChat *open_device(struct ofono_modem *modem,
>>   	return chat;
>>   }
>>
>> +static void sim_state_cb(gboolean present, gpointer user_data)
>> +{
>> +	struct ofono_modem *modem = user_data;
>> +	struct sierra_data *data = ofono_modem_get_data(modem);
>> +
>> +	at_util_sim_state_query_free(data->sim_state_query);
>> +	data->sim_state_query = NULL;
>> +
>> +	data->have_sim = present;
>> +
>> +	ofono_modem_set_powered(modem, TRUE);
>> +
>> +	g_at_chat_send(data->chat, "AT&C0", NULL, NULL, NULL, NULL);
> Fair enough, however:
>
> - You are not using data->have_sim anywhere, presumably you want to use
> it in sierra_pre_sim to notify the sim inserted status...

Yes, I forgot to use it.

> - The sierra driver does not instantiate any gprs_context drivers, so it
> isn't even capable of GPRS in its current state.  Sending the&C0
> command is only needed if you are planning to use PPP and probably
> belongs in a separate patch.
>

I didn't see the gprs context was never created into sierra plugin, do 
we have a reason for that?

>> +}
>> +
>>   static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
>>   {
>>   	struct ofono_modem *modem = user_data;
>> @@ -128,9 +148,13 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
>>   	if (!ok) {
>>   		g_at_chat_unref(data->chat);
>>   		data->chat = NULL;
>> +
>> +		ofono_modem_set_powered(modem, FALSE);
>> +		return;
>>   	}
>>
>> -	ofono_modem_set_powered(modem, ok);
>> +	data->sim_state_query = at_util_sim_state_query_new(data->chat,
>> +						2, 20, sim_state_cb, modem);
>>   }
>>
>>   static int sierra_enable(struct ofono_modem *modem)
> Regards,
> -Denis
>

Kind regards,
Guillaume

Re: [RFC_v1 2/2] sierra: Add SIM state polling

[Marcel Holtmann]

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

Hi Denis,

> >> - The sierra driver does not instantiate any gprs_context drivers, so it
> >> isn't even capable of GPRS in its current state.  Sending the&C0
> >> command is only needed if you are planning to use PPP and probably
> >> belongs in a separate patch.
> >>
> > 
> > I didn't see the gprs context was never created into sierra plugin, do
> > we have a reason for that?
> > 
> 
> Probably just an oversight.

actually the TTY allocation for the Sierra modems are a bit funny. They
have one channel that is capable of doing PPP and the others are just
app1, app2, app3 channels. However only when issuing ATI it will tell
you what channel is which.

Regards

Marcel