Re: [PATCH] Add GPRS, mux, volume control support in SIMCOM sim900 modem module

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Renat,

On 03/12/2013 09:37 AM, r.r.zaripov(a)gmail.com wrote:
> From: Renat Zaripov<r.r.zaripov@gmail.com>
>
> ---
>   plugins/sim900.c |  201 ++++++++++++++++++++++++++++++++++++++++++++++--------
>   1 file changed, 171 insertions(+), 30 deletions(-)
>

I applied this patch, however here are a few comments:

> @@ -119,10 +132,32 @@ static GAtChat *open_device(struct ofono_modem *modem,
>   	if (channel == NULL)
>   		return NULL;
>
> +	data->device = channel;
>   	syntax = g_at_syntax_new_gsm_permissive();
>   	chat = g_at_chat_new(channel, syntax);
>   	g_at_syntax_unref(syntax);
>
> +	if (chat == NULL)
> +		return NULL;

In this case you are not destroying the data->device object.  This is a 
potential memory leak.  I fixed this for you in commit 
b4518caa50f03b40e9f9434dc17558665bb8ee3f

> +
> +	if (getenv("OFONO_AT_DEBUG"))
> +		g_at_chat_set_debug(chat, sim900_debug, debug);
> +
> +	return chat;
> +}
> +

<snip>

> @@ -134,6 +169,96 @@ static GAtChat *open_device(struct ofono_modem *modem,
>   	return chat;
>   }
>
> +static void shutdown_device(struct sim900_data *data)
> +{
> +	int i, fd;

make --no-print-directory all-am
   CC     plugins/sim900.o
cc1: warnings being treated as errors
plugins/sim900.c: In function ‘shutdown_device’:
plugins/sim900.c:180:9: error: unused variable ‘fd’
make[1]: *** [plugins/sim900.o] Error 1
make: *** [all] Error 2

Please 'make distcheck' before submitting patches.

> +
> +	DBG("");
> +
> +	for (i = 0; i<  NUM_DLC; i++) {
> +		if (data->dlcs[i] == NULL)
> +			continue;
> +
> +		g_at_chat_unref(data->dlcs[i]);
> +		data->dlcs[i] = NULL;
> +	}
> +
> +	if (data->mux) {
> +		g_at_mux_shutdown(data->mux);
> +		g_at_mux_unref(data->mux);
> +		data->mux = NULL;
> +		goto done;

This goto seems redundant.  I got rid of this for you in commit 
548611e93948902ccffaf5568dcd31bef1d4f764.

> +	}
> +
> +done:
> +	g_io_channel_unref(data->device);
> +	data->device = NULL;
> +}
> +

<snip>

>   static void cfun_disable(gboolean ok, GAtResult *result, gpointer user_data)
>   {
>   	struct ofono_modem *modem = user_data;
> @@ -181,8 +307,11 @@ static void cfun_disable(gboolean ok, GAtResult *result, gpointer user_data)
>
>   	DBG("");
>
> -	g_at_chat_unref(data->modem);
> -	data->modem = NULL;
> +	g_at_mux_shutdown(data->mux);
> +	g_at_mux_unref(data->mux);
> +
> +	g_at_chat_unref(data->dlcs[SETUP_DLC]);
> +	data->dlcs[SETUP_DLC] = NULL;
>
>   	if (ok)
>   		ofono_modem_set_powered(modem, FALSE);


> @@ -194,12 +323,11 @@ static int sim900_disable(struct ofono_modem *modem)
>
>   	DBG("%p", modem);
>
> -	g_at_chat_cancel_all(data->modem);
> -	g_at_chat_unregister_all(data->modem);
> -
> -	g_at_chat_send(data->modem, "AT+CFUN=4", none_prefix,
> +	g_at_chat_send(data->dlcs[SETUP_DLC], "AT+CFUN=4", none_prefix,
>   					cfun_disable, modem, NULL);
>
> +	shutdown_device(data);
> +

Calling shutdown_device here is premature.  I bet that you're never 
actually sending the CFUN=4 with this setup because you close the tty 
before that happens.  I fixed this in commit 
93ac1669a069a1bcce1ed121ca60977db53abf4e.  If you disagree, please let 
me know.

>   	return -EINPROGRESS;
>   }
>

Regards,
-Denis