[PATCH v4 0/6] mbim: Add support for MediaTek T700 5G

[PATCH v4 0/6] mbim: Add support for MediaTek T700 5G

[Muhammad Asif]

This modem is a PCIe MBIM modem, found in a lot of modern laptops,
like the Lenovo ThinkPad X1. Unfortunately, oFono does not support
PCIe MBIM modems out of the box, so this patch adds support for them,
as well as MBIM extensions which are needed for newer modems.

Muhammad Asif (6):
  plugins/udevng: Add support for PCIe MBIM modems * Parses through the
    sysfs tree and detects MBIM control nodes, net and   AT nodes
  mbimmodem: add support for MBIM extensions * With MBIMEx 3.0,
    arguments for activating GPRS changed. Update as   needed.
  mbim: fetch MBIM version on init
  plugins/udevng: Add support for the MediaTek T700 5G modem * Found in
    the Lenovo ThinkPad X1 Yoga Gen 8 as the Fibocom FM350-GL   modem *
    Works perfectly with ofono now
  mbimmodem: sim: add support for querying IMSI with MBIMEx 3 This
    commit adds the ability to query IMSI with newer modems that use the
    MBIMEx 3 protocol.
  mbimmodem: sim: add support for querying ICCID This implements the
    required functions to query file info and read file info from the
    modem. Currently the other file IDs haven't been implemented, but
    they will be tackled later.

 drivers/mbimmodem/gprs-context.c | 59 +++++++++++++++++++------
 drivers/mbimmodem/mbim.c         | 55 +++++++++++++++++++++++
 drivers/mbimmodem/mbim.h         |  7 +++
 drivers/mbimmodem/sim.c          | 75 +++++++++++++++++++++++++++++---
 plugins/mbim.c                   |  2 +
 plugins/udevng.c                 | 57 ++++++++++++++++++++++++
 6 files changed, 235 insertions(+), 20 deletions(-)

-- 
2.49.0

[PATCH v4 1/6] plugins/udevng: Add support for PCIe MBIM modems * Parses through the sysfs tree and detects MBIM control nodes, net and AT nodes

[Muhammad Asif]

---
 plugins/udevng.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/plugins/udevng.c b/plugins/udevng.c
index b8df66de..09c916db 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -1176,7 +1176,13 @@ static gboolean setup_mbim(struct modem_info *modem)
 {
 	const char *ctl = NULL, *net = NULL, *atcmd = NULL;
 	GSList *list;
+	const char *sub_subsystem = NULL, *type = NULL;
+	char path[512], wwan_path[1024], sub_path[2048];
 	char descriptors[PATH_MAX];
+	struct udev *new_udev;
+	struct udev_device *wwan_device, *sub_device;
+	struct dirent *dir = NULL, *subdir = NULL;
+	DIR *d = NULL, *sd = NULL;
 
 	DBG("%s [%s:%s]", modem->syspath, modem->vendor, modem->model);
 
@@ -1196,6 +1202,56 @@ static gboolean setup_mbim(struct modem_info *modem)
 		else if (g_strcmp0(subsystem, "tty") == 0) {
 			if (g_strcmp0(info->number, "02") == 0)
 				atcmd = info->devnode;
+		} else if (g_strcmp0(subsystem, "pci") == 0) {
+			sprintf(path, "%s/wwan", udev_device_get_syspath(info->udev_device));
+
+			d = opendir(path);
+			if (!d)
+				return FALSE;
+
+			new_udev = udev_new();
+
+			while ((dir = readdir(d))) {
+				if (g_strcmp0(dir->d_name, ".") == 0 ||
+					g_strcmp0(dir->d_name, "..") == 0)
+					continue;
+
+				sprintf(wwan_path, "%s/%s", path, dir->d_name);
+
+				wwan_device = udev_device_new_from_syspath(new_udev,
+									wwan_path);
+				net = udev_device_get_sysname(wwan_device);
+
+				// Parse all the subdirectories now
+				sd = opendir(wwan_path);
+				while ((subdir = readdir(sd))) {
+					if (subdir->d_type != DT_DIR)
+						continue;
+
+					if (g_strcmp0(subdir->d_name, ".") == 0 ||
+						g_strcmp0(subdir->d_name, "..") == 0)
+						continue;
+
+					sprintf(sub_path, "%s/%s", wwan_path, subdir->d_name);
+
+					sub_device = udev_device_new_from_syspath(new_udev,
+										sub_path);
+					sub_subsystem = udev_device_get_subsystem(sub_device);
+
+					if (g_strcmp0(sub_subsystem, "wwan") == 0) {
+						type = udev_device_get_sysattr_value(sub_device, "type");
+
+						if (g_strcmp0(type, "MBIM") == 0)
+							ctl = udev_device_get_devnode(sub_device);
+						else if (g_strcmp0(type, "AT") == 0)
+							atcmd = udev_device_get_devnode(sub_device);
+					}
+				}
+				closedir(sd);
+				break;
+			}
+			closedir(d);
+			udev_unref(new_udev);
 		}
 	}
 
-- 
2.49.0

[PATCH v4 2/6] mbimmodem: add support for MBIM extensions * With MBIMEx 3.0, arguments for activating GPRS changed. Update as needed.

[Muhammad Asif]

---
 drivers/mbimmodem/gprs-context.c | 59 ++++++++++++++++++++++++--------
 drivers/mbimmodem/mbim.c         | 55 +++++++++++++++++++++++++++++
 drivers/mbimmodem/mbim.h         |  7 ++++
 3 files changed, 107 insertions(+), 14 deletions(-)

diff --git a/drivers/mbimmodem/gprs-context.c b/drivers/mbimmodem/gprs-context.c
index c420e300..e912b6a5 100644
--- a/drivers/mbimmodem/gprs-context.c
+++ b/drivers/mbimmodem/gprs-context.c
@@ -102,9 +102,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_check_mbimex_version(gcd->device, 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)
@@ -288,10 +295,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_check_mbimex_version(gcd->device, 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))
@@ -352,16 +367,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);
+
+	if (mbim_device_check_mbimex_version(gcd->device, 3, 0)) {
+		mbim_message_set_arguments(message, "uuuuu16yussss",
+			ctx->cid,
+			1, // Activate
+			0, // Compression
+			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, // Activate
+			ctx->apn,
+			username,
+			password,
+			0, // compression
+			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 43ff9704..be4df986 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;
@@ -253,6 +258,11 @@ struct mbim_device {
 	struct message_assembly *assembly;
 	struct l_idle *close_io;
 
+	uint8_t mbim_version_major;
+	uint8_t mbim_version_minor;
+	uint8_t mbimex_version_major;
+	uint8_t mbimex_version_minor;
+
 	bool is_ready : 1;
 	bool in_notify : 1;
 };
@@ -875,6 +885,22 @@ static bool close_read_handler(struct l_io *io, void *user_data)
 	return true;
 }
 
+static void parse_mbim_version(struct mbim_message *message, void *user_data)
+{
+	struct mbim_device *device = user_data;
+	uint16_t mbim_version = 0, mbimex_version = 0;
+
+	if (mbim_message_get_error(message) != 0)
+		return;
+
+	mbim_message_get_arguments(message, "qq", &mbim_version, &mbimex_version);
+
+	device->mbim_version_major = mbim_version >> 8;
+	device->mbim_version_minor = mbim_version & 0xFF;
+	device->mbimex_version_major = mbimex_version >> 8;
+	device->mbimex_version_minor = mbimex_version & 0xFF;
+}
+
 struct mbim_device *mbim_device_new(int fd, uint32_t max_segment_size)
 {
 	struct mbim_device *device;
@@ -1035,6 +1061,35 @@ bool mbim_device_set_ready_handler(struct mbim_device *device,
 	return true;
 }
 
+void mbim_device_get_version(struct mbim_device *device)
+{
+	// Version is formatted as (major version) << 8 | (minor version)
+	const int mbim_version = 1 << 8 | 0;
+
+	// Always open with MBIMEx 3, devices not supporting it will fallback to MBIMEx 2
+	const int mbimex_version = 3 << 8 | 0;
+
+	struct mbim_message *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);
+
+	// For some reason, sending a message will return an error code, even if valid
+	mbim_device_send(device, 0, message,
+						parse_mbim_version, device, NULL);
+}
+
+bool mbim_device_check_mbimex_version(struct mbim_device *device,
+					int version_major, int version_minor)
+{
+	return (device->mbimex_version_major > version_major) ||
+			((device->mbimex_version_major == version_major) &&
+			(device->mbimex_version_minor >= 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..4a6c8f47 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,10 @@ bool mbim_device_set_ready_handler(struct mbim_device *device,
 					void *user_data,
 					mbim_device_destroy_func_t destroy);
 
+void mbim_device_get_version(struct mbim_device *device);
+bool mbim_device_check_mbimex_version(struct mbim_device *device,
+					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,
-- 
2.49.0

[PATCH v4 3/6] mbim: fetch MBIM version on init

[Muhammad Asif]

---
 plugins/mbim.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/plugins/mbim.c b/plugins/mbim.c
index e1d33f47..3a1942da 100644
--- a/plugins/mbim.c
+++ b/plugins/mbim.c
@@ -287,6 +287,8 @@ static int mbim_enable(struct ofono_modem *modem)
 					mbim_device_closed, modem, NULL);
 	mbim_device_set_debug(md->device, mbim_debug, "MBIM:", NULL);
 
+	mbim_device_get_version(md->device);
+
 	return -EINPROGRESS;
 }
 
-- 
2.49.0

[PATCH v4 4/6] plugins/udevng: Add support for the MediaTek T700 5G modem * Found in the Lenovo ThinkPad X1 Yoga Gen 8 as the Fibocom FM350-GL modem * Works perfectly with ofono now

[Muhammad Asif]

---
 plugins/udevng.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/plugins/udevng.c b/plugins/udevng.c
index 09c916db..5ca745c2 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -2282,6 +2282,7 @@ static const struct {
 	const char *pid;
 } pci_driver_list[] = {
 	{ "xmm7xxx",	"imc_ipc",	"0x8086",	"0x7560"},
+	{ "mbim",		"mtk_t7xx",	"0x14c3",	"0x4d75"},
 	{ }
 };
 
-- 
2.49.0

[PATCH v4 5/6] mbimmodem: sim: add support for querying IMSI with MBIMEx 3 This commit adds the ability to query IMSI with newer modems that use the MBIMEx 3 protocol.

[Muhammad Asif]

---
 drivers/mbimmodem/sim.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/mbimmodem/sim.c b/drivers/mbimmodem/sim.c
index df8d73ce..8f9998a7 100644
--- a/drivers/mbimmodem/sim.c
+++ b/drivers/mbimmodem/sim.c
@@ -390,12 +390,22 @@ static void mbim_subscriber_ready_status_changed(struct mbim_message *message,
 	char *imsi;
 	char *iccid;
 	uint32_t ready_info;
+	uint32_t ready_flags;
+	bool r;
 
 	DBG("");
 
-	if (!mbim_message_get_arguments(message, "ussu",
-					&ready_state, &imsi,
-					&iccid, &ready_info))
+	if (mbim_device_check_mbimex_version(sd->device, 3, 0)) {
+		r = mbim_message_get_arguments(message, "uussu",
+						&ready_state, &ready_flags, &imsi,
+						&iccid, &ready_info);
+	} else {
+		r = mbim_message_get_arguments(message, "ussu",
+						&ready_state, &imsi,
+						&iccid, &ready_info);
+	}
+
+	if (!r)
 		return;
 
 	l_free(sd->iccid);
@@ -418,6 +428,7 @@ static void mbim_subscriber_ready_status_cb(struct mbim_message *message,
 	char *imsi;
 	char *iccid;
 	uint32_t ready_info;
+	uint32_t ready_flags;
 	bool r;
 
 	DBG("");
@@ -426,9 +437,16 @@ static void mbim_subscriber_ready_status_cb(struct mbim_message *message,
 		goto error;
 
 	/* We don't bother parsing MSISDN/MDN array */
-	r = mbim_message_get_arguments(message, "ussu",
-					&ready_state, &imsi,
-					&iccid, &ready_info);
+	if (mbim_device_check_mbimex_version(sd->device, 3, 0)) {
+		r = mbim_message_get_arguments(message, "uussu",
+						&ready_state, &ready_flags, &imsi,
+						&iccid, &ready_info);
+	} else {
+		r = mbim_message_get_arguments(message, "ussu",
+						&ready_state, &imsi,
+						&iccid, &ready_info);
+	}
+
 	if (!r)
 		goto error;
 
-- 
2.49.0

[PATCH v4 6/6] mbimmodem: sim: add support for querying ICCID This implements the required functions to query file info and read file info from the modem. Currently the other file IDs haven't been implemented, but they will be tackled later.

[Muhammad Asif]

---
 drivers/mbimmodem/sim.c | 45 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/mbimmodem/sim.c b/drivers/mbimmodem/sim.c
index 8f9998a7..c5bd2049 100644
--- a/drivers/mbimmodem/sim.c
+++ b/drivers/mbimmodem/sim.c
@@ -18,6 +18,9 @@
 #include <ofono/modem.h>
 #include <ofono/sim.h>
 
+#include <glib.h>
+#include "simutil.h"
+
 #include "drivers/mbimmodem/mbim.h"
 #include "drivers/mbimmodem/mbim-message.h"
 #include "drivers/mbimmodem/mbimmodem.h"
@@ -76,6 +79,45 @@ static void mbim_read_imsi(struct ofono_sim *sim,
 	CALLBACK_WITH_SUCCESS(cb, sd->imsi, user_data);
 }
 
+static void mbim_read_file_info(struct ofono_sim *sim,
+					int fileid, const unsigned char *path,
+					unsigned int path_len,
+					ofono_sim_file_info_cb_t cb, void *user_data)
+{
+	unsigned char access[3] = {0x0f, 0xff, 0xff};
+
+	switch (fileid) {
+		case SIM_EF_ICCID_FILEID:
+			CALLBACK_WITH_SUCCESS(cb, 10, 0, 0, access, 1, user_data);
+			break;
+		default:
+			CALLBACK_WITH_FAILURE(cb, -1, -1, -1, NULL, 0, user_data);
+			break;
+	}
+}
+
+static void mbim_read_file_transparent(struct ofono_sim *sim,
+					int fileid,	int start, int length,
+					const unsigned char *path, unsigned int path_len,
+					ofono_sim_read_cb_t cb, void *user_data)
+{
+	struct sim_data *sd = ofono_sim_get_data(sim);
+	unsigned char iccid[10];
+	int iccid_len, len = strlen(sd->imsi);
+
+	sim_encode_bcd_number(sd->iccid, iccid);
+	iccid_len = len / 2;
+
+	switch (fileid) {
+		case SIM_EF_ICCID_FILEID:
+			CALLBACK_WITH_SUCCESS(cb, iccid, iccid_len, user_data);
+			break;
+		default:
+			CALLBACK_WITH_FAILURE(cb, NULL, 0, user_data);
+			break;
+	}
+}
+
 static enum ofono_sim_password_type mbim_pin_type_to_sim_password(
 							uint32_t pin_type)
 {
@@ -453,6 +495,7 @@ static void mbim_subscriber_ready_status_cb(struct mbim_message *message,
 	sd->iccid = iccid;
 	sd->imsi = imsi;
 
+	DBG("Got ICCID! %s", iccid);
 	if (!mbim_device_register(sd->device, SIM_GROUP,
 					mbim_uuid_basic_connect,
 					MBIM_CID_SUBSCRIBER_READY_STATUS,
@@ -516,6 +559,8 @@ static void mbim_sim_remove(struct ofono_sim *sim)
 static const struct ofono_sim_driver driver = {
 	.probe			= mbim_sim_probe,
 	.remove			= mbim_sim_remove,
+	.read_file_info 	= mbim_read_file_info,
+	.read_file_transparent 	= mbim_read_file_transparent,
 	.read_imsi		= mbim_read_imsi,
 	.query_passwd_state	= mbim_pin_query,
 	.query_pin_retries	= mbim_pin_retries_query,
-- 
2.49.0

Re: [PATCH v4 1/6] plugins/udevng: Add support for PCIe MBIM modems * Parses through the sysfs tree and detects MBIM control nodes, net and AT nodes

[Denis Kenzior]

Hi Muhammad,

On 5/4/25 2:19 PM, Muhammad Asif wrote:
> ---
>   plugins/udevng.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 56 insertions(+)
> 

Some general comments:

oFono still prefers that lines are limited to 80 characters, so please 
restructure accordingly.  Also, checkpatch still flags some other issues:

0001-plugins-udevng-Add-support-for-PCIe-MBIM-modems-Pars.patch:8: WARNING: 
Missing commit description - Add an appropriate one
0001-plugins-udevng-Add-support-for-PCIe-MBIM-modems-Pars.patch:70: WARNING: 
line length of 105 exceeds 100 columns
0001-plugins-udevng-Add-support-for-PCIe-MBIM-modems-Pars.patch:72: WARNING: Too 
many leading tabs - consider code refactoring
0001-plugins-udevng-Add-support-for-PCIe-MBIM-modems-Pars.patch:74: WARNING: Too 
many leading tabs - consider code refactoring

> diff --git a/plugins/udevng.c b/plugins/udevng.c
> index b8df66de..09c916db 100644
> --- a/plugins/udevng.c
> +++ b/plugins/udevng.c
> @@ -1176,7 +1176,13 @@ static gboolean setup_mbim(struct modem_info *modem)
>   {
>   	const char *ctl = NULL, *net = NULL, *atcmd = NULL;
>   	GSList *list;
> +	const char *sub_subsystem = NULL, *type = NULL;
> +	char path[512], wwan_path[1024], sub_path[2048];
>   	char descriptors[PATH_MAX];
> +	struct udev *new_udev;
> +	struct udev_device *wwan_device, *sub_device;
> +	struct dirent *dir = NULL, *subdir = NULL;
> +	DIR *d = NULL, *sd = NULL;
>   
>   	DBG("%s [%s:%s]", modem->syspath, modem->vendor, modem->model);
>   

I see that you added "mbim" to a list of pci drivers in patch 4.  This patch and 
patch 4 should be combined into a single patch.  Can you provide a log from 
'ofonod -nd'?  Especially after patch 4, oFono should be detecting all nodes 
related to this device.  Or, hmm...

Perhaps there needs to be some additional logic in check_wwan_device() as well? 
Right now it only detects MHI devices.

> @@ -1196,6 +1202,56 @@ static gboolean setup_mbim(struct modem_info *modem)
>   		else if (g_strcmp0(subsystem, "tty") == 0) {
>   			if (g_strcmp0(info->number, "02") == 0)
>   				atcmd = info->devnode;
> +		} else if (g_strcmp0(subsystem, "pci") == 0) {
> +			sprintf(path, "%s/wwan", udev_device_get_syspath(info->udev_device));
> +
> +			d = opendir(path);
> +			if (!d)
> +				return FALSE;

Hmm, so you are detecting some node (I assume root) of the device, then open 
coding custom detection logic based on the wwan subsystem provided sysfs links. 
What if the WWAN subsystem isn't enabled in the kernel?  Can udevng be made to 
work without these links?

At a high level, if WWAN subsystem is enabled, then udevng is redundant.  There 
needs to be a dedicated wwan subsystem detection plugin, however this is a 
missing piece in oFono at the moment.  Marcel, didn't you have some beginnings 
of this?

> +
> +			new_udev = udev_new();
> +
> +			while ((dir = readdir(d))) {
> +				if (g_strcmp0(dir->d_name, ".") == 0 ||
> +					g_strcmp0(dir->d_name, "..") == 0)
> +					continue;
> +
> +				sprintf(wwan_path, "%s/%s", path, dir->d_name);
> +
> +				wwan_device = udev_device_new_from_syspath(new_udev,
> +									wwan_path);
> +				net = udev_device_get_sysname(wwan_device);
> +
> +				// Parse all the subdirectories now

nit: We use C comments, not C++

> +				sd = opendir(wwan_path);
> +				while ((subdir = readdir(sd))) {
> +					if (subdir->d_type != DT_DIR)
> +						continue;
> +
> +					if (g_strcmp0(subdir->d_name, ".") == 0 ||
> +						g_strcmp0(subdir->d_name, "..") == 0)
> +						continue;
> +
> +					sprintf(sub_path, "%s/%s", wwan_path, subdir->d_name);
> +
> +					sub_device = udev_device_new_from_syspath(new_udev,
> +										sub_path);
> +					sub_subsystem = udev_device_get_subsystem(sub_device);
> +
> +					if (g_strcmp0(sub_subsystem, "wwan") == 0) {
> +						type = udev_device_get_sysattr_value(sub_device, "type");
> +
> +						if (g_strcmp0(type, "MBIM") == 0)
> +							ctl = udev_device_get_devnode(sub_device);
> +						else if (g_strcmp0(type, "AT") == 0)
> +							atcmd = udev_device_get_devnode(sub_device);
> +					}
> +				}
> +				closedir(sd);
> +				break;
> +			}
> +			closedir(d);
> +			udev_unref(new_udev);
>   		}
>   	}
>   

Regards,
-Denis

Re: [PATCH v4 2/6] mbimmodem: add support for MBIM extensions * With MBIMEx 3.0, arguments for activating GPRS changed. Update as needed.

[Denis Kenzior]

Hi Muhammad,

On 5/4/25 2:19 PM, Muhammad Asif wrote:
> ---
>   drivers/mbimmodem/gprs-context.c | 59 ++++++++++++++++++++++++--------
>   drivers/mbimmodem/mbim.c         | 55 +++++++++++++++++++++++++++++
>   drivers/mbimmodem/mbim.h         |  7 ++++
>   3 files changed, 107 insertions(+), 14 deletions(-)
> 

<snip>

> @@ -253,6 +258,11 @@ struct mbim_device {
>   	struct message_assembly *assembly;
>   	struct l_idle *close_io;
>   
> +	uint8_t mbim_version_major;
> +	uint8_t mbim_version_minor;
> +	uint8_t mbimex_version_major;
> +	uint8_t mbimex_version_minor;
> +

In the current design, mbim_device doesn't really manage such state.  This 
belongs in plugins/mbim.c instead.

>   	bool is_ready : 1;
>   	bool in_notify : 1;
>   };
> @@ -875,6 +885,22 @@ static bool close_read_handler(struct l_io *io, void *user_data)
>   	return true;
>   }
>   
> +static void parse_mbim_version(struct mbim_message *message, void *user_data)
> +{
> +	struct mbim_device *device = user_data;
> +	uint16_t mbim_version = 0, mbimex_version = 0;
> +
> +	if (mbim_message_get_error(message) != 0)
> +		return;
> +
> +	mbim_message_get_arguments(message, "qq", &mbim_version, &mbimex_version);
> +
> +	device->mbim_version_major = mbim_version >> 8;
> +	device->mbim_version_minor = mbim_version & 0xFF;
> +	device->mbimex_version_major = mbimex_version >> 8;
> +	device->mbimex_version_minor = mbimex_version & 0xFF;
> +}
> +

The version should be obtained similar to how capabilities are obtained.

>   struct mbim_device *mbim_device_new(int fd, uint32_t max_segment_size)
>   {
>   	struct mbim_device *device;
> @@ -1035,6 +1061,35 @@ bool mbim_device_set_ready_handler(struct mbim_device *device,
>   	return true;
>   }
>   
> +void mbim_device_get_version(struct mbim_device *device)
> +{
> +	// Version is formatted as (major version) << 8 | (minor version)
> +	const int mbim_version = 1 << 8 | 0;

nit: static const?

> +
> +	// Always open with MBIMEx 3, devices not supporting it will fallback to MBIMEx 2
> +	const int mbimex_version = 3 << 8 | 0;

nit: ditto, also please use C style comments

> +
> +	struct mbim_message *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);
> +
> +	// For some reason, sending a message will return an error code, even if valid
> +	mbim_device_send(device, 0, message,
> +						parse_mbim_version, device, NULL);

Hmm, this is suspicious.  mbim_device_send should never fail, unless the device 
isn't open.

> +}
> +
> +bool mbim_device_check_mbimex_version(struct mbim_device *device,
> +					int version_major, int version_minor)

The naming needs some work?  Perhaps mbim_device_mbimex_version_at_least()? 
However, since mbim_device doesn't really manage this part, consider using a 
modem property instead?  Or pass the mbimex version to the atom driver directly. 
  See for example how QMI drivers do this with the new 'probev' driver method.

> +{
> +	return (device->mbimex_version_major > version_major) ||
> +			((device->mbimex_version_major == version_major) &&
> +			(device->mbimex_version_minor >= version_minor));
> +}
> +
>   uint32_t mbim_device_send(struct mbim_device *device, uint32_t gid,
>   				struct mbim_message *message,
>   				mbim_device_reply_func_t function,

Regards,
-Denis

Re: [PATCH v4 3/6] mbim: fetch MBIM version on init

[Denis Kenzior]

Hi Muhammad,

On 5/4/25 2:19 PM, Muhammad Asif wrote:
> ---
>   plugins/mbim.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/plugins/mbim.c b/plugins/mbim.c
> index e1d33f47..3a1942da 100644
> --- a/plugins/mbim.c
> +++ b/plugins/mbim.c
> @@ -287,6 +287,8 @@ static int mbim_enable(struct ofono_modem *modem)
>   					mbim_device_closed, modem, NULL);
>   	mbim_device_set_debug(md->device, mbim_debug, "MBIM:", NULL);
>   
> +	mbim_device_get_version(md->device);
> +

I think you need to invoke this after mbim_device_ready() has been invoked.

>   	return -EINPROGRESS;
>   }
>   

Regards,
-Denis

Re: [PATCH v4 6/6] mbimmodem: sim: add support for querying ICCID This implements the required functions to query file info and read file info from the modem. Currently the other file IDs haven't been implemented, but they will be tackled later.

[Denis Kenzior]

Hi Muhammad,

On 5/4/25 2:19 PM, Muhammad Asif wrote:
> ---
>   drivers/mbimmodem/sim.c | 45 +++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 45 insertions(+)
> 

<snip>

> +static void mbim_read_file_transparent(struct ofono_sim *sim,
> +					int fileid,	int start, int length,
> +					const unsigned char *path, unsigned int path_len,
> +					ofono_sim_read_cb_t cb, void *user_data)
> +{
> +	struct sim_data *sd = ofono_sim_get_data(sim);
> +	unsigned char iccid[10];
> +	int iccid_len, len = strlen(sd->imsi);
> +
> +	sim_encode_bcd_number(sd->iccid, iccid);
> +	iccid_len = len / 2;

Hmm, len is based on the IMSI, not iccid?  Also, the number will be truncated, 
not rounded up.  Do you mean something like:

sim_encode_bcd_number(sd->iccid, iccid);
iccid_len = (strlen(sd->iccid) + 1) / 2;

?

> +
> +	switch (fileid) {
> +		case SIM_EF_ICCID_FILEID:
> +			CALLBACK_WITH_SUCCESS(cb, iccid, iccid_len, user_data);
> +			break;
> +		default:
> +			CALLBACK_WITH_FAILURE(cb, NULL, 0, user_data);
> +			break;
> +	}
> +}
> +
>   static enum ofono_sim_password_type mbim_pin_type_to_sim_password(
>   							uint32_t pin_type)
>   {

Regards,
-Denis

Re: [PATCH v4 1/6] plugins/udevng: Add support for PCIe MBIM modems * Parses through the sysfs tree and detects MBIM control nodes, net and AT nodes

[Muhammad]

Hello Denis,

Thank you for the reply.

On 5/5/25 20:46, Denis Kenzior wrote:
> Hi Muhammad,
>
> On 5/4/25 2:19 PM, Muhammad Asif wrote:
>
>> diff --git a/plugins/udevng.c b/plugins/udevng.c
>> index b8df66de..09c916db 100644
>> --- a/plugins/udevng.c
>> +++ b/plugins/udevng.c
>> @@ -1176,7 +1176,13 @@ static gboolean setup_mbim(struct modem_info 
>> *modem)
>>   {
>>       const char *ctl = NULL, *net = NULL, *atcmd = NULL;
>>       GSList *list;
>> +    const char *sub_subsystem = NULL, *type = NULL;
>> +    char path[512], wwan_path[1024], sub_path[2048];
>>       char descriptors[PATH_MAX];
>> +    struct udev *new_udev;
>> +    struct udev_device *wwan_device, *sub_device;
>> +    struct dirent *dir = NULL, *subdir = NULL;
>> +    DIR *d = NULL, *sd = NULL;
>>         DBG("%s [%s:%s]", modem->syspath, modem->vendor, modem->model);
>
> I see that you added "mbim" to a list of pci drivers in patch 4. This 
> patch and patch 4 should be combined into a single patch. Can you 
> provide a log from 'ofonod -nd'?  Especially after patch 4, oFono 
> should be detecting all nodes related to this device. Or, hmm...
>
> Perhaps there needs to be some additional logic in check_wwan_device() 
> as well? Right now it only detects MHI devices.
>
I tried this actually, oFono cannot comb through the nodes because the 
subsystem shows up as PCI, not WWAN. So I had to manually parse through 
all of the WWAN nodes.

On 5/5/25 20:46, Denis Kenzior wrote:

>> @@ -1196,6 +1202,56 @@ static gboolean setup_mbim(struct modem_info 
>> *modem)
>>           else if (g_strcmp0(subsystem, "tty") == 0) {
>>               if (g_strcmp0(info->number, "02") == 0)
>>                   atcmd = info->devnode;
>> +        } else if (g_strcmp0(subsystem, "pci") == 0) {
>> +            sprintf(path, "%s/wwan", 
>> udev_device_get_syspath(info->udev_device));
>> +
>> +            d = opendir(path);
>> +            if (!d)
>> +                return FALSE;
>
> Hmm, so you are detecting some node (I assume root) of the device, 
> then open coding custom detection logic based on the wwan subsystem 
> provided sysfs links. What if the WWAN subsystem isn't enabled in the 
> kernel?  Can udevng be made to work without these links?

Without the WWAN subsystem, the kernel does not enumerate the WWAN 
nodes, and also does not include the wwan/ directories in sysfs. oFono 
will handle that and report the modem as unusable.

- Muhammad

Re: [PATCH v4 2/6] mbimmodem: add support for MBIM extensions * With MBIMEx 3.0, arguments for activating GPRS changed. Update as needed.

[Muhammad]

Hi Denis,

On 5/5/25 21:06, Denis Kenzior wrote:
>> @@ -253,6 +258,11 @@ struct mbim_device {
>>       struct message_assembly *assembly;
>>       struct l_idle *close_io;
>>   +    uint8_t mbim_version_major;
>> +    uint8_t mbim_version_minor;
>> +    uint8_t mbimex_version_major;
>> +    uint8_t mbimex_version_minor;
>> +
>
> In the current design, mbim_device doesn't really manage such state.  
> This belongs in plugins/mbim.c instead.
I will move this to the plugins part.
>>       bool is_ready : 1;
>>       bool in_notify : 1;
>>   };
>> @@ -875,6 +885,22 @@ static bool close_read_handler(struct l_io *io, 
>> void *user_data)
>>       return true;
>>   }
>>   +static void parse_mbim_version(struct mbim_message *message, void 
>> *user_data)
>> +{
>> +    struct mbim_device *device = user_data;
>> +    uint16_t mbim_version = 0, mbimex_version = 0;
>> +
>> +    if (mbim_message_get_error(message) != 0)
>> +        return;
>> +
>> +    mbim_message_get_arguments(message, "qq", &mbim_version, 
>> &mbimex_version);
>> +
>> +    device->mbim_version_major = mbim_version >> 8;
>> +    device->mbim_version_minor = mbim_version & 0xFF;
>> +    device->mbimex_version_major = mbimex_version >> 8;
>> +    device->mbimex_version_minor = mbimex_version & 0xFF;
>> +}
>> +
>
> The version should be obtained similar to how capabilities are obtained.
>
>>   struct mbim_device *mbim_device_new(int fd, uint32_t max_segment_size)
>>   {
>>       struct mbim_device *device;
>> @@ -1035,6 +1061,35 @@ bool mbim_device_set_ready_handler(struct 
>> mbim_device *device,
>>       return true;
>>   }
>>   +void mbim_device_get_version(struct mbim_device *device)
>> +{
>> +    // Version is formatted as (major version) << 8 | (minor version)
>> +    const int mbim_version = 1 << 8 | 0;
>
> nit: static const?
>
>> +
>> +    // Always open with MBIMEx 3, devices not supporting it will 
>> fallback to MBIMEx 2
>> +    const int mbimex_version = 3 << 8 | 0;
>
> nit: ditto, also please use C style comments
>
>> +
>> +    struct mbim_message *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);
>> +
>> +    // For some reason, sending a message will return an error code, 
>> even if valid
>> +    mbim_device_send(device, 0, message,
>> +                        parse_mbim_version, device, NULL);
>
> Hmm, this is suspicious.  mbim_device_send should never fail, unless 
> the device isn't open.
>
>> +}
>> +
>> +bool mbim_device_check_mbimex_version(struct mbim_device *device,
>> +                    int version_major, int version_minor)
>
> The naming needs some work?  Perhaps 
> mbim_device_mbimex_version_at_least()? However, since mbim_device 
> doesn't really manage this part, consider using a modem property 
> instead?  Or pass the mbimex version to the atom driver directly.  See 
> for example how QMI drivers do this with the new 'probev' driver method.
>
>> +{
>> +    return (device->mbimex_version_major > version_major) ||
>> +            ((device->mbimex_version_major == version_major) &&
>> +            (device->mbimex_version_minor >= version_minor));
>> +}
>> +
>>   uint32_t mbim_device_send(struct mbim_device *device, uint32_t gid,
>>                   struct mbim_message *message,
>>                   mbim_device_reply_func_t function,

I will move the version into a modem property.

- Muhammad

Re: [PATCH v4 3/6] mbim: fetch MBIM version on init

[Muhammad]

Hi Denis,

On 5/5/25 21:07, Denis Kenzior wrote:
> Hi Muhammad,
>
> On 5/4/25 2:19 PM, Muhammad Asif wrote:
>> ---
>>   plugins/mbim.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/plugins/mbim.c b/plugins/mbim.c
>> index e1d33f47..3a1942da 100644
>> --- a/plugins/mbim.c
>> +++ b/plugins/mbim.c
>> @@ -287,6 +287,8 @@ static int mbim_enable(struct ofono_modem *modem)
>>                       mbim_device_closed, modem, NULL);
>>       mbim_device_set_debug(md->device, mbim_debug, "MBIM:", NULL);
>>   +    mbim_device_get_version(md->device);
>> +
>
> I think you need to invoke this after mbim_device_ready() has been 
> invoked.
>
>>       return -EINPROGRESS;
>>   }
>
> Regards,
> -Denis

I will move it to be invoked after the device is ready.

- Muhammad

Re: [PATCH v4 6/6] mbimmodem: sim: add support for querying ICCID This implements the required functions to query file info and read file info from the modem. Currently the other file IDs haven't been implemented, but they will be tackled later.

[Muhammad]

Hi Denis,

On 5/5/25 21:29, Denis Kenzior wrote:
>> +static void mbim_read_file_transparent(struct ofono_sim *sim,
>> +                    int fileid,    int start, int length,
>> +                    const unsigned char *path, unsigned int path_len,
>> +                    ofono_sim_read_cb_t cb, void *user_data)
>> +{
>> +    struct sim_data *sd = ofono_sim_get_data(sim);
>> +    unsigned char iccid[10];
>> +    int iccid_len, len = strlen(sd->imsi);
>> +
>> +    sim_encode_bcd_number(sd->iccid, iccid);
>> +    iccid_len = len / 2;
>
> Hmm, len is based on the IMSI, not iccid?  Also, the number will be 
> truncated, not rounded up.  Do you mean something like:
>
> sim_encode_bcd_number(sd->iccid, iccid);
> iccid_len = (strlen(sd->iccid) + 1) / 2;
>
> ?

oFono's `sim_encode_bcd_number()` will encode the ICCID string into BCD, 
which halves the length of the string. ICCID's are 20 bytes long, so 
this just returns 10 for length. The QMI driver also does the same.

- Muhammad