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

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