Re: [PATCH] ModemManager: Icera CID handling without an APN

[Nathan Williams <njw google com>]

On Thu, May 19, 2011 at 4:54 PM, Dan Williams <dcbw redhat com> wrote:
> On Thu, 2011-05-12 at 19:02 -0400, Nathan Williams wrote:
>> In mm-generic-gsm.c, if an APN is not provided as a property to
>> simple_connect(), priv->cid is never set and remains at -1.
>> The Icera modem plugin has a _get_cid() wrapper call which checks for
>> this and returns 0 if the stored CID is -1. There are two problems
>> with this:
>>   * 0 is not a valid CID; according to 3GPP 27.007, CIDs have a
>> minimum value of 1. Thus, if an APN was not provided, the plugin
>> issues commands like %IPDPCFG and %IPDPACT with CID=0, which the modem
>> rejects.
>>   * connection_enabled() uses the priv->cid value rather than the
>> _get_cid() wrapped value, and bails out if it's -1, so a connection is
>> never acknowledged and the modem remains in "connecting" state.
>
> So we should never be using an invalid CID here, it should always be 1
> or higher.  I think the -1 was used as an error value that we should
> never hit, and there used to be assertions for that in the code.  I
> think the whole dance here was just to be paranoid since we never ever
> want to send -1 to the modem, which happened before in some corner cases
> which have since been fixed.
>
> So we basically have to deal with the corner case of what happens if the
> code somehow screws up and we for whatever reason can't figure out the
> CID of the PDP context that we're trying to disconnect.  I think
> returning "1" here should only happen for the disconnect case, since in
> all other cases, there would be a logic error to have an invalid CID and
> we should fail out, because that's a bug we need to fix.

I'm a little puzzled. The issue I see is at connect time, if an APN
hasn't been specified. Is it a logic bug that we're not selecting
(perhaps arbitrarily) a CID in this case (in simple_state_machine)?
I'm not sure where the responsibility lies here.

    - Nathan

  So I don't
> think the patch as sent is the right approach; perhaps we should
> g_assert(cid > 1) in the other cases, and be more relaxed for
> disconnect?  Or in disconnect if we dont' get a valid CID, we just send
> "ATH" as a desperate attempt to kill everything since something went
> wrong...
>
> Dan
>
>>
>> I've attached a patch that (I think) fixes this, by having _get_cid()
>> return 1 and having connection_enabled() use it instead of
>> mm_generic_gsm_get_cid(); however, I have a feeling that the way it
>> works now is deliberate and I'm missing something about how the no-APN
>> case is supposed to work.
>> Thoughts? Or, if this analysis seems correct, the patch should be
>> ready to go.
>>
>>
>>     - Nathan
>>
>>
>> _______________________________________________
>> networkmanager-list mailing list
>> networkmanager-list gnome org
>> http://mail.gnome.org/mailman/listinfo/networkmanager-list
>
>
>