[PATCH 1/2] qmimodem: Rework GET_CARD_STATUS retry logic

[PATCH 1/2] qmimodem: Rework GET_CARD_STATUS retry logic

[Denis Kenzior]

Port the retry logic from glib based implementation that uses
g_timeout_add to use l_timeout instead.  While here, fix the existing
logic to not leak memory when a retry is performed.  This happens in one
of two ways:

- When retry_cbd is allocated via cb_data_new, it is never freed when
  the timeout GSource fires.
- If the timeout GSource is removed early (i.e. due to remove() being
  called), the associated retry_cbd is not freed

Fix this by using cb_data_ref / cb_data_unref and utilize destroy
callbacks properly.
---
 drivers/qmimodem/sim.c | 86 +++++++++++++++++++++++-------------------
 1 file changed, 47 insertions(+), 39 deletions(-)

diff --git a/drivers/qmimodem/sim.c b/drivers/qmimodem/sim.c
index 8605e03c6f4f..e561e269f1f2 100644
--- a/drivers/qmimodem/sim.c
+++ b/drivers/qmimodem/sim.c
@@ -64,13 +64,10 @@ struct sim_data {
 	uint32_t event_mask;
 	uint8_t app_type;
 	uint32_t retry_count;
-	guint poll_source;
+	struct l_timeout *retry_timer;
 	uint16_t card_status_indication_id;
 };
 
-static void qmi_query_passwd_state(struct ofono_sim *sim,
-				ofono_sim_passwd_cb_t cb, void *user_data);
-
 static int create_fileid_data(uint8_t app_type, int fileid,
 					const unsigned char *path,
 					unsigned int path_len,
@@ -574,22 +571,28 @@ static enum get_card_status_result handle_get_card_status_result(
 	return handle_get_card_status_data(result, sim_stat);
 }
 
-static gboolean query_passwd_state_retry(gpointer userdata)
+static void query_passwd_state_cb(struct qmi_result *result, void *user_data);
+
+static void query_passwd_state_retry(struct l_timeout *timeout, void *user)
 {
-	struct cb_data *cbd = userdata;
+	struct cb_data *cbd = user;
 	ofono_sim_passwd_cb_t cb = cbd->cb;
 	struct ofono_sim *sim = cbd->user;
 	struct sim_data *data = ofono_sim_get_data(sim);
 
-	data->poll_source = 0;
+	if (qmi_service_send(data->uim, QMI_UIM_GET_CARD_STATUS, NULL,
+				query_passwd_state_cb, cbd, cb_data_unref) > 0) {
+		cb_data_ref(cbd);
+		return;
+	}
 
-	qmi_query_passwd_state(sim, cb, cbd->data);
+	CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
 
-	return FALSE;
+	l_timeout_remove(data->retry_timer);
+	data->retry_timer = NULL;
 }
 
-static void query_passwd_state_cb(struct qmi_result *result,
-					void *user_data)
+static void query_passwd_state_cb(struct qmi_result *result, void *user_data)
 {
 	struct cb_data *cbd = user_data;
 	ofono_sim_passwd_cb_t cb = cbd->cb;
@@ -597,49 +600,54 @@ static void query_passwd_state_cb(struct qmi_result *result,
 	struct sim_data *data = ofono_sim_get_data(sim);
 	struct sim_status sim_stat;
 	enum get_card_status_result res;
-	struct cb_data *retry_cbd;
 	unsigned int i;
 
 	for (i = 0; i < OFONO_SIM_PASSWORD_INVALID; i++)
 		sim_stat.retries[i] = -1;
 
 	res = handle_get_card_status_result(result, &sim_stat);
+	if (res == GET_CARD_STATUS_RESULT_TEMP_ERROR &&
+			++data->retry_count <= MAX_RETRY_COUNT) {
+		DBG("Retry command");
+
+		if (!data->retry_timer) {
+			cb_data_ref(cbd);
+			data->retry_timer = l_timeout_create_ms(20,
+					query_passwd_state_retry,
+					cbd, cb_data_unref);
+		} else
+			l_timeout_modify_ms(data->retry_timer, 20);
+
+		return;
+	}
+
+	l_timeout_remove(data->retry_timer);
+	data->retry_timer = NULL;
+	data->retry_count = 0;
+
 	switch (res) {
 	case GET_CARD_STATUS_RESULT_OK:
 		DBG("passwd state %d", sim_stat.passwd_state);
-		data->retry_count = 0;
-		if (sim_stat.passwd_state == OFONO_SIM_PASSWORD_INVALID) {
-			CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
-			ofono_sim_inserted_notify(sim, false);
-		} else
+
+		if (sim_stat.passwd_state != OFONO_SIM_PASSWORD_INVALID) {
 			CALLBACK_WITH_SUCCESS(cb, sim_stat.passwd_state,
 								cbd->data);
+			return;
+		}
+
 		break;
 	case GET_CARD_STATUS_RESULT_TEMP_ERROR:
-		data->retry_count++;
-		if (data->retry_count > MAX_RETRY_COUNT) {
-			DBG("Failed after %d attempts. Card state:%d",
-							data->retry_count,
-							sim_stat.card_state);
-			data->retry_count = 0;
-			CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
-			ofono_sim_inserted_notify(sim, false);
-		} else {
-			DBG("Retry command");
-			retry_cbd = cb_data_new(cb, cbd->data);
-			retry_cbd->user = sim;
-			data->poll_source = g_timeout_add(20,
-						query_passwd_state_retry,
-						retry_cbd);
-		}
+		DBG("Failed after %d attempts. Card state:%d",
+						data->retry_count,
+						sim_stat.card_state);
 		break;
 	case GET_CARD_STATUS_RESULT_ERROR:
 		DBG("Command failed");
-		data->retry_count = 0;
-		CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
-		ofono_sim_inserted_notify(sim, false);
 		break;
 	}
+
+	CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
+	ofono_sim_inserted_notify(sim, false);
 }
 
 static void qmi_query_passwd_state(struct ofono_sim *sim,
@@ -653,7 +661,7 @@ static void qmi_query_passwd_state(struct ofono_sim *sim,
 	cbd->user = sim;
 
 	if (qmi_service_send(data->uim, QMI_UIM_GET_CARD_STATUS, NULL,
-					query_passwd_state_cb, cbd, l_free) > 0)
+				query_passwd_state_cb, cbd, cb_data_unref) > 0)
 		return;
 
 	CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
@@ -920,8 +928,8 @@ static void qmi_sim_remove(struct ofono_sim *sim)
 
 	ofono_sim_set_data(sim, NULL);
 
-	if (data->poll_source > 0)
-		g_source_remove(data->poll_source);
+	l_timeout_remove(data->retry_timer);
+	data->retry_timer = NULL;
 
 	if (data->uim) {
 		if (data->card_status_indication_id) {
-- 
2.43.0

[PATCH 2/2] qmimodem: sms: Silence valgrind warning

[Denis Kenzior]

ofonod[2670789]: drivers/qmimodem/sms.c:get_msg_protocol_cb()
==2670789== Conditional jump or move depends on uninitialised value(s)
==2670789==    at 0x46552A: get_msg_protocol_cb (sms.c:565)
==2670789==    by 0x45D5C1: service_send_callback (qmi.c:2762)
==2670789==    by 0x4594F5: __rx_message (qmi.c:846)
==2670789==    by 0x45A6A4: received_qmux_data (qmi.c:1393)
==2670789==    by 0x58D71C: io_callback (io.c:105)
==2670789==    by 0x58C073: l_main_iterate (main.c:461)
==2670789==    by 0x500EC0: event_check (main.c:190)
==2670789==    by 0x48FC09D: ??? (in /usr/lib/libglib-2.0.so.0.7800.3)
==2670789==    by 0x49591CF: ??? (in /usr/lib/libglib-2.0.so.0.7800.3)
==2670789==    by 0x48FBB96: g_main_loop_run (in /usr/lib/libglib-2.0.so.0.7800.3)
==2670789==    by 0x5011F5: main (main.c:284)
==2670789==
ofonod[2670789]: drivers/qmimodem/sms.c:get_msg_list()

The warning is triggered because GET_MSG_PROTOCOL command succeeds and
qmi_result_set_error() returns false.  It seems the intent in this case
is to use the msg_mode reported by the device by obtaining it using
qmi_result_get_uint8.  In case GET_MSG_PROTOCOL command fails, both CDMA
and WCDMA messages should be queried.
---
 drivers/qmimodem/sms.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/qmimodem/sms.c b/drivers/qmimodem/sms.c
index 498a6d778303..b9fe963520d6 100644
--- a/drivers/qmimodem/sms.c
+++ b/drivers/qmimodem/sms.c
@@ -556,21 +556,23 @@ static void get_msg_protocol_cb(struct qmi_result *result, void *user_data)
 
 	DBG("");
 
-	if (qmi_result_set_error(result, &err) &&
-			(err != QMI_ERR_OP_DEVICE_UNSUPPORTED)) {
-		DBG("Err: protocol %d - %s", err, qmi_result_get_error(result));
-		return;
-	}
+	if (qmi_result_set_error(result, &err)) {
+		if (err != QMI_ERR_OP_DEVICE_UNSUPPORTED) {
+			DBG("Err: protocol %d - %s",
+					err, qmi_result_get_error(result));
+			return;
+		}
 
-	if (err != QMI_ERR_OP_DEVICE_UNSUPPORTED) {
-		/* modem supports only 1 protocol */
-		qmi_result_get_uint8(result, QMI_WMS_PARAM_PROTOCOL,
-					&data->msg_mode);
-	} else {
-		/* check both, start with 1 then switch to other */
-		DBG("device supports CDMA and WCDMA msg protocol");
+		/* Get Message Protocol operation is not supported */
+		DBG("query both CDMA and WCDMA");
 		data->msg_mode_all = true;
 		data->msg_mode = QMI_WMS_MESSAGE_MODE_CDMA;
+	} else {
+		/* Query of current protocol succeeded, use that */
+		qmi_result_get_uint8(result, QMI_WMS_PARAM_PROTOCOL,
+					&data->msg_mode);
+
+		DBG("msg_mode: %s", data->msg_mode ? "WCDMA" : "CDMA");
 	}
 
 	/* check for messages */
-- 
2.43.0

Re: [PATCH 1/2] qmimodem: Rework GET_CARD_STATUS retry logic

[patchwork-bot+ofono]

Hello:

This series was applied to ofono.git (master)
by Denis Kenzior <denkenz@gmail.com>:

On Tue, 19 Mar 2024 11:42:53 -0500 you wrote:
> Port the retry logic from glib based implementation that uses
> g_timeout_add to use l_timeout instead.  While here, fix the existing
> logic to not leak memory when a retry is performed.  This happens in one
> of two ways:
> 
> - When retry_cbd is allocated via cb_data_new, it is never freed when
>   the timeout GSource fires.
> - If the timeout GSource is removed early (i.e. due to remove() being
>   called), the associated retry_cbd is not freed
> 
> [...]

Here is the summary with links:
  - [1/2] qmimodem: Rework GET_CARD_STATUS retry logic
    https://git.kernel.org/pub/scm/network/ofono/ofono.git/?id=559c224c4cd3
  - [2/2] qmimodem: sms: Silence valgrind warning
    https://git.kernel.org/pub/scm/network/ofono/ofono.git/?id=b0f808ccd6e1

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html