From 5cc583bf3ad0e29e093da27e422eee9db9e68051 Mon Sep 17 00:00:00 2001 From: Xavier Chapron Date: Wed, 9 Mar 2022 17:36:56 +0100 Subject: [PATCH] drivers: modem: modem_cmd_handler.c: Drop cmd from buf if args are missing In 90c6dc5e7f45ba696de42ab2e02c0f31e07843e0, a change was introduced to allow modem commands determine if they have enough data or not. In a situation where some data is missing, the command should return -EAGAIN and this should lead to another call of the command with more data. In this commit, the argument parser was also allowed to return -EAGAIN to request more data due to missing arguments. However, this can't work because in cmd_handler_process_rx_buf() before calling process_cmd(): - we make sure that a CR/LF has been found. - we compute match_len which can't be greater than the distance to the next CR/LF. Therefore, even if the command argument parser ask for more data by returning -EAGAIN, next call will have the same value for match_len, meaning that the parsing of argument will result in the same missing argument situation. This leads to an infinite loop of parsing the same data over and over in an infinite loop. This commit change this behavior to always drop the data in such a situation. The command will not be answered and will therefore timeout, but at least, next commands will correctly parse their returned data. Signed-off-by: Xavier Chapron --- drivers/modem/modem_cmd_handler.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/drivers/modem/modem_cmd_handler.c b/drivers/modem/modem_cmd_handler.c index 5b5bd31be96..04f6b76c245 100644 --- a/drivers/modem/modem_cmd_handler.c +++ b/drivers/modem/modem_cmd_handler.c @@ -159,7 +159,16 @@ static int parse_params(struct modem_cmd_handler_data *data, size_t match_len, /* missing arguments */ if (*argc < cmd->arg_count_min) { - return -EAGAIN; + /* Do not return -EAGAIN here as there is no way new argument + * can be parsed later because match_len is computed to be + * the minimum of the distance to the first CRLF and the size + * of the buffer. + * Therefore, waiting more data on the interface won't change + * match_len value, which mean there is no point in waiting + * for more arguments, this will just end in a infinite loop + * parsing data and finding that some arguments are missing. + */ + return -EINVAL; } /* @@ -370,9 +379,13 @@ static void cmd_handler_process_rx_buf(struct modem_cmd_handler_data *data) LOG_DBG("match cmd [%s] (len:%zu)", log_strdup(cmd->cmd), match_len); - if (process_cmd(cmd, match_len, data) == -EAGAIN) { + ret = process_cmd(cmd, match_len, data); + if (ret == -EAGAIN) { k_sem_give(&data->sem_parse_lock); break; + } else if (ret < 0) { + LOG_ERR("process cmd [%s] (len:%zu, ret:%d)", + log_strdup(cmd->cmd), match_len, ret); } /*