[PATCH v7] ifx: Adding modem selftest for Infineon modem

[PATCH v7] ifx: Adding modem selftest for Infineon modem

[Robertino Benis]

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

Infineon modem selftest, during ifx_enable().
Two steps trigger, with timeout. In case one
fails, modem will not power up.

---
 plugins/ifx.c |   50 +++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/plugins/ifx.c b/plugins/ifx.c
index 411c012..da8ed0b 100644
--- a/plugins/ifx.c
+++ b/plugins/ifx.c
@@ -72,6 +72,8 @@
 #define GPRS3_DLC   4
 #define AUX_DLC     5
 
+#define IFX_SETUP_TIMEOUT	10
+
 static char *dlc_prefixes[NUM_DLC] = { "Voice: ", "Net: ", "GPRS1: ",
 					"GPRS2: ", "GPRS3: ", "Aux: " };
 
@@ -524,10 +526,12 @@ static gboolean mux_timeout_cb(gpointer user_data)
 	struct ofono_modem *modem = user_data;
 	struct ifx_data *data = ofono_modem_get_data(modem);
 
-	ofono_error("Timeout with multiplexer setup");
+	ofono_error("Timeout with AT command setup");
 
 	data->mux_init_timeout = 0;
 
+	g_at_chat_cancel_all(data->dlcs[AUX_DLC]);
+
 	g_at_chat_unref(data->dlcs[AUX_DLC]);
 	data->dlcs[AUX_DLC] = NULL;
 
@@ -539,6 +543,35 @@ static gboolean mux_timeout_cb(gpointer user_data)
 	return FALSE;
 }
 
+static void dev_ver_selftest_cb(gboolean ok, GAtResult *result,
+				gpointer user_data)
+{
+
+	struct ofono_modem *modem = user_data;
+	struct ifx_data *data = ofono_modem_get_data(modem);
+
+	if (!ok) {
+		ofono_error("ERROR:IFX Selftest at(a)vers:device_version_id()"
+				"-FAILED");
+
+		g_at_chat_cancel_all(data->dlcs[AUX_DLC]);
+	}
+}
+
+static void rtc_gti_selftest_cb(gboolean ok, GAtResult *result,
+			gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct ifx_data *data = ofono_modem_get_data(modem);
+
+	if (!ok) {
+		ofono_error("ERROR:IFX Selftest"
+			"at(a)rtc:rtc_gti_test_verify_32khz()-FAILED");
+
+		g_at_chat_cancel_all(data->dlcs[AUX_DLC]);
+	}
+}
+
 static int ifx_enable(struct ofono_modem *modem)
 {
 	struct ifx_data *data = ofono_modem_get_data(modem);
@@ -592,13 +625,24 @@ static int ifx_enable(struct ofono_modem *modem)
 	g_at_chat_send(chat, "ATE0 +CMEE=1", NULL,
 					NULL, NULL, NULL);
 
+	/* Execute Modem Self tests */
+	data->dlcs[AUX_DLC] = chat;
+
+	g_at_chat_send(data->dlcs[AUX_DLC],
+		"at(a)rtc:rtc_gti_test_verify_32khz()",
+		NULL, rtc_gti_selftest_cb, modem, NULL);
+
+	g_at_chat_send(data->dlcs[AUX_DLC], "at(a)vers:device_version_id()",
+		NULL, dev_ver_selftest_cb, modem, NULL);
+
+	/* Enable  MUX Channels */
 	data->frame_size = 1509;
 
 	g_at_chat_send(chat, "AT+CMUX=0,0,,1509,10,3,30,,", NULL,
 					mux_setup_cb, modem, NULL);
 
-	data->mux_init_timeout = g_timeout_add_seconds(5, mux_timeout_cb,
-								modem);
+	data->mux_init_timeout = g_timeout_add_seconds(
+		IFX_SETUP_TIMEOUT, mux_timeout_cb, modem);
 
 	data->dlcs[AUX_DLC] = chat;
 
-- 
1.7.1

Re: [PATCH v7] ifx: Adding modem selftest for Infineon modem

[Marcel Holtmann]

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

Hi Robertino,

> Infineon modem selftest, during ifx_enable().
> Two steps trigger, with timeout. In case one
> fails, modem will not power up.
> 
> ---
>  plugins/ifx.c |   50 +++++++++++++++++++++++++++++++++++++++++++++++---
>  1 files changed, 47 insertions(+), 3 deletions(-)
> 
> diff --git a/plugins/ifx.c b/plugins/ifx.c
> index 411c012..da8ed0b 100644
> --- a/plugins/ifx.c
> +++ b/plugins/ifx.c
> @@ -72,6 +72,8 @@
>  #define GPRS3_DLC   4
>  #define AUX_DLC     5
>  
> +#define IFX_SETUP_TIMEOUT	10
> +
>  static char *dlc_prefixes[NUM_DLC] = { "Voice: ", "Net: ", "GPRS1: ",
>  					"GPRS2: ", "GPRS3: ", "Aux: " };
>  
> @@ -524,10 +526,12 @@ static gboolean mux_timeout_cb(gpointer user_data)
>  	struct ofono_modem *modem = user_data;
>  	struct ifx_data *data = ofono_modem_get_data(modem);
>  
> -	ofono_error("Timeout with multiplexer setup");
> +	ofono_error("Timeout with AT command setup");

call this "Timeout with modem and multiplexer setup" now.

>  	data->mux_init_timeout = 0;
>  
> +	g_at_chat_cancel_all(data->dlcs[AUX_DLC]);
> +

This one is not needed. Since first it is the last command anyway. There
is nothing else in the queue.

>  	g_at_chat_unref(data->dlcs[AUX_DLC]);

And second this will do it anyway for you.

>  	data->dlcs[AUX_DLC] = NULL;
>  
> @@ -539,6 +543,35 @@ static gboolean mux_timeout_cb(gpointer user_data)
>  	return FALSE;
>  }
>  
> +static void dev_ver_selftest_cb(gboolean ok, GAtResult *result,
> +				gpointer user_data)
> +{
> +
> +	struct ofono_modem *modem = user_data;
> +	struct ifx_data *data = ofono_modem_get_data(modem);
> +
> +	if (!ok) {
> +		ofono_error("ERROR:IFX Selftest at(a)vers:device_version_id()"
> +				"-FAILED");
> +
> +		g_at_chat_cancel_all(data->dlcs[AUX_DLC]);
> +	}
> +}

I would just do here this:

	if (ok)
		return;

	ofono_error(...

However you now need to clean up properly.

	if (data->mux_init_timeout > 0) {
		g_source_remove(data->mux_init_timeout);
		data->mux_init_timeout = 0;
	}

	g_at_chat_unref(data->dlcs[AUX_DLC]);
	data->dlcs[AUX_DLC] = NULL;

	g_io_channel_unref(data->device);
	data->device = NULL;

	ofono_modem_set_powered(modem, FALSE);

If you don't do that you leak memory and a file descriptor. And in
addition the user has to wait for the setup timeout, even if it is
already clear that the enabling failed.

> +
> +static void rtc_gti_selftest_cb(gboolean ok, GAtResult *result,
> +			gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct ifx_data *data = ofono_modem_get_data(modem);
> +
> +	if (!ok) {
> +		ofono_error("ERROR:IFX Selftest"
> +			"at(a)rtc:rtc_gti_test_verify_32khz()-FAILED");
> +
> +		g_at_chat_cancel_all(data->dlcs[AUX_DLC]);
> +	}
> +}
> +
>  static int ifx_enable(struct ofono_modem *modem)
>  {
>  	struct ifx_data *data = ofono_modem_get_data(modem);
> @@ -592,13 +625,24 @@ static int ifx_enable(struct ofono_modem *modem)
>  	g_at_chat_send(chat, "ATE0 +CMEE=1", NULL,
>  					NULL, NULL, NULL);
>  
> +	/* Execute Modem Self tests */
> +	data->dlcs[AUX_DLC] = chat;

Don't bother moving this one around. It is fine where it is. There is no
race condition here. We are single threaded and we need to go back to
the mainloop to write the commands before this will be called.

> +	g_at_chat_send(data->dlcs[AUX_DLC],
> +		"at(a)rtc:rtc_gti_test_verify_32khz()",
> +		NULL, rtc_gti_selftest_cb, modem, NULL);
> +
> +	g_at_chat_send(data->dlcs[AUX_DLC], "at(a)vers:device_version_id()",
> +		NULL, dev_ver_selftest_cb, modem, NULL);
> +
> +	/* Enable  MUX Channels */
>  	data->frame_size = 1509;
>  
>  	g_at_chat_send(chat, "AT+CMUX=0,0,,1509,10,3,30,,", NULL,
>  					mux_setup_cb, modem, NULL);
>  
> -	data->mux_init_timeout = g_timeout_add_seconds(5, mux_timeout_cb,
> -								modem);
> +	data->mux_init_timeout = g_timeout_add_seconds(
> +		IFX_SETUP_TIMEOUT, mux_timeout_cb, modem);

To be honest the define with the setup timeout value does not help here.
Just change this to 10 and put a comment section above the the timeout
command to state what is expected execution time of the test commands.
That makes this a lot clearer to understand.
 
>  	data->dlcs[AUX_DLC] = chat;

Regards

Marcel