Re: [PATCH v5 03/10] mbimmodem: add support for MBIM extensions * With MBIMEx 3.0, arguments for activating GPRS changed. Update as needed.
Andres Salomon <[email protected]> Fri, 28 Nov 2025 03:46:25 -0500
| Newsgroups | dev.linux.lists.ofono |
|---|---|
| Message-ID | <[email protected]> |
On 11/27/25 12:49, Muhammad Asif wrote:
> ---
> drivers/mbimmodem/gprs-context.c | 64 +++++++++++++++++++++++++-------
> drivers/mbimmodem/mbim.c | 13 +++++++
> drivers/mbimmodem/mbim.h | 6 +++
> drivers/mbimmodem/mbimmodem.h | 2 +-
> drivers/mbimmodem/sim.c | 4 ++
> drivers/mbimmodem/util.c | 1 -
> plugins/mbim.c | 40 ++++++++++++++++++++
> 7 files changed, 114 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/mbimmodem/gprs-context.c b/drivers/mbimmodem/gprs-context.c
> index c420e300..4ef5e10f 100644
> --- a/drivers/mbimmodem/gprs-context.c
> +++ b/drivers/mbimmodem/gprs-context.c
> @@ -91,6 +91,8 @@ static void mbim_gprs_deactivate_primary(struct ofono_gprs_context *gc,
> ofono_gprs_context_cb_t cb, void *data)
> {
> struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
> + struct ofono_modem *modem = ofono_gprs_context_get_modem(gc);
> + uint16_t mbimex_version = ofono_modem_get_integer(modem, "MBIMExVersion");
> struct mbim_message *message;
>
> DBG("cid %u", cid);
> @@ -102,9 +104,16 @@ static void mbim_gprs_deactivate_primary(struct ofono_gprs_context *gc,
> message = mbim_message_new(mbim_uuid_basic_connect,
> MBIM_CID_CONNECT,
> MBIM_COMMAND_TYPE_SET);
> - mbim_message_set_arguments(message, "uusssuuu16y",
> - cid, 0, NULL, NULL, NULL, 0, 0, 0,
> - mbim_context_type_internet);
> +
> + if (mbim_device_mbimex_version_equal(mbimex_version, 3, 0)) {
> + mbim_message_set_arguments(message, "uuuuu16yussss",
> + cid, 0, 0, 0, 0, mbim_context_type_internet,
> + 0, NULL, NULL, NULL, NULL);
> + } else {
> + mbim_message_set_arguments(message, "uusssuuu16y",
> + cid, 0, NULL, NULL, NULL, 0, 0, 0,
> + mbim_context_type_internet);
> + }
>
> if (mbim_device_send(gcd->device, GPRS_CONTEXT_GROUP, message,
> mbim_deactivate_cb, gc, NULL) > 0)
> @@ -121,6 +130,7 @@ static void mbim_ip_configuration_cb(struct mbim_message *message, void *user)
> struct ofono_gprs_context *gc = user;
> struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
> struct ofono_modem *modem = ofono_gprs_context_get_modem(gc);
> + uint16_t mbimex_version = ofono_modem_get_integer(modem, "MBIMExVersion");
> const char *interface;
> uint32_t session_id;
> uint32_t ipv4_config_available;
> @@ -288,10 +298,18 @@ error:
> message = mbim_message_new(mbim_uuid_basic_connect,
> MBIM_CID_CONNECT,
> MBIM_COMMAND_TYPE_SET);
> - mbim_message_set_arguments(message, "uusssuuu16y",
> +
> + if (mbim_device_mbimex_version_equal(mbimex_version, 3, 0)) {
> + mbim_message_set_arguments(message, "uuuuu16yussss",
> + gcd->active_context, 0, 0, 0, 0,
> + mbim_context_type_internet, 0, NULL,
> + NULL, NULL, NULL);
> + } else {
> + mbim_message_set_arguments(message, "uusssuuu16y",
> gcd->active_context, 0,
> NULL, NULL, NULL, 0, 0, 0,
> mbim_context_type_internet);
> + }
>
> if (!mbim_device_send(gcd->device, GPRS_CONTEXT_GROUP, message,
> NULL, NULL, NULL))
> @@ -331,6 +349,8 @@ static void mbim_gprs_activate_primary(struct ofono_gprs_context *gc,
> ofono_gprs_context_cb_t cb, void *data)
> {
> struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
> + struct ofono_modem *modem = ofono_gprs_context_get_modem(gc);
> + uint16_t mbimex_version = ofono_modem_get_integer(modem, "MBIMExVersion");
> struct mbim_message *message;
> const char *username = NULL;
> const char *password = NULL;
> @@ -352,16 +372,32 @@ static void mbim_gprs_activate_primary(struct ofono_gprs_context *gc,
> message = mbim_message_new(mbim_uuid_basic_connect,
> MBIM_CID_CONNECT,
> MBIM_COMMAND_TYPE_SET);
> - mbim_message_set_arguments(message, "uusssuuu16y",
> - ctx->cid,
> - 1, /* MBIMActivationCommandActivate */
> - ctx->apn,
> - username,
> - password,
> - 0, /*MBIMCompressionNone */
> - auth_method_to_auth_protocol(ctx->auth_method),
> - proto_to_context_ip_type(ctx->proto),
> - mbim_context_type_internet);
> +./drivers/isimodem/sms.c
> + if (mbim_device_mbimex_version_equal(mbimex_version, 3, 0)) {
> + mbim_message_set_arguments(message, "uuuuu16yussss",
> + ctx->cid,
> + 1, /* MBIMActivationCommandActivate */
> + 0, /* MBIMCompressionNone */
> + auth_method_to_auth_protocol(ctx->auth_method),
> + proto_to_context_ip_type(ctx->proto),
> + mbim_context_type_internet,
> + 0, /* MBIMMediaTypeNone */
> + ctx->apn,
> + username,
> + password,
> + NULL);
> + } else {
> + mbim_message_set_arguments(message, "uusssuuu16y",
> + ctx->cid,
> + 1, /* MBIMActivationCommandActivate */
> + ctx->apn,
> + username,
> + password,
> + 0, /* MBIMCompressionNone */
> + auth_method_to_auth_protocol(ctx->auth_method),
> + proto_to_context_ip_type(ctx->proto),
> + mbim_context_type_internet);
> + }
>
> if (mbim_device_send(gcd->device, GPRS_CONTEXT_GROUP, message,
> mbim_activate_cb, gc, NULL) > 0)
> diff --git a/drivers/mbimmodem/mbim.c b/drivers/mbimmodem/mbim.c
> index c405761d..646b8055 100644
> --- a/drivers/mbimmodem/mbim.c
> +++ b/drivers/mbimmodem/mbim.c
> @@ -107,6 +107,11 @@ const uint8_t mbim_context_type_local[] = {
> 0x03, 0x3C, 0x39, 0xF6, 0x0D, 0xB9,
> };
>
> +const uint8_t mbim_ms_basic_connect_extensions[] = {
> + 0x3D, 0x01, 0xDC, 0xC5, 0xFE, 0xF5, 0x4D, 0x05, 0x0D, 0x3A,
> + 0xBE, 0xF7, 0x05, 0x8E, 0x9A, 0xAF,
> +};
> +
> struct message_assembly_node {
> struct mbim_message_header msg_hdr;
> struct mbim_fragment_header frag_hdr;
> @@ -1039,6 +1044,14 @@ bool mbim_device_set_ready_handler(struct mbim_device *device,
> return true;
> }
>
> +bool mbim_device_mbimex_version_equal(uint16_t mbimex_version,
> + int version_major, int version_minor)
The name here implies you're checking for strict equality. Similar to
the isi driver, I would suggest naming it
mbim_device_mbimex_version_at_least(...).
You could also pass it an ofono_modem* instead of mbimex_version, since
you're not using mbimex_version anywhere else.
> +{
> + return ((mbimex_version >> 8) > version_major) ||
> + (((mbimex_version >> 8) == version_major) &&
> + ((mbimex_version & 0xFF) >= version_minor));
> +}
> +
> uint32_t mbim_device_send(struct mbim_device *device, uint32_t gid,
> struct mbim_message *message,
> mbim_device_reply_func_t function,
> diff --git a/drivers/mbimmodem/mbim.h b/drivers/mbimmodem/mbim.h
> index 5f15d0a2..6ef7150b 100644
> --- a/drivers/mbimmodem/mbim.h
> +++ b/drivers/mbimmodem/mbim.h
> @@ -31,6 +31,8 @@ struct mbim_message;
> #define MBIM_CID_IP_PACKET_FILTERS 23
> #define MBIM_CID_MULTICARRIER_PROVIDERS 24
>
> +#define MBIM_CID_MS_BASIC_CONNECT_EXTENSIONS_VERSION 15
> +
> #define MBIM_CID_SMS_CONFIGURATION 1
> #define MBIM_CID_SMS_READ 2
> #define MBIM_CID_SMS_SEND 3
> @@ -87,6 +89,7 @@ extern const uint8_t mbim_uuid_phonebook[];
> extern const uint8_t mbim_uuid_stk[];
> extern const uint8_t mbim_uuid_auth[];
> extern const uint8_t mbim_uuid_dss[];
> +extern const uint8_t mbim_ms_basic_connect_extensions[];
>
> extern const uint8_t mbim_context_type_none[];
> extern const uint8_t mbim_context_type_internet[];
> @@ -118,6 +121,9 @@ bool mbim_device_set_ready_handler(struct mbim_device *device,
> void *user_data,
> mbim_device_destroy_func_t destroy);
>
> +bool mbim_device_mbimex_version_equal(uint16_t mbimex_version,
> + int version_major, int version_minor);
> +
> uint32_t mbim_device_send(struct mbim_device *device, uint32_t gid,
> struct mbim_message *message,
> mbim_device_reply_func_t function,
> diff --git a/drivers/mbimmodem/mbimmodem.h b/drivers/mbimmodem/mbimmodem.h
> index 96432738..a517877e 100644
> --- a/drivers/mbimmodem/mbimmodem.h
> +++ b/drivers/mbimmodem/mbimmodem.h
> @@ -13,4 +13,3 @@ enum MBIM_GROUP {
> SMS_GROUP = 3,
> GPRS_GROUP = 4,
> GPRS_CONTEXT_GROUP = 101,
> -};
> +};
What is this?
> diff --git a/drivers/mbimmodem/sim.c b/drivers/mbimmodem/sim.c
> index df8d73ce..fb23609a 100644
> --- a/drivers/mbimmodem/sim.c
> +++ b/drivers/mbimmodem/sim.c
> @@ -385,7 +385,9 @@ static void mbim_subscriber_ready_status_changed(struct mbim_message *message,
> void *user)
> {
> struct ofono_sim *sim = user;
> + struct ofono_modem *modem = ofono_sim_get_modem(sim);
> struct sim_data *sd = ofono_sim_get_data(sim);
> + uint16_t mbimex_version = ofono_modem_get_integer(modem, "MBIMExVersion");
> uint32_t ready_state;
> char *imsi;
> char *iccid;
> @@ -413,7 +415,9 @@ static void mbim_subscriber_ready_status_cb(struct mbim_message *message,
> void *user)
> {
> struct ofono_sim *sim = user;
> + struct ofono_modem *modem = ofono_sim_get_modem(sim);
> struct sim_data *sd = ofono_sim_get_data(sim);
> + uint16_t mbimex_version = ofono_modem_get_integer(modem, "MBIMExVersion");
> uint32_t ready_state;
> char *imsi;
> char *iccid;
And these two should probably go into patch #5?
> diff --git a/drivers/mbimmodem/util.c b/drivers/mbimmodem/util.c
> index b4e61d69..4a3d9627 100644
> --- a/drivers/mbimmodem/util.c
> +++ b/drivers/mbimmodem/util.c
> @@ -37,4 +37,3 @@ int mbim_data_class_to_tech(uint32_t n)
>
> return -1;
> }
> -
> diff --git a/plugins/mbim.c b/plugins/mbim.c
> index eeedfdac..1f82a3e4 100644
> --- a/plugins/mbim.c
> +++ b/plugins/mbim.c
> @@ -281,6 +281,30 @@ error:
> mbim_device_shutdown(md->device);
> }
>
> +static void mbim_device_mbimex_version_cb(struct mbim_message *message,
> + void *user)
> +{
> + struct ofono_modem *modem = user;
> + struct mbim_data *md = ofono_modem_get_data(modem);
> + uint16_t mbim_version, mbimex_version;
Keep in mind that mbim_message_get_arguments() below CAN fail, in which
case you've got random memory (hopefully zeroed out, but...) in
mbim*_version. I'd suggest initializing mbim_version to 1 and
mbimex_version here to 0; since that's your fallback, it gets rid of
your goto and other stuff.
Eg:
struct mbim_data *md = ofono_modem_get_data(modem);
/* Fallback to MBIM 1.0 with no extensions */
uint16_t mbim_version = (1 << 8) | 0;
uint16_t mbimex_version = 0;
if (mbim_message_get_error(message) == 0) {
mbim_message_get_arguments(message, "qq",
&mbim_version, &mbimex_version);
}
ofono_modem_set_integer(...);
> +
> + if (mbim_message_get_error(message) != 0) {
> + /* Fallback to MBIM 1.0 with no extensions */
> + mbim_version = (1 << 8) | 0;
> + mbimex_version = (0 << 8) | 0;
> + goto version_set;
> + }
> +
> + mbim_message_get_arguments(message, "qq",> + &mbim_version,
&mbimex_version);
> +
> +version_set:
> + ofono_modem_set_integer(modem, "MBIMVersion",
> + mbim_version);
> + ofono_modem_set_integer(modem, "MBIMExVersion",
> + mbimex_version);
> +}
> +
> static void mbim_device_closed(void *user_data)
> {
> struct ofono_modem *modem = user_data;
> @@ -298,6 +322,22 @@ static void mbim_device_ready(void *user_data)
> struct mbim_data *md = ofono_modem_get_data(modem);
> struct mbim_message *message;
>
> + /* Version is formatted as (major version) << 8 | (minor version) */
> + static const int mbim_version = 1 << 8 | 0;
> +
> + /* Always open with MBIMEx 3, devices not supporting it will fallback to MBIMEx 2 */
> + static const int mbimex_version = 3 << 8 | 0;
A minor nit, but 'q' in mbim_message_set_arguments() is a uint16_t, so
these should probably also use that instead of int.
> +
> + message = mbim_message_new(mbim_ms_basic_connect_extensions,
> + MBIM_CID_MS_BASIC_CONNECT_EXTENSIONS_VERSION,
> + MBIM_COMMAND_TYPE_QUERY);
> + mbim_message_set_arguments(message, "qq",
> + mbim_version,
> + mbimex_version);
> +
> + mbim_device_send(md->device, 0, message,
> + mbim_device_mbimex_version_cb, modem, NULL);
> +
> message = mbim_message_new(mbim_uuid_basic_connect,
> MBIM_CID_DEVICE_CAPS,
> MBIM_COMMAND_TYPE_QUERY)
I think both your code AND the existing code are missing calls to
mbim_message_unref(). Though I just did a cursory scan, so maybe the
messages are being unref'd and freed somewhere else?
;