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 <eich...@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?

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 <stefan.eichenber...@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
> >
> 

Thanks a lot for the review so far, I will fix the findings as soon as it seems
to be okay to have this dirty workaround.

Regards,
Stefan

_______________________________________________
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Reply via email to