Re: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 11:24:54, "Slark Xiao" <slark_xiao@163.com> wrote:
>
>
>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);
>>                ^^^^^^^^^^^^^^^^^^^^^^^^

>>
Hi Sergey,
Can we modify it like this?

+	if (err) {
+		ida_free(&minors, minor);
+		port->dev.class = NULL;

This shall be able to avoid the double free issue.

>>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