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

[Jakub Kicinski]

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?

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