Re:RE: [net-next v4 2/8] net: wwan: core: split port creation and registration

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

At 2026-01-07 00:49:50, "Sai Krishna Gajula" <saikrishnag@marvell.com> wrote:
>
>> -----Original Message-----
>> From: Slark Xiao <slark_xiao@163.com>
>> Sent: Monday, January 5, 2026 3:50 PM
>> To: loic.poulain@oss.qualcomm.com; ryazanov.s.a@gmail.com;
>> johannes@sipsolutions.net; andrew+netdev@lunn.ch;
>> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
>> pabeni@redhat.com; mani@kernel.org
>> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org
>> Subject: [net-next v4 2/8] net: wwan: core: split port creation and
>> registration
>> 
>> 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 
>> 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.
>> 
>> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
>> ---
>>  drivers/net/wwan/wwan_core.c | 66 ++++++++++++++++++++++--------------
>>  1 file changed, 40 insertions(+), 26 deletions(-)
>> 
>> diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
>> index ade8bbffc93e..edee5ff48f28 100644
>> --- a/drivers/net/wwan/wwan_core.c
>> +++ b/drivers/net/wwan/wwan_core.c
>> @@ -361,7 +361,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);
>> @@ -440,6 +441,41 @@ 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) {
>
>As per kernel style, braces need to be on next line 
>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)
>> +		return err;
>Please check, if freeing with ida_free is required before returning err.
>if (err) {
>    ida_free(&minors, minor);
>    return err;
>}
Yes, you are right.
And patch 7/8 modifies this file as well. We need to align with that changes
since there are some issues(we still allocates the minor even the cdev is
false). This would lead to the release function can't release the correct
devt.
>> +
>> +	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, @@ -
>> 448,8 +484,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);
>> @@ -461,17 +496,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;
>>  	}
>> 
>> @@ -485,27 +512,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
>> 
>