Re: ModemManager: improving port probing

[Dan Williams <dcbw redhat com>]

On Tue, 2011-09-20 at 17:07 +0200, Aleksander Morgado wrote:
> Hi all,
> 
> I've been lately trying to improve the probing process in ModemManager,
> so that it is faster and easier to maintain, keeping in mind the
> different specific needs of the current plugins.
> 
> The current status of the work can be seen in the 'probing-refactor'
> branch in the following git repo:
> git://gitorious.org/aleksander/modemmanager.git It currently only
> compiles some plugins (Cinterion, Nokia and Generic) which are the ones
> ported to the new probing process being suggested here. The branch is
> organized in logical commits so that it can be easily reviewed (hope so,
> at least).
> 
> I had mixed feelings on whether I should suggest all these new things
> before having it fully ready, but I think it makes sense to at least
> discuss them before I spend more time on it.
> 
> 
> 
> 1. Plugin Manager
> -----------------------------------------------
> 
> The MMManager object handled too many different tasks, that it made
> sense to me to have an independent MMPluginManager object which takes
> care of: 
>  * Finding plugins and loading them
>  * Controlling the requests to find the best plugin to support a given
> port.
> 
> So whenever a new port is found/detected, we can just pass it to
> mm_plugin_manager_find_port_support() and wait for the async method to
> return the best plugin found for it. When devices with multiple ports
> are detected, the Plugin Manager also takes care of propagating the
> first best plugin found to support checks of ports in the same device.
> 
> This Plugin Manager work is at the beginning of the branch; and quite
> independent to the rest of the changes (in other words, directly
> mergeable to master if you guys like it).

Sounds like a good idea.  One thing to remember is that if a plugin
claims a port, all other ports of that physical device should be given
to that plugin (by comparing the port's udev parent or grandparent).
Don't really want a situation where we might have two plugins trying to
fight for the same actual device.

> 
> 
> 2. Port Support Checks
> -----------------------------------------------
> 
> The whole probing process is now done in a new MMPortProbe object
> (somewhat equivalent to the MMPluginBaseSupportsTask object, which was
> removed). The object serves as a temporary storage for already probed
> information (as the MMPluginBaseSupportsTask one), but also allows
> specifying what needs to be probed in the port and does also the real
> probing.

Yay!  MMPluginBaseSupportsTask was getting really complicated.  Die die die.

> The MMPluginBase will control whether a probing is needed, and will also
> process the results of the probing reported by the MMPortProbe object.
> Specific plugins are able to set at construction time different property
> values to modify the both the probing process and the pre- and
> post-probing filters to run. See the ported Nokia plugin as example:
> https://gitorious.org/aleksander/modemmanager/commit/d2992abdb1f386962063e6b61ae8bf26eb0f995a

I forget whether any of the plugins need to modify the probing commands
they use at runtime, but would this approach preclude that?  x22x for
example only sends the AT+GMR request if the vendor+product ID is
0x1bbb/0x0000.  We don't need it for other modems.

> The whole port support check done by every plugin can be summarized in
> the following steps:
>  * [MMPluginBase] Check whether we need to launch probing (pre-probing
> filters).
>  * [MMPortProbe] Launch port probing, get probing results.
>  * [MMPluginBase] Check whether the results we got are the ones expected
> (post-probing filters).
>  * [MMPluginBase] Report port support check results.
> 
> 
> 2.1 Pre-probing filters
> 
> Pre-probing filters are those which control whether the probing, as
> requested by the specific plugin, takes place. The best example of this
> filter type is the VENDOR_IDS one: The probing request will be aborted
> (and therefore report as unsupported) unless the port reports a Vendor
> ID equal to one in the given array.

Sounds fine too.  Though at least in one instance, Alcatel/TCT branded
devices use the same vendor/product ID for modems with two different
firmware types which should be handled by two different modems, and the
only way we can differentiate that is by the AT+GMR response.
 
>  * MM_PLUGIN_BASE_ALLOWED_VENDOR_IDS: A plugin can set this property to
> a 0-terminated array of uint16s containing the udev-reported vendor IDs
> it can handle, e.g.:
> 
>     static const guint16 vendor_ids[] = { 0x0421 , 0 };
> 
>     return MM_PLUGIN (
>         g_object_new (MM_TYPE_PLUGIN_NOKIA,
>                       ...
>                       MM_PLUGIN_BASE_ALLOWED_VENDOR_IDS, vendor_ids,
>                       NULL));
> 
> Other pre-probing filters considered and already implemented are
> Subsystems, Drivers, Product ID and Udev Tags. See:
> https://gitorious.org/aleksander/modemmanager/commit/142d0161070777a8a8cac44e8320b07b70fb437a
> 
> 
> 2.2 Probing-specific configurations
> 
> If the port passed all pre-probing filters, a probing process can be
> started. The specific probing to be done directly depends on the
> requested post-probing filters. Some examples:
>   ** Cinterion modems do not expose QCDM ports, therefore no QCDM port
> probing is needed.
>   ** ZTE modems are never RS232, therefore no Vendor/Product AT-probing
> is needed.
> 
> If all the requested probing information is already cached, the probing
> is not launched again. This may happen when probing a port in a RS232
> modem, where capabilities and Vendor-string probing may be done by an
> earlier plugin.
> 
> When any AT-specific probing is requested (capabilities, vendor,
> product), we first check whether the port is an AT port or not (just
> sending "AT" and not expecting any error). If the port is found to be an
> AT port, capabilities/vendor/product probing can continue. We will only
> check if the port is to be QCDM if the plugin object was created with
> MM_PLUGIN_BASE_ALLOWED_QCDM set to TRUE.

Sounds fine, though in most cases a QCDM port can be exposed by any
kernel serial driver.  But we'll know for some plugins that we never
expect QCDM ports (Ericsson/mbm, Nokia) so we can ignore QCDM probing
for those.  Most other plugins *could* handle QCDM ports though, even if
they currently don't.

> As before, plugins are allowed to specify custom initialization
> commands, in this case with the MM_PLUGIN_BASE_CUSTOM_INIT property
> which expects a NULL-terminated array of the new MMPortProbeAtCommand
> structs. This custom initialization can also be used to check whether
> the port is an AT port or not. As an example, the Nokia plugin may use:

We've got a few cases of this:

1) novatel: the main port needs to run DMAT=1 which flips the secondary
port into AT command mode, and then the secondary port can be probed as
an AT port

2) huawei: turn off unsolicited messages on the secondary port, which if
it's not opened and being listened to, the firmware appears to keep
sending them, run out of buffer space because nothing is listening, and
then crash.  we also try to get the portmap right away so that we have
hints about what type of ports the others are

3) longcheer/x22x: grab the AT+GMR response because this is the only way
to distinguish between the X200 and the X060s modems, one of which is
driven by longcheer, and the other by x22x, since they both use the same
USB product/vendor but use completely different firmwares, and only the
AT+GMR response is unique

4) zte: same sort of thing as huawei, they flood the secondary port with
unsolicited indications the first time you open it, and eventually fill
up the firmware serial buffer and crash

>     static gboolean
>     parse_init (const gchar *response,
>                 const GError *error,
>                 GValue *result,
>                 GError **result_error)
>     {
>         if (error)
>             /* Go on to next command */
>             return FALSE;
> 
>         /* If we didn't get any error, it is an AT port */
>         g_value_init (result, G_TYPE_BOOLEAN);
>         g_value_set_boolean (result, TRUE);
>         return TRUE;
>     }
> 
>     static gboolean
>     parse_init_last (const gchar *response,
>                      const GError *error,
>                      GValue *result,
>                      GError **result_error)
>     {
>         g_value_init (result, G_TYPE_BOOLEAN);
>         /* On last error, report as not being an AT port */
>         g_value_set_boolean (result, error ? FALSE : TRUE);
>         return TRUE;
>     }
> 
>     /* ------ */
> 
>     static const MMPortProbeAtCommand custom_init[] = {
>         { "ATE1 E0", parse_init },
>         { "ATE1 E0", parse_init },
>         { "ATE1 E0", parse_init_last },
>         { NULL }
>     };

Looks good; though I wonder how we'd handle the x22x/longcheer case of
only probing AT+GMR based on specific vendor/product IDs.  We don't
*need* to restrict it to specific vendor/product IDs though, but other
types of probing we might want to restrict with various other filters.

>     return MM_PLUGIN (
>         g_object_new (MM_TYPE_PLUGIN_NOKIA,
>                       ...
>                       MM_PLUGIN_BASE_CUSTOM_INIT, custom_init,
>                       NULL));
> 
> This is, "ATE1 E0" will be sent up to 3 times, and if we kept getting
> errors in all 3 tries, the port is reported as not being an AT port.
> Being able to say whether a port is AT or not from the custom init is
> just about convenience, the custom init command is of course not for
> that and the callback is allowed to return without setting the output
> 'result'. Also, if the callback sets 'result_error', the whole probing
> process would get cancelled. See:
> https://gitorious.org/aleksander/modemmanager/commit/40e66ea506101a0af6922bde24ad9579392f0429

This sounds fine.

> 
> 2.3 Post-probing filters
> 
> Post-probing filters are those which control whether the plugin can
> support the port once the desired probing results are ready. The best
> example of this filter type is the CAPABILITIES one: The support request
> will report unsupported unless the probed capabilities from the port
> match the capabilities given in the property.
> 
>  * MM_PLUGIN_BASE_ALLOWED_CAPABILITIES: A plugin can set this property
> to a guint representing a mask of flags where supported capabilities are
> given, e.g.:
> 
>     static const guint32 capabilities =
> MM_PORT_PROBE_CAPABILITY_GSM_OR_CDMA;
> 
>     return MM_PLUGIN (
>         g_object_new (MM_TYPE_PLUGIN_NOKIA,
>                       ...
>                       MM_PLUGIN_BASE_ALLOWED_CAPABILITIES, capabilities,
>                       NULL));
> 
> Other post-probing filters considered and implemented are Vendor Strings
> and Product Strings (used only in plugins supporting RS232 devices).
> See:
> https://gitorious.org/aleksander/modemmanager/commit/bc1d62ddb58abbff3eea21196deac8cf9eff3473

Need the GMR results for x22x/longcheer for the Alcatel/TCT device too.
Sounds fine though.

> 
> 2.4 Port support results
> 
> Given a port, there is always a single plugin that may support it. MM
> currently reports 'support level' values (given as an integer in the
> [0-100] range), but the truth seems to be that only one single plugin
> ends up reporting a support_level > 0 (due to the udev or AT-probed
> vendor/product filtering). Therefore, a new SUPPORTED state was added to
> the possible replies of the port support check done by the plugins, to
> be used instead of reporting a support_level > 0.

Yeah, we can ditch support_level, that was an idea that never, ever
happened and is pointless.  Only one plugin will support a modem at any
given time.

> The whole supports_port() method was also changed so that it always
> reports results asynchronously (so there is no longer need for
> IN_PROGRESS status). This eases a lot the management of port support
> requests within the Plugin Manager. See:
> https://gitorious.org/aleksander/modemmanager/commit/b07189f14fe6e88a87cd8db5312d9fdf6734e680

Sweet.

> 
> 3. TODOs
> -----------------------------------------------
> 
> 3.1 Port Grabbing: In the same way that the port support checking was
> fully moved to the MMPluginBase, port grabbing can also possibly be
> completely moved out of the plugin implementations. For example, letting
> plugins specify in a new property the GType of the modem object to
> create when GSM capability is found, and such.

One thing we want to fix up with this is allowing plugins with multiple
capabilities, since there are modems out there (Qualcomm ones) that
support both GSM and CDMA that we just don't handle right now.  So this
is intimately tied into the multi-capability modem changes we've talked
about extensively.  Ideally the probing process does just enough to
determine that the port is (a) an [AT, qcdm, qmi, wmc, cns, ethernet]
port and (b) what plugin to use for that port and that's it; hopefully
we can punt the GSM/CDMA/LTE capabilities decision until later because
that's not always available via AT+GCAP for example.  This would help us
get the modems detected more quickly too.

Also of note is that we still need to ensure the blacklist/whitelist
stuff works, since people have Uninteruptible Power Supplies that
sometimes crash when you send them the wrong strings, and by default
these get probed by MM right now unless the whitelist/blacklist is
installed.

> 3.2 Port remaining plugins to the new probing process. I first wanted to
> discuss all these ideas in the list before going on with that.
> 
> 
> Comments?

I'd port at least ZTE over to the new probing process, I'm happy to test
since I have like 50 ZTE modems.

But basically, if you say its easier, clearer, faster, and less
error-prone, I trust you that the solution you've come up with is those
things and I'm happy to rely on your judgment.  I think the current
probing code has gotten pretty ugly and a cleanup is most welcome.

Dan