CIDs not released when MM stops/restarts [openwrt]

[Aleksander Morgado]

Hey,

>
> I’m attempting to fix below reported problem with below patch. But mm crashes as soon as g_list_free_full (self->priv->qmi_clients, g_free) is called in mm_port_mbim_close(). If I comment g_list_free_full, then mm shuts down cleanly. Any suggestion what is going wrong?
>

Yes, see comments below.

>
> diff --git a/src/mm-port-mbim.c b/src/mm-port-mbim.c
>
> index dbc70fef..4f620a55 100644
>
> --- a/src/mm-port-mbim.c
>
> +++ b/src/mm-port-mbim.c
>
> @@ -424,6 +424,20 @@ mm_port_mbim_close (MMPortMbim *self,
>
> #if defined WITH_QMI && QMI_MBIM_QMUX_SUPPORTED
>
>      if (self->priv->qmi_device) {
>
>          GError *error = NULL;
>
> +        GList *l;

Please add a whiteline after declaring variables.

>
> +        /* Release all allocated clients */
>
> +        for (l = self->priv->qmi_clients; l; l = g_list_next (l)) {
>
> +            QmiClient *qmi_client = QMI_CLIENT (l->data);
>
> +
>
> +            mm_dbg ("Releasing client for qmi_clients '%s'...", qmi_service_get_string (qmi_client_get_service (qmi_client)));
>
> +            qmi_device_release_client (self->priv->qmi_device,
>
> +                                       qmi_client,
>
> +                                       QMI_DEVICE_RELEASE_CLIENT_FLAGS_RELEASE_CID,
>
> +                                       3, NULL, NULL, NULL);
>
> +            g_clear_object (&qmi_client);
>
> +        }
>
> +        g_list_free_full (self->priv->qmi_clients, g_free);
>

You're attempting to release the QmiClient twice. First with the
g_clear_object() and then (wrong) with g_list_free_full (..., g_free).
You have two options:
 a) Leave the g_clear_object() and change
g_list_free_full(qmi_clients, g_free) with just g_list_free
(qmi_clients).
 b) Remove the g_clear_object() inside the loop and do a single
g_list_free_full (qmi_clients, g_object_unref) instead.

I would prefer doing b)


> +        self->priv->qmi_clients = NULL;
>
>
>
>          if (!qmi_device_close (self->priv->qmi_device, &error)) {
>
>              mm_warn ("Couldn't properly close QMI device: %s", error->message);

And g_error_free (error);

>
> @@ -473,9 +487,17 @@ static void
>
> dispose (GObject *object)
>
> {
>
>      MMPortMbim *self = MM_PORT_MBIM (object);
>
> +    GList *l;
>
>
>
> #if defined WITH_QMI && QMI_MBIM_QMUX_SUPPORTED
>
> -    g_list_free_full (self->priv->qmi_clients, g_object_unref);
>
> +    /* Deallocate all clients */
>
> +    for (l = self->priv->qmi_clients; l; l = g_list_next (l)) {
>
> +        QmiClient *qmi_client = QMI_CLIENT (l->data);
>
> +
>
> +        if (qmi_client)
>
> +            g_object_unref (qmi_client);
>
> +    }
>
> +    g_list_free_full (self->priv->qmi_clients, g_free);
>

No, this change in dispose is not right. You should better leave
g_list_free_full (self->priv->qmi_clients, g_object_unref);


>      self->priv->qmi_clients = NULL;
>
>      g_clear_object (&self->priv->qmi_device);
>
> #endif
>
>
>
> From: Amol Lad
> Sent: Thursday, 21 November 2019 2:52 PM
> To: modemmanager-devel at lists.freedesktop.org
> Subject: CIDs not released when MM stops/restarts [openwrt]
>
>
>
> Hi,
>
>
>
> I’m using MM 1.12.0 in openwrt system [EM7565 in MBIM mode].
>
>
>
> When MM starts , we see below Client ID allocations.
>
>
>
> Nov 21 08:50:00 OpenWrt [9084]: [/dev/cdc-wdm0] Allocating new client ID...
>
> Nov 21 08:50:00 OpenWrt [9084]: [/dev/cdc-wdm0] Registered 'dms' (version 1.0) client with ID '3'
>
> Nov 21 08:50:00 OpenWrt [9084]: [/dev/cdc-wdm0] Allocating new client ID...
>
> Nov 21 08:50:00 OpenWrt [9084]: [/dev/cdc-wdm0] Registered 'nas' (version 1.25) client with ID '4'
>
> Nov 21 08:50:00 OpenWrt [9084]: [/dev/cdc-wdm0] Allocating new client ID...
>
> Nov 21 08:50:00 OpenWrt [9084]: [/dev/cdc-wdm0] Registered 'loc' (version 2.0) client with ID '3'
>
> Nov 21 08:50:00 OpenWrt [9084]: [/dev/cdc-wdm0] Allocating new client ID...
>
> Nov 21 08:50:00 OpenWrt [9084]: [/dev/cdc-wdm0] Registered 'pdc' (version 1.0) client with ID '3'
>
>
>
>
>
> However, when I restart MM using “service modemmanager restart” command then the log becomes:
>
>
>
>
>
> Nov 21 08:50:53 OpenWrt [10172]: [/dev/cdc-wdm0] Allocating new client ID...
>
> Nov 21 08:50:53 OpenWrt [10172]: [/dev/cdc-wdm0] Registered 'dms' (version 1.0) client with ID '4'
>
> Nov 21 08:50:53 OpenWrt [10172]: [/dev/cdc-wdm0] Allocating new client ID...
>
> Nov 21 08:50:53 OpenWrt [10172]: [/dev/cdc-wdm0] Registered 'nas' (version 1.25) client with ID '5'
>
> Nov 21 08:50:53 OpenWrt [10172]: [/dev/cdc-wdm0] Allocating new client ID...
>
> Nov 21 08:50:53 OpenWrt [10172]: [/dev/cdc-wdm0] Registered 'loc' (version 2.0) client with ID '4'
>
> Nov 21 08:50:53 OpenWrt [10172]: [/dev/cdc-wdm0] Allocating new client ID...
>
> Nov 21 08:50:53 OpenWrt [10172]: [/dev/cdc-wdm0] Registered 'pdc' (version 1.0) client with ID '4'
>
>
>
> Restarting MM again increments the CIDs. It seems when MM stops then CIDs are not released eventually resulting in “ClientIdsExhausted” error after multiple MM restarts.
>
>
>
> (INFO: If I issue “qmicli -d /dev/cdc-wdm0 -p –nas-reset” then “nas” CID gets reset)
>
>
>
> Please let me know if any more information is needed and advise how we can address this.
>
>
>
> Thanks
>
> Amol
>
>
>
>
>
>
>
> ________________________________
> The information in this email communication (inclusive of attachments) is confidential to 4RF Limited and the intended recipient(s). If you are not the intended recipient(s), please note that any use, disclosure, distribution or copying of this information or any part thereof is strictly prohibited and that the author accepts no liability for the consequences of any action taken on the basis of the information provided. If you have received this email in error, please notify the sender immediately by return email and then delete all instances of this email from your system. 4RF Limited will not accept responsibility for any consequences associated with the use of this email (including, but not limited to, damages sustained as a result of any viruses and/or any action or lack of action taken in reliance on it).



-- 
Aleksander
https://aleksander.es