From e7df25c848c6ab54bc81430605454f91ca204623 Mon Sep 17 00:00:00 2001 From: Benjamin Lindqvist Date: Fri, 9 Oct 2020 13:08:28 +0200 Subject: [PATCH] drivers: modem: gsm_ppp: lock modem when required If other threads are accessing the modem concurrently during gsm_ppp initialization, they can easily corrupt UART comms. Normally this is not noticable since those services are usually started only after modem setup has been completed. But with the new gsm_ppp stop/start functionality, the need of synchronizing concurrent access in a structured way has arisen. This commit ensures modem_cmd_handler is locked when concurrent access to the modem UART would interfere with the driver, or otherwise cause problems. Since the semaphore is not available during this period, all essential calls to modem_cmd_send has been replaced with the non-locking equivalents. Signed-off-by: Benjamin Lindqvist --- drivers/modem/gsm_ppp.c | 99 ++++++++++++++++++++++------------------- 1 file changed, 53 insertions(+), 46 deletions(-) diff --git a/drivers/modem/gsm_ppp.c b/drivers/modem/gsm_ppp.c index f87c9e492b..a289d1061e 100644 --- a/drivers/modem/gsm_ppp.c +++ b/drivers/modem/gsm_ppp.c @@ -289,21 +289,21 @@ static int gsm_setup_mccmno(struct gsm_modem *gsm) if (CONFIG_MODEM_GSM_MANUAL_MCCMNO[0]) { /* use manual MCC/MNO entry */ - ret = modem_cmd_send(&gsm->context.iface, - &gsm->context.cmd_handler, - NULL, 0, - "AT+COPS=1,2,\"" - CONFIG_MODEM_GSM_MANUAL_MCCMNO - "\"", - &gsm->sem_response, - GSM_CMD_AT_TIMEOUT); + ret = modem_cmd_send_nolock(&gsm->context.iface, + &gsm->context.cmd_handler, + NULL, 0, + "AT+COPS=1,2,\"" + CONFIG_MODEM_GSM_MANUAL_MCCMNO + "\"", + &gsm->sem_response, + GSM_CMD_AT_TIMEOUT); } else { /* register operator automatically */ - ret = modem_cmd_send(&gsm->context.iface, - &gsm->context.cmd_handler, - NULL, 0, "AT+COPS=0,0", - &gsm->sem_response, - GSM_CMD_AT_TIMEOUT); + ret = modem_cmd_send_nolock(&gsm->context.iface, + &gsm->context.cmd_handler, + NULL, 0, "AT+COPS=0,0", + &gsm->sem_response, + GSM_CMD_AT_TIMEOUT); } if (ret < 0) { @@ -353,12 +353,12 @@ static void gsm_finalize_connection(struct gsm_modem *gsm) int ret; if (IS_ENABLED(CONFIG_GSM_MUX) && gsm->mux_enabled) { - ret = modem_cmd_send(&gsm->context.iface, - &gsm->context.cmd_handler, - &response_cmds[0], - ARRAY_SIZE(response_cmds), - "AT", &gsm->sem_response, - GSM_CMD_AT_TIMEOUT); + ret = modem_cmd_send_nolock(&gsm->context.iface, + &gsm->context.cmd_handler, + &response_cmds[0], + ARRAY_SIZE(response_cmds), + "AT", &gsm->sem_response, + GSM_CMD_AT_TIMEOUT); if (ret < 0) { LOG_ERR("modem setup returned %d, %s", ret, "retrying..."); @@ -370,12 +370,12 @@ static void gsm_finalize_connection(struct gsm_modem *gsm) (void)gsm_setup_mccmno(gsm); - ret = modem_cmd_handler_setup_cmds(&gsm->context.iface, - &gsm->context.cmd_handler, - setup_cmds, - ARRAY_SIZE(setup_cmds), - &gsm->sem_response, - GSM_CMD_SETUP_TIMEOUT); + ret = modem_cmd_handler_setup_cmds_nolock(&gsm->context.iface, + &gsm->context.cmd_handler, + setup_cmds, + ARRAY_SIZE(setup_cmds), + &gsm->sem_response, + GSM_CMD_SETUP_TIMEOUT); if (ret < 0) { LOG_DBG("modem setup returned %d, %s", ret, "retrying..."); @@ -401,12 +401,12 @@ static void gsm_finalize_connection(struct gsm_modem *gsm) LOG_DBG("modem setup returned %d, %s", ret, "enable PPP"); - ret = modem_cmd_handler_setup_cmds(&gsm->context.iface, - &gsm->context.cmd_handler, - connect_cmds, - ARRAY_SIZE(connect_cmds), - &gsm->sem_response, - GSM_CMD_SETUP_TIMEOUT); + ret = modem_cmd_handler_setup_cmds_nolock(&gsm->context.iface, + &gsm->context.cmd_handler, + connect_cmds, + ARRAY_SIZE(connect_cmds), + &gsm->sem_response, + GSM_CMD_SETUP_TIMEOUT); if (ret < 0) { LOG_DBG("modem setup returned %d, %s", ret, "retrying..."); @@ -427,20 +427,22 @@ static void gsm_finalize_connection(struct gsm_modem *gsm) LOG_DBG("iface %suart error %d", "AT ", ret); } else { /* Do a test and try to send AT command to modem */ - ret = modem_cmd_send(&gsm->context.iface, - &gsm->context.cmd_handler, - &response_cmds[0], - ARRAY_SIZE(response_cmds), - "AT", &gsm->sem_response, - GSM_CMD_AT_TIMEOUT); + ret = modem_cmd_send_nolock( + &gsm->context.iface, + &gsm->context.cmd_handler, + &response_cmds[0], + ARRAY_SIZE(response_cmds), + "AT", &gsm->sem_response, + GSM_CMD_AT_TIMEOUT); if (ret < 0) { - LOG_DBG("modem setup returned %d, %s", + LOG_WRN("modem setup returned %d, %s", ret, "AT cmds failed"); } else { LOG_INF("AT channel %d connected to %s", DLCI_AT, gsm->at_dev->name); } } + modem_cmd_handler_tx_unlock(&gsm->context.cmd_handler); } } @@ -450,7 +452,7 @@ static int mux_enable(struct gsm_modem *gsm) /* Turn on muxing */ if (IS_ENABLED(CONFIG_MODEM_GSM_SIMCOM)) { - ret = modem_cmd_send( + ret = modem_cmd_send_nolock( &gsm->context.iface, &gsm->context.cmd_handler, &response_cmds[0], @@ -473,7 +475,7 @@ static int mux_enable(struct gsm_modem *gsm) GSM_CMD_AT_TIMEOUT); } else { /* Generic GSM modem */ - ret = modem_cmd_send(&gsm->context.iface, + ret = modem_cmd_send_nolock(&gsm->context.iface, &gsm->context.cmd_handler, &response_cmds[0], ARRAY_SIZE(response_cmds), @@ -632,12 +634,12 @@ static void gsm_configure(struct k_work *work) LOG_DBG("Starting modem %p configuration", gsm); - ret = modem_cmd_send(&gsm->context.iface, - &gsm->context.cmd_handler, - &response_cmds[0], - ARRAY_SIZE(response_cmds), - "AT", &gsm->sem_response, - GSM_CMD_AT_TIMEOUT); + ret = modem_cmd_send_nolock(&gsm->context.iface, + &gsm->context.cmd_handler, + &response_cmds[0], + ARRAY_SIZE(response_cmds), + "AT", &gsm->sem_response, + GSM_CMD_AT_TIMEOUT); if (ret < 0) { LOG_DBG("modem not ready %d", ret); @@ -710,6 +712,11 @@ void gsm_ppp_stop(const struct device *device) uart_mux_disable(gsm->ppp_dev); } } + + if (modem_cmd_handler_tx_lock(&gsm->context.cmd_handler, + K_SECONDS(10))) { + LOG_WRN("Failed locking modem cmds!"); + } } static int gsm_init(const struct device *device)