Hi Sergey,

On 8/15/20 4:43 PM, Sergey Matyukevich wrote:
> Function gemalto_modem_ready attempts to restart AT chat data->app
> after  incomplete shutdown. As a result, new AT chat does not work
> as expected loosing AT commands.
> 
> Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
> ---
>   plugins/gemalto.c | 8 ++++++++
>   1 file changed, 8 insertions(+)

Patch 2, 3 look good to me.

For this one, what is it trying to do actually?

> 
> diff --git a/plugins/gemalto.c b/plugins/gemalto.c
> index 238c7cc4..321c8c1b 100644
> --- a/plugins/gemalto.c
> +++ b/plugins/gemalto.c
> @@ -222,6 +222,8 @@ static void sim_state_cb(gboolean present, gpointer user_data)
>   	struct ofono_modem *modem = user_data;
>   	struct gemalto_data *data = ofono_modem_get_data(modem);
>   
> +	DBG("");
> +
>   	at_util_sim_state_query_free(data->sim_state_query);
>   	data->sim_state_query = NULL;
>   
> @@ -241,6 +243,8 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
>   	struct ofono_modem *modem = user_data;
>   	struct gemalto_data *data = ofono_modem_get_data(modem);
>   
> +	DBG("");
> +
>   	if (!ok) {
>   		g_at_chat_unref(data->app);
>   		data->app = NULL;
> @@ -451,6 +455,8 @@ static void gemalto_modem_ready(GAtResult *result, gpointer user_data)
>   	data->modem_ready_id = 0;
>   	data->trial_cmd_id = 0;
>   
> +	g_at_chat_cancel_all(data->app);
> +	g_at_chat_unregister_all(data->app);

These get called automatically on unref below, assuming  this is the last (it 
should be) reference.

>   	g_at_chat_unref(data->app);

Instead of this dance of opening / closing the chat in order to force-cancel the 
'AT' command queued in gemalto_enable(), maybe there should be something in 
GAtChat itself to handle this case.  Either a 'force cancel the top of the queue 
because I know the modem is broken' or maybe some form of 'send this on the port 
until it responds properly'.  Alternatively, you could simply use GAtIO and a 
timer to write stuff to the port until it responds and only create the chat at 
that time.

There's already something a bit similar in the form of 
g_at_chat_set_wakeup_command().  It was used on the old Neo Freerunner modem 
which would go to 'sleep' after several seconds of inactivity.  So the 
subsequent AT command would get eaten.  You basically had to poke it with an 
empty command, have it timeout / return OK and then you could go on with 
submitting AT commands again.  But I don't think it is a good fit for this 
particular case.

>   
>   	data->app = open_device(app);
> @@ -466,6 +472,8 @@ static void gemalto_at_cb(gboolean ok, GAtResult *result, gpointer user_data)
>   	struct ofono_modem *modem = user_data;
>   	struct gemalto_data *data = ofono_modem_get_data(modem);
>   
> +	DBG("");
> +
>   	g_at_chat_unregister(data->app, data->modem_ready_id);
>   	data->modem_ready_id = 0;
>   
> 

Regards,
-Denis