[PATCH] huawei: add basic support for ME909u-521
Aleksander Morgado
aleksander at aleksander.es
Fri Sep 16 07:59:14 UTC 2016
Hey,
Code-wise review of the patch only here; see comments below.
On Thu, Sep 15, 2016 at 6:57 PM, Stefan Eichenberger <eichest at gmail.com> wrote:
>
> The ME909u-512 does theoretically support the DHCP feature over AT but if
> this is done right after the NDISDUP command, it seems that the firmware
> has some trouble with it because the firmware will not attach the
> Ethernet header afterwards. This can be seen by doing a tcpdump while
> pinging e.g. 8.8.8.8. The request has a size of 98 Bytes, while the
> replies only have a size of 84 Bytes. The first 14 Bytes are missing
> which is exactly the size of the Ethernet header.
>
> To reproduce this issue, I've tried the command sequence manually:
> mmcli -m 0 --command 'AT^NDISDUP=1,1,"gprs.swisscom.ch"'
> mmcli -m 0 --command "AT+CREG?"
> mmcli -m 0 --command "AT^NDISSTATQRY?"
> mmcli -m 0 --command "AT+CGREG?"
> mmcli -m 0 --command "AT^DHCP?"
>
> The error appears in with this sequence
>
> mmcli -m 0 --command 'AT^NDISDUP=1,1,"gprs.swisscom.ch"'
> mmcli -m 0 --command "AT+CREG?"
> mmcli -m 0 --command "AT^NDISSTATQRY?"
> mmcli -m 0 --command "AT+CGREG?"
>
> The error does not appear in this sequence
>
> This commit applies to 1.6.0 and adds a udev option for this modem,
> which disables the DHCP over AT command feature.
>
So calling AT^DHCP actually modifies the behavior of the network
interface? Have we seen something like this in any other Huawei modem?
> Signed-off-by: Stefan Eichenberger <stefan.eichenberger at netmodule.com>
> ---
> plugins/huawei/77-mm-huawei-net-port-types.rules | 3 ++
> plugins/huawei/mm-broadband-bearer-huawei.c | 50 ++++++++++++++++++------
> 2 files changed, 42 insertions(+), 11 deletions(-)
>
> diff --git a/plugins/huawei/77-mm-huawei-net-port-types.rules b/plugins/huawei/77-mm-huawei-net-port-types.rules
> index f60f1f8..d35f2d5 100644
> --- a/plugins/huawei/77-mm-huawei-net-port-types.rules
> +++ b/plugins/huawei/77-mm-huawei-net-port-types.rules
> @@ -6,6 +6,9 @@ ENV{ID_VENDOR_ID}!="12d1", GOTO="mm_huawei_port_types_end"
> # MU609 does not support getportmode (crashes modem with default firmware)
> ATTRS{idProduct}=="1573", ENV{ID_MM_HUAWEI_DISABLE_GETPORTMODE}="1"
>
> +# MU909u does not support DHCP properly, it can happen that the Ethernet frames do not attach the Ethernet header afterwards.
> +ATTRS{idProduct}=="1573", ENV{ID_MM_HUAWEI_DISABLE_DHCP}="1"
> +
> # Mark the modem and at port flags for ModemManager
> SUBSYSTEMS=="usb", ATTRS{bInterfaceClass}=="ff", ATTRS{bInterfaceSubClass}=="01", ATTRS{bInterfaceProtocol}=="01", ENV{ID_MM_HUAWEI_MODEM_PORT}="1"
> SUBSYSTEMS=="usb", ATTRS{bInterfaceClass}=="ff", ATTRS{bInterfaceSubClass}=="01", ATTRS{bInterfaceProtocol}=="02", ENV{ID_MM_HUAWEI_AT_PORT}="1"
> diff --git a/plugins/huawei/mm-broadband-bearer-huawei.c b/plugins/huawei/mm-broadband-bearer-huawei.c
> index 60a91e5..11782c3 100644
> --- a/plugins/huawei/mm-broadband-bearer-huawei.c
> +++ b/plugins/huawei/mm-broadband-bearer-huawei.c
> @@ -465,17 +465,34 @@ connect_3gpp_context_step (Connect3gppContext *ctx)
> g_object_ref (ctx->self));
> return;
>
> - case CONNECT_3GPP_CONTEXT_STEP_IP_CONFIG:
> - mm_base_modem_at_command_full (ctx->modem,
> - ctx->primary,
> - "^DHCP?",
> - 3,
> - FALSE,
> - FALSE,
> - NULL,
> - (GAsyncReadyCallback)connect_dhcp_check_ready,
> - g_object_ref (ctx->self));
> - return;
> + case CONNECT_3GPP_CONTEXT_STEP_IP_CONFIG: {
> + GUdevClient *client;
> + GUdevDevice *data_device;
> +
Don't call it "data_device"; as it's not; it's a tty_device.
> + // ME909u has a problem with DHCP over AT. If it's done right after NDSIDUP
> + // the modem doesn't send the Ethernet header anymore which confuses the network stack
No comments with //, use only /* this */ even for single line comments :)
> + client = g_udev_client_new (NULL);
> + data_device = (g_udev_client_query_by_subsystem_and_name (
> + client,
> + "tty",
> + mm_port_get_device (&ctx->primary->parent.parent)));
No "&primary->parent.parent". I'm assuming you need a MMPort from a
MMPortAt; so you should do:
mm_port_get_device (MM_PORT (ctx->primary))
> + if (!data_device || !g_udev_device_get_property_as_boolean (data_device, "ID_MM_HUAWEI_DISABLE_DHCP")) {
Shouldn't we actually error out if !data_device?
> + mm_base_modem_at_command_full (ctx->modem,
> + ctx->primary,
> + "^DHCP?",
> + 3,
> + FALSE,
> + FALSE,
> + NULL,
> + (GAsyncReadyCallback)connect_dhcp_check_ready,
> + g_object_ref (ctx->self));
Both client and data_device are leaking here.
> + return;
> + }
> +
Both client and data_device are leaking here.
> + mm_info("This device (%s) does not support DHCP over AT", mm_port_get_device (ctx->data));
You could set here the ctx->ipv4_config value with
MM_BEARER_IP_METHOD_DHCP, so that you don't need to change the next
step. I think this would also be clearer; ipv4_config will be set
always in the CONNECT_3GPP_CONTEXT_STEP_IP_CONFIG step, regardless of
whether the modem supports ^DHCP or not.
> + ctx->step ++;
> + /* Fall down to the next step */
> + }
>
> case CONNECT_3GPP_CONTEXT_STEP_LAST:
> /* Clear context */
> @@ -489,6 +506,17 @@ connect_3gpp_context_step (Connect3gppContext *ctx)
> mm_bearer_connect_result_new (ctx->data, ctx->ipv4_config, NULL),
> (GDestroyNotify)mm_bearer_connect_result_unref);
> }
> + else {
> + MMBearerIpConfig *ipv4_config;
> +
> + ipv4_config = mm_bearer_ip_config_new ();
> + mm_bearer_ip_config_set_method (ipv4_config, MM_BEARER_IP_METHOD_DHCP);
> + g_simple_async_result_set_op_res_gpointer (
> + ctx->result,
> + mm_bearer_connect_result_new (ctx->data, ipv4_config, NULL),
> + (GDestroyNotify)mm_bearer_connect_result_unref);
> + g_object_unref (ipv4_config);
> + }
> }
>
> connect_3gpp_context_complete_and_free (ctx);
> --
> 2.0.4
>
--
Aleksander
https://aleksander.es
More information about the ModemManager-devel
mailing list