Re:Re: [net-next,v7,3/8] net: wwan: core: split port creation and registration

["Slark Xiao" <slark_xiao@163.com>]

At 2026-01-16 10:44:35, "Jakub Kicinski" <kuba@kernel.org> wrote:
>This is an AI-generated review of your patch. The human sending this
>email has considered the AI review valid, or at least plausible.
>
>For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
>---
>net: wwan: core: split port creation and registration
>
>This patch splits the port creation function to prepare for upcoming GNSS
>port type support. It separates device_register() into device_initialize()
>and device_add(), and moves minor number allocation into the new
>wwan_port_register_wwan() function.
>
>> diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
>> index 1da935e84008..1a9a77d597e6 100644
>> --- a/drivers/net/wwan/wwan_core.c
>> +++ b/drivers/net/wwan/wwan_core.c
>> @@ -357,7 +357,8 @@ static void wwan_port_destroy(struct device *dev)
>>  {
>>  	struct wwan_port *port = to_wwan_port(dev);
>>
>> -	ida_free(&minors, MINOR(port->dev.devt));
>> +	if (dev->class == &wwan_class)
>> +		ida_free(&minors, MINOR(dev->devt));
>>  	mutex_destroy(&port->data_lock);
>>  	mutex_destroy(&port->ops_lock);
>>  	kfree(port);
>
>[ ... ]
>
>> +/* Register a regular WWAN port device (e.g. AT, MBIM, etc.) */
>> +static int wwan_port_register_wwan(struct wwan_port *port)
>> +{
>> +	struct wwan_device *wwandev = to_wwan_dev(port->dev.parent);
>> +	char namefmt[0x20];
>> +	int minor, err;
>> +
>> +	/* A port is exposed as character device, get a minor */
>> +	minor = ida_alloc_range(&minors, 0, WWAN_MAX_MINORS - 1, GFP_KERNEL);
>> +	if (minor < 0)
>> +		return minor;
>> +
>> +	port->dev.class = &wwan_class;
>> +	port->dev.devt = MKDEV(wwan_major, minor);
>
>[ ... ]
>
>> +	err = device_add(&port->dev);
>> +
>> +	mutex_unlock(&wwan_register_lock);
>> +
>> +	if (err) {
>> +		ida_free(&minors, minor);
>                ^^^^^^^^^^^^^^^^^^^^^^^^
>
>When device_add() fails, this frees the minor. However, port->dev.class
>was already set to wwan_class above, and port->dev.devt contains the minor
>number. The caller (wwan_create_port) then calls put_device() which
>triggers wwan_port_destroy(). Since dev->class == &wwan_class is true,
>wwan_port_destroy() calls ida_free() again on the same minor.
>
>Wouldn't this cause a double ida_free when device_add() fails? The call
>chain is: wwan_port_register_wwan() fails -> ida_free(minor) -> return
>error -> wwan_create_port() -> put_device() -> wwan_port_destroy() ->
>ida_free(minor) again.
>
>Maybe the class should be cleared on error before returning, or the
>ida_free in the error path should be removed since wwan_port_destroy()
>will handle it?
>
Hi Sergey,
We need to review this changes again.

>> +		return err;
>> +	}
>> +
>> +	dev_info(&wwandev->dev, "port %s attached\n", dev_name(&port->dev));
>> +
>> +	return 0;
>> +}
>
>[ ... ]
>
>> +	device_initialize(&port->dev);
>> +
>> +	err = wwan_port_register_wwan(port);
>>  	if (err)
>>  		goto error_put_device;
>> +
>>  	return port;
>>
>>  error_put_device:
>> +	put_device(&port->dev);
>-- 
>pw-bot: cr