From: Sergey Ryazanov <ryazanov.s.a@gmail.com>

Upcoming GNSS (NMEA) port type support requires exporting it via the
GNSS subsystem. On another hand, we still need to do basic WWAN core
work: find or allocate the WWAN device, make it the port parent, etc. To
reuse as much code as possible, split the port creation function into
the registration of a regular WWAN port device, and basic port struct
initialization.

To be able to use put_device() uniformly, break the device_register()
call into device_initialize() and device_add() and call device
initialization earlier.

While at it, fix a minor number leak upon WWAN port registration
failure.

Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
 drivers/net/wwan/wwan_core.c | 68 ++++++++++++++++++++++--------------
 1 file changed, 42 insertions(+), 26 deletions(-)

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);
@@ -436,6 +437,43 @@ static int __wwan_port_dev_assign_name(struct wwan_port *port, const char *fmt)
 	return dev_set_name(&port->dev, "%s", buf);
 }
 
+/* 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);
+
+	/* allocate unique name based on wwan device id, port type and number */
+	snprintf(namefmt, sizeof(namefmt), "wwan%u%s%%d", wwandev->id,
+		 wwan_port_types[port->type].devsuf);
+
+	/* Serialize ports registration */
+	mutex_lock(&wwan_register_lock);
+
+	__wwan_port_dev_assign_name(port, namefmt);
+	err = device_add(&port->dev);
+
+	mutex_unlock(&wwan_register_lock);
+
+	if (err) {
+		ida_free(&minors, minor);
+		return err;
+	}
+
+	dev_info(&wwandev->dev, "port %s attached\n", dev_name(&port->dev));
+
+	return 0;
+}
+
 struct wwan_port *wwan_create_port(struct device *parent,
 				   enum wwan_port_type type,
 				   const struct wwan_port_ops *ops,
@@ -444,8 +482,7 @@ struct wwan_port *wwan_create_port(struct device *parent,
 {
 	struct wwan_device *wwandev;
 	struct wwan_port *port;
-	char namefmt[0x20];
-	int minor, err;
+	int err;
 
 	if (type > WWAN_PORT_MAX || !ops)
 		return ERR_PTR(-EINVAL);
@@ -457,17 +494,9 @@ struct wwan_port *wwan_create_port(struct device *parent,
 	if (IS_ERR(wwandev))
 		return ERR_CAST(wwandev);
 
-	/* 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) {
-		err = minor;
-		goto error_wwandev_remove;
-	}
-
 	port = kzalloc(sizeof(*port), GFP_KERNEL);
 	if (!port) {
 		err = -ENOMEM;
-		ida_free(&minors, minor);
 		goto error_wwandev_remove;
 	}
 
@@ -481,27 +510,14 @@ struct wwan_port *wwan_create_port(struct device *parent,
 	mutex_init(&port->data_lock);
 
 	port->dev.parent = &wwandev->dev;
-	port->dev.class = &wwan_class;
 	port->dev.type = &wwan_port_dev_type;
-	port->dev.devt = MKDEV(wwan_major, minor);
 	dev_set_drvdata(&port->dev, drvdata);
+	device_initialize(&port->dev);
 
-	/* allocate unique name based on wwan device id, port type and number */
-	snprintf(namefmt, sizeof(namefmt), "wwan%u%s%%d", wwandev->id,
-		 wwan_port_types[port->type].devsuf);
-
-	/* Serialize ports registration */
-	mutex_lock(&wwan_register_lock);
-
-	__wwan_port_dev_assign_name(port, namefmt);
-	err = device_register(&port->dev);
-
-	mutex_unlock(&wwan_register_lock);
-
+	err = wwan_port_register_wwan(port);
 	if (err)
 		goto error_put_device;
 
-	dev_info(&wwandev->dev, "port %s attached\n", dev_name(&port->dev));
 	return port;
 
 error_put_device:
-- 
2.25.1

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

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

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

On Thu, Jan 22, 2026 at 3:32 AM Slark Xiao <slark_xiao@163.com> wrote:
>
>
>
> 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.

I would prefer to let wwan_port_destroy() handle this, meaning we
shouldn’t free the IDA here and should simply return the error code,
but your approach is also acceptable.

Regards,
Loic