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

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