Re: [PATCH] huawei: add basic support for ME909u-521

2016-09-16 Thread Stefan Eichenberger
Hi,

On Fri, Sep 16, 2016 at 09:59:14AM +0200, Aleksander Morgado wrote:
> Hey,
> 
> Code-wise review of the patch only here; see comments below.
> 
> On Thu, Sep 15, 2016 at 6:57 PM, Stefan Eichenberger  
> 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?

I have to say the ME909u has fw version 12.636.11.01.00. We also use a ME609 
with fw version 12.107.08.01.00 where it works correctly and with the ME909s 
which has as far as I know a HiSilicon chipset it works too.

> 
> > Signed-off-by: Stefan Eichenberger 
> > ---
> >  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 ne

Re: [PATCH] huawei: add basic support for ME909u-521

2016-09-16 Thread Stefan Eichenberger
Hi Dan,

On Thu, Sep 15, 2016 at 01:00:35PM -0500, Dan Williams wrote:
> On Thu, 2016-09-15 at 18:57 +0200, Stefan Eichenberger 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?"
> 
> Have you tested out how long the firmware needs before it can take the
> AT^DHCP without problems?
I was doing the test this morning again. It seems that it's not really
time related. I can wait as long as I want between AT+CGREG? and
AT^DHCP? if I do it the Ethernet header will be missing. But when I do
a dhcp request over the Ethernet interface between the two steps, then I
can do the AT^DHCP? without problems.

So this was working for me:
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?"
udhcpc -i usb0
mmcli -m 0 --command "AT^DHCP?"

> 
> Dan
> 
> > 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.
> > 
> > Signed-off-by: Stefan Eichenberger  > >
> > ---
> >  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;
> > +
> > +// 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
> > +client = g_udev_client_new (NULL);
> > +data_device = (g_udev_client_query_by_subsystem_and_name (
> > +   client,
> > +   "tty",
> > +   mm_port_get_device (

Re: [PATCH] huawei: add basic support for ME909u-521

2016-09-16 Thread Aleksander Morgado
Hey,

Code-wise review of the patch only here; see comments below.

On Thu, Sep 15, 2016 at 6:57 PM, Stefan Eichenberger  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 
> ---
>  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,
> +   

Re: [PATCH] huawei: add basic support for ME909u-521

2016-09-15 Thread Dan Williams
On Thu, 2016-09-15 at 18:57 +0200, Stefan Eichenberger 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?"

Have you tested out how long the firmware needs before it can take the
AT^DHCP without problems?

Dan

> 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.
> 
> Signed-off-by: Stefan Eichenberger  >
> ---
>  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;
> +
> +// 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
> +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)));
> +if (!data_device || !g_udev_device_get_property_as_boolean
> (data_device, "ID_MM_HUAWEI_DISABLE_DHCP")) {
> +mm_base_modem_at_command_full (ctx->modem,
> +   ctx->primary,
> +   "^DHCP?",
> +   3,
> +   FALSE,
> +   FALSE,
> +   NULL,
> +   (GAsyncReadyCallback)conn
> ect_dhcp_check_ready,
> +   g_object_ref (ctx-
> >self));
> +return;
> +}
> +
> +mm_info("This device (%s) does not support DHCP over AT",
> mm_port_g