Re: [PATCH v7 10/10] mbim/network-registration: add support for manual registration

On 11/28/25 12:36, Muhammad Asif wrote:
> Adds support for scanning available operators, and manually registering
> to a selected network.
[...]
> diff --git a/drivers/mbimmodem/network-registration.c b/drivers/mbimmodem/network-registration.c
> index ded1bf69..e95dd73a 100644
> --- a/drivers/mbimmodem/network-registration.c
> +++ b/drivers/mbimmodem/network-registration.c
> @@ -204,6 +204,70 @@ static void mbim_current_operator(struct ofono_netreg *netreg,
>   	CALLBACK_WITH_FAILURE(cb, NULL, data);
>   }
>   
> +static void mbim_list_operators_cb(struct mbim_message *message,
> +						void *user)
> +{
> +	struct cb_data *cb = user;
> +	ofono_netreg_operator_list_cb_t cb_func = cb->cb;
> +	struct ofono_network_operator *list = NULL;
> +
> +	uint32_t providers_count, provider_state, cellular_class;
> +	uint32_t rssi, error_rate;
> +	char *provider_id, *provider_name;
> +	struct mbim_message_iter iter;
> +	int i = 0;
> +
> +	if (!mbim_message_get_arguments(message, "ua(susuuu)", &providers_count, &iter)) {
> +		CALLBACK_WITH_FAILURE(cb_func, 0, NULL, cb->data);
> +		return;
> +	}
> +
> +	list = l_new(struct ofono_network_operator, providers_count);
> +
> +	while (mbim_message_iter_next_entry(&iter, &provider_id, &provider_state,
> +					&provider_name, &cellular_class, &rssi, &error_rate)) {
> +
> +		strcpy(list[i].name, provider_name);

This should probably be a strncpy using OFONO_MAX_OPERATOR_NAME_LENGTH.


> +		strncpy(list[i].mcc, provider_id, OFONO_MAX_MCC_LENGTH);
> +		list[i].mcc[OFONO_MAX_MCC_LENGTH] = '\0';
> +
> +		strncpy(list[i].mnc, provider_id + OFONO_MAX_MCC_LENGTH, OFONO_MAX_MNC_LENGTH);
> +		list[i].mnc[OFONO_MAX_MNC_LENGTH] = '\0';
> +
> +		list[i].status = mbim_provider_state_to_status(provider_state);
> +		list[i].tech = -1;
> +
> +		i++;
> +	}
> +
> +	if (list)
> +		CALLBACK_WITH_SUCCESS(cb_func, providers_count, list, cb->data);
> +	else
> +		CALLBACK_WITH_FAILURE(cb_func, 0, NULL, cb->data);
> +}
> +
> +static void mbim_list_operators(struct ofono_netreg *netreg,
> +					ofono_netreg_operator_list_cb_t cb, void *user_data)
> +{
> +	struct netreg_data *nd = ofono_netreg_get_data(netreg);
> +	struct cb_data *cbd = cb_data_new(cb, user_data);
> +	struct mbim_message *message;
> +
> +	message = mbim_message_new(mbim_uuid_basic_connect,
> +						MBIM_CID_VISIBLE_PROVIDERS,
> +						MBIM_COMMAND_TYPE_QUERY);
> +	/* "0" is a full scan, whereas "1" means a restricted scan */
> +	mbim_message_set_arguments(message, "u", 0);
> +
> +	if (mbim_device_send(nd->device, NETREG_GROUP, message,
> +						mbim_list_operators_cb, cbd, l_free) > 0)
> +		return;
> +
> +	l_free(cbd);
> +	mbim_message_unref(message);
> +	CALLBACK_WITH_FAILURE(cb, 0, NULL, user_data);
> +}
> +
>   static void mbim_register_state_set_cb(struct mbim_message *message, void *user)
>   {
>   	struct cb_data *cbd = user;
> @@ -233,7 +297,42 @@ static void mbim_register_auto(struct ofono_netreg *netreg,
>   	message = mbim_message_new(mbim_uuid_basic_connect,
>   					MBIM_CID_REGISTER_STATE,
>   					MBIM_COMMAND_TYPE_SET);
> -	mbim_message_set_arguments(message, "suu", NULL, 0, data_class);
> +	mbim_message_set_arguments(message, "suu", NULL,
> +					MBIM_REGISTER_TYPE_AUTOMATIC, data_class);
> +
> +	if (mbim_device_send(nd->device, NETREG_GROUP, message,
> +				mbim_register_state_set_cb, cbd, l_free) > 0)
> +		return;
> +
> +	l_free(cbd);
> +	mbim_message_unref(message);
> +	CALLBACK_WITH_FAILURE(cb, data);
> +}
> +
> +static void mbim_register_manual(struct ofono_netreg *netreg,
> +				const char *mcc, const char *mnc,
> +				ofono_netreg_register_cb_t cb, void *data)
> +{
> +	static const uint32_t data_class = MBIM_DATA_CLASS_GPRS |
> +						MBIM_DATA_CLASS_EDGE |
> +						MBIM_DATA_CLASS_UMTS |
> +						MBIM_DATA_CLASS_HSDPA |
> +						MBIM_DATA_CLASS_HSUPA |
> +						MBIM_DATA_CLASS_LTE;
> +	struct netreg_data *nd = ofono_netreg_get_data(netreg);
> +	struct cb_data *cbd = cb_data_new(cb, data);
> +	struct mbim_message *message;
> +	L_AUTO_FREE_VAR(char *, provider_id) = NULL;
> +
> +	DBG("");
> +
> +	provider_id = l_strdup_printf("%s%s", mcc, mnc);
> +
> +	message = mbim_message_new(mbim_uuid_basic_connect,
> +					MBIM_CID_REGISTER_STATE,
> +					MBIM_COMMAND_TYPE_SET);
> +	mbim_message_set_arguments(message, "suu", provider_id,
> +					MBIM_REGISTER_TYPE_MANUAL, data_class);
>   

Given that mbim_register_auto and mbim_register_manual are almost the 
exact same function except for the MBIM_REGISTER_TYPE_* argument and 
provider_id (which is leaked, btw; should probably be deleted in 
mbim_register_state_set_cb?)... maybe it makes sense to have a common 
function for these two, with mbim_register_auto and mbim_register_manual 
calling it?



>   	if (mbim_device_send(nd->device, NETREG_GROUP, message,
>   				mbim_register_state_set_cb, cbd, l_free) > 0)
> @@ -385,7 +484,9 @@ static const struct ofono_netreg_driver driver = {
>   	.remove				= mbim_netreg_remove,
>   	.registration_status		= mbim_registration_status,
>   	.current_operator		= mbim_current_operator,
> +	.list_operators			= mbim_list_operators,
>   	.register_auto			= mbim_register_auto,
> +	.register_manual		= mbim_register_manual,
>   	.strength			= mbim_signal_strength,
>   };
>   
> diff --git a/drivers/mbimmodem/util.c b/drivers/mbimmodem/util.c
> index 731764fc..45b7af9b 100644
> --- a/drivers/mbimmodem/util.c
> +++ b/drivers/mbimmodem/util.c
> @@ -39,6 +39,26 @@ int mbim_data_class_to_tech(uint32_t n)
>   	return -1;

>