| Message ID | 20240413215318.12236-4-adam@piggz.co.uk (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v4,1/4] qmimodem: voicecall: Implement call dialing | expand |
Hi Adam, On 4/13/24 16:53, Adam Pigg wrote: > The send_dtmf function sets up a call to send_one_dtmf, which will call > the QMI_VOICE_START_CONTINUOUS_DTMF service function. The parameters to > this call are a hard coded call-id and the DTMF character to send. > start_cont_dtmf_cb will then be called which will set up a call to > QMI_VOICE_STOP_CONTINUOUS_DTMF to stop the tone. Finally, > stop_cont_dtmf_cb will check the final status. > > --- > Changes in V4 > -Removed unused enum > -Minor formatting fixes > -Ensure data->full_dtmf is free'd > -Use cb_data_ref/unref between chains of dtmf calls > --- > --- > drivers/qmimodem/voice.h | 6 ++ > drivers/qmimodem/voicecall.c | 131 +++++++++++++++++++++++++++++++++++ > 2 files changed, 137 insertions(+) > <snip> > @@ -614,6 +616,133 @@ static void hangup_active(struct ofono_voicecall *vc, ofono_voicecall_cb_t cb, > release_specific(vc, call->id, cb, data); > } > > +static void stop_cont_dtmf_cb(struct qmi_result *result, void *user_data) > +{ > + struct cb_data *cbd = user_data; > + ofono_voicecall_cb_t cb = cbd->cb; > + > + uint16_t error; > + > + DBG(""); > + > + if (qmi_result_set_error(result, &error)) { > + DBG("QMI Error %d", error); > + CALLBACK_WITH_FAILURE(cb, cbd->data); > + return; > + } > + > + CALLBACK_WITH_SUCCESS(cb, cbd->data); > + l_free(cbd); This looks wrong. When invoking QMI_VOICE_STOP_CONTINUOUS_DTMF, you use cb_data_unref as the destroy function. The destroy invocation should take care of cbd. > +} > + > +static void start_cont_dtmf_cb(struct qmi_result *result, void *user_data) > +{ > + struct cb_data *cbd = user_data; > + ofono_voicecall_cb_t cb = cbd->cb; > + struct ofono_voicecall *vc = cbd->user; > + struct voicecall_data *vd = ofono_voicecall_get_data(vc); > + uint16_t error; > + struct qmi_param *param; > + > + DBG(""); > + > + if (qmi_result_set_error(result, &error)) { > + DBG("QMI Error %d", error); > + CALLBACK_WITH_FAILURE(cb, cbd->data); > + return; This is called with cbd, and hopefully in all cases cbd->ref == 1 at this point with the destroy callback being cb_data_unref. Once this function returns, destroy is called and cbd is freed correctly. This looks good. > + } > + > + param = qmi_param_new(); > + > + if (!qmi_param_append_uint8(param, QMI_VOICE_DTMF_DATA, 0xff)) > + goto error; > + > + if (qmi_service_send(vd->voice, QMI_VOICE_STOP_CONTINUOUS_DTMF, param, > + stop_cont_dtmf_cb, cbd, cb_data_unref) > 0) { > + cb_data_ref(cbd); > + return; On the success path, we take another ref to cbd (so cbd->ref == 2). Once we return, destroy is called and cbd->ref is back down to 1. > + } > + > +error: > + CALLBACK_WITH_FAILURE(cb, cbd->data); > + l_free(cbd); On the error path, this isn't necessary at all. cbd->ref should be 1, and once this function returns, cb_data_unref should free cbd. > + l_free(param); > +} > + > +static void send_one_dtmf(struct ofono_voicecall *vc, const char dtmf, > + ofono_voicecall_cb_t cb, void *data) > +{ > + struct voicecall_data *vd = ofono_voicecall_get_data(vc); > + struct qmi_param *param = NULL; > + uint8_t param_body[2]; > + struct cb_data *cbd = cb_data_new(cb, data); > + > + DBG(""); > + > + cbd->user = vc; > + cb_data_ref(cbd); No need to take a ref here, cb_data_new creates cbd with ref==1. > + > + param = qmi_param_new(); > + > + param_body[0] = 0xff; > + param_body[1] = (uint8_t)dtmf; > + > + if (!qmi_param_append(param, QMI_VOICE_DTMF_DATA, sizeof(param_body), > + param_body)) > + goto error; > + > + if (qmi_service_send(vd->voice, QMI_VOICE_START_CONTINUOUS_DTMF, param, > + start_cont_dtmf_cb, cbd, cb_data_unref) > 0) > + return; > + > +error: > + CALLBACK_WITH_FAILURE(cb, data); > + l_free(cbd); > + l_free(param); > +} > + So you have two types of cb_data objects: 1. The one you create in send_dtmf() that contains the callback to ofono core 2. The one you create here that contains the callback to send_one_dtmf_cb and pointer to 1. The 2nd cbd goes through the following callbacks: send_one_dtmf -> ref: 1 start_cont_dtmf_cb -> take ref to 2 on success, in all cases unref is called when this function returns stop_cont_dtmf -> ref: 1, unref is called when this function returns Note that you still might leak the object in 1 if the DTMF is interrupted mid-execution. For example upon voicecall atom removal. > +static void send_one_dtmf_cb(const struct ofono_error *error, void *data) > +{ > + struct cb_data *cbd = data; > + struct voicecall_data *vd = ofono_voicecall_get_data(cbd->user); > + ofono_voicecall_cb_t cb = cbd->cb; > + > + DBG(""); > + > + if (error->type != OFONO_ERROR_TYPE_NO_ERROR || > + *vd->next_dtmf == 0) { > + if (error->type == OFONO_ERROR_TYPE_NO_ERROR) > + CALLBACK_WITH_SUCCESS(cb, cbd->data); > + else > + CALLBACK_WITH_FAILURE(cb, cbd->data); > + > + l_free(vd->full_dtmf); > + vd->full_dtmf = NULL; > + l_free(cbd); > + } else { > + send_one_dtmf(cbd->user, > + *(vd->next_dtmf++), > + send_one_dtmf_cb, data); > + } > +} > + > +static void send_dtmf(struct ofono_voicecall *vc, const char *dtmf, > + ofono_voicecall_cb_t cb, void *data) > +{ > + struct voicecall_data *vd = ofono_voicecall_get_data(vc); > + struct cb_data *cbd = cb_data_new(cb, data); Hmm, you alloc cb_data_new here which creates cbd with 1 ref > + > + DBG(""); > + > + l_free(vd->full_dtmf); > + vd->full_dtmf = l_strdup(dtmf); > + vd->next_dtmf = &vd->full_dtmf[1]; > + > + cbd->user = vc; > + > + send_one_dtmf(vc, *dtmf, send_one_dtmf_cb, cbd); Then here, cbd is refed again. There's likely a leak? You're much safer storing 'cb' and 'data' inside struct voicecall_data (as send_dtmf_cb and send_dtmf_data). > +} > + > static void create_voice_cb(struct qmi_service *service, void *user_data) > { > struct ofono_voicecall *vc = user_data; Regards, -Denis
diff --git a/drivers/qmimodem/voice.h b/drivers/qmimodem/voice.h index caedb079..961fe19f 100644 --- a/drivers/qmimodem/voice.h +++ b/drivers/qmimodem/voice.h @@ -53,6 +53,8 @@ enum voice_commands { QMI_VOICE_GET_ALL_CALL_INFO = 0x2f, QMI_VOICE_END_CALL = 0x21, QMI_VOICE_ANSWER_CALL = 0x22, + QMI_VOICE_START_CONTINUOUS_DTMF = 0x29, + QMI_VOICE_STOP_CONTINUOUS_DTMF = 0x2A, QMI_VOICE_SUPS_NOTIFICATION_IND = 0x32, QMI_VOICE_SET_SUPS_SERVICE = 0x33, QMI_VOICE_GET_CALL_WAITING = 0x34, @@ -85,6 +87,10 @@ enum qmi_voice_call_state { QMI_VOICE_CALL_STATE_SETUP }; +enum qmi_voice_call_dtmf_param { + QMI_VOICE_DTMF_DATA = 0x01, +}; + struct qmi_ussd_data { uint8_t dcs; uint8_t length; diff --git a/drivers/qmimodem/voicecall.c b/drivers/qmimodem/voicecall.c index 396732ad..f3111c9d 100644 --- a/drivers/qmimodem/voicecall.c +++ b/drivers/qmimodem/voicecall.c @@ -42,6 +42,8 @@ struct voicecall_data { uint16_t minor; struct l_queue *call_list; struct ofono_phone_number dialed; + char *full_dtmf; + const char *next_dtmf; }; struct qmi_voice_call_information_instance { @@ -614,6 +616,133 @@ static void hangup_active(struct ofono_voicecall *vc, ofono_voicecall_cb_t cb, release_specific(vc, call->id, cb, data); } +static void stop_cont_dtmf_cb(struct qmi_result *result, void *user_data) +{ + struct cb_data *cbd = user_data; + ofono_voicecall_cb_t cb = cbd->cb; + + uint16_t error; + + DBG(""); + + if (qmi_result_set_error(result, &error)) { + DBG("QMI Error %d", error); + CALLBACK_WITH_FAILURE(cb, cbd->data); + return; + } + + CALLBACK_WITH_SUCCESS(cb, cbd->data); + l_free(cbd); +} + +static void start_cont_dtmf_cb(struct qmi_result *result, void *user_data) +{ + struct cb_data *cbd = user_data; + ofono_voicecall_cb_t cb = cbd->cb; + struct ofono_voicecall *vc = cbd->user; + struct voicecall_data *vd = ofono_voicecall_get_data(vc); + uint16_t error; + struct qmi_param *param; + + DBG(""); + + if (qmi_result_set_error(result, &error)) { + DBG("QMI Error %d", error); + CALLBACK_WITH_FAILURE(cb, cbd->data); + return; + } + + param = qmi_param_new(); + + if (!qmi_param_append_uint8(param, QMI_VOICE_DTMF_DATA, 0xff)) + goto error; + + if (qmi_service_send(vd->voice, QMI_VOICE_STOP_CONTINUOUS_DTMF, param, + stop_cont_dtmf_cb, cbd, cb_data_unref) > 0) { + cb_data_ref(cbd); + return; + } + +error: + CALLBACK_WITH_FAILURE(cb, cbd->data); + l_free(cbd); + l_free(param); +} + +static void send_one_dtmf(struct ofono_voicecall *vc, const char dtmf, + ofono_voicecall_cb_t cb, void *data) +{ + struct voicecall_data *vd = ofono_voicecall_get_data(vc); + struct qmi_param *param = NULL; + uint8_t param_body[2]; + struct cb_data *cbd = cb_data_new(cb, data); + + DBG(""); + + cbd->user = vc; + cb_data_ref(cbd); + + param = qmi_param_new(); + + param_body[0] = 0xff; + param_body[1] = (uint8_t)dtmf; + + if (!qmi_param_append(param, QMI_VOICE_DTMF_DATA, sizeof(param_body), + param_body)) + goto error; + + if (qmi_service_send(vd->voice, QMI_VOICE_START_CONTINUOUS_DTMF, param, + start_cont_dtmf_cb, cbd, cb_data_unref) > 0) + return; + +error: + CALLBACK_WITH_FAILURE(cb, data); + l_free(cbd); + l_free(param); +} + +static void send_one_dtmf_cb(const struct ofono_error *error, void *data) +{ + struct cb_data *cbd = data; + struct voicecall_data *vd = ofono_voicecall_get_data(cbd->user); + ofono_voicecall_cb_t cb = cbd->cb; + + DBG(""); + + if (error->type != OFONO_ERROR_TYPE_NO_ERROR || + *vd->next_dtmf == 0) { + if (error->type == OFONO_ERROR_TYPE_NO_ERROR) + CALLBACK_WITH_SUCCESS(cb, cbd->data); + else + CALLBACK_WITH_FAILURE(cb, cbd->data); + + l_free(vd->full_dtmf); + vd->full_dtmf = NULL; + l_free(cbd); + } else { + send_one_dtmf(cbd->user, + *(vd->next_dtmf++), + send_one_dtmf_cb, data); + } +} + +static void send_dtmf(struct ofono_voicecall *vc, const char *dtmf, + ofono_voicecall_cb_t cb, void *data) +{ + struct voicecall_data *vd = ofono_voicecall_get_data(vc); + struct cb_data *cbd = cb_data_new(cb, data); + + DBG(""); + + l_free(vd->full_dtmf); + vd->full_dtmf = l_strdup(dtmf); + vd->next_dtmf = &vd->full_dtmf[1]; + + cbd->user = vc; + + send_one_dtmf(vc, *dtmf, send_one_dtmf_cb, cbd); +} + static void create_voice_cb(struct qmi_service *service, void *user_data) { struct ofono_voicecall *vc = user_data; @@ -673,6 +802,7 @@ static void qmi_voicecall_remove(struct ofono_voicecall *vc) qmi_service_unref(data->voice); l_queue_destroy(data->call_list, l_free); + l_free(data->full_dtmf); l_free(data); } @@ -683,6 +813,7 @@ static const struct ofono_voicecall_driver driver = { .answer = answer, .hangup_active = hangup_active, .release_specific = release_specific, + .send_tones = send_dtmf, }; OFONO_ATOM_DRIVER_BUILTIN(voicecall, qmimodem, &driver)