<div dir="ltr">Thanks Dan for the very helpful feedback. I will get update the patch with the changes. It may take me some time to squeeze it in as our dev sprints allow but I'll get it back up here.</div><div class="gmail_extra"> <div class="gmail_quote">On Mon, Aug 29, 2016 at 1:19 PM, Dan Williams <<a href="mailto:dcbw@redhat.com" target="_blank">dcbw@redhat.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Mon, 2016-08-29 at 09:44 -0600, matthew stanger wrote: > Attached is the patch, gmail is the culprit. Thanks! A couple general comments. If it's possible, could the GPS stuff be split out into a separate patch? Makes for easier review of both that and the SWWAN stuff. For feature detection, maybe instead of assuming everything with a net port supports SWWAN, just send ^SWWAN? and if you get an error then obviously it's unsupported, if you get a "yes" response *and* we know of a net port already, then assume supported. I'd do this at modem initialization time and do the usb0/usb1 :: PDP# at the same time, rather than on bearer creation. Next, I would actually separate the connect and disconnect state machine functions. I feel like piling multiple cases into get_swwan_response() and cinterion_3gpp_state_machine_logic() confuses the functions. I feel like it would be easier to follow to move the actual state machine logic out of the command-parsing functions, and into smaller functions that just call the command parsing functions and handle their result. I know that's probably what you're trying to do in get_swwan_response() but I think it could be more generalized to make the flow clearer. This also lets you pass the specific connect or disconnect context to the _ready() functions that process the responses, which will make some of the "connect_pending_context" stuff simpler too. In other modems we only use those when we get unsolicited responses since there's no caller function to pass a context from. For connect cancellation (like if the user manually disconnects while you're already connecting), I believe you'll get a report_connection_status() call when the connect is canceled, so you can handle that instead of checking manually for in connect_3gpp_context_step(). Although it does seem like the Huawei plugin does the same thing your patch does, so maybe I'm off-base here. Instead of doing a bunch of stuff in STEP_INIT, I'd just do that in connect_3gpp() when creating the Control3gppContext. I'd probably also defer creating the ipv4_config until we actually read the bearer details. Then at the end of INIT in connect_3gpp_step() instead of calling cinterion_3gpp_state_machine_logic() you can just drop down to the next case statement, eg no 'break'. But put an explicit comment there to show that's intended. Then for VERIFY_SWWAN, I can't see where it waits with a timeout before polling ^SWWAN again? I think the Huawei bearer is the model to follow here, since that sends the ^NDISDUP command that is like ^SWWAN to start the initial connection, then periodically polls ^NDISSTATQRY to see if the attempt succeeded. Note that the response handler connect_ndisstatqry_check_ready() parses the response with a generic helper function, and if the modem still isn't connected it starts a 1s timeout before trying again. In your case, that timeout would trigger another call to ^SWWAN to check the status. Basically, the flow should go from step-to-step and typically not back up steps. I think your model can follow this too. Basically: INIT: do the ip type stuff, drop down to next step AUTH: check if auth is required, if so send SGAUTH, otherwise drop down CONNECT: send ^SWWAN=1,X,Y and when that's complete advance to next step WAIT: poll the connection status with SWWAN? and if not yet connected, g_timeout_add_seconds() to poll again. When connected, advance step. IP_CONFIG: pull bearer properties and set ip4_config FINISH: complete the context Disconnect would follow a similar process, but of course you don't need AUTH or IP_CONFIG steps. Lastly, is the PDP CID -> usb interface mapping hardcoded in the firmware? I can't find anything about that in PLS8-E_ATC_V02.011 or PLS8-E_ATC_V01.000. ie, can't we activate any arbitrary PDP context for an arbitary WWAN adapter with ^SWWAN=1,X,Y where X is a PDP context already determined by the MM core? The +CGDCONT logic in the Cinterion plugin is overriding that and and may conflict with MM core logic. Dan </blockquote></div> </div>