| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAFEp6-0kBH2HMVAWK_CAoo-Hd3FU8k-54L1tzvBnqs=eS39Gkg@mail.gmail.com>
Date: Mon, 14 Apr 2025 20:50:06 +0200
From: Loic Poulain <loic.poulain@....qualcomm.com>
To: Sergey Ryazanov <ryazanov.s.a@...il.com>
Cc: Johannes Berg <johannes@...solutions.net>,
Andrew Lunn <andrew+netdev@...n.ch>,
Eric Dumazet <edumazet@...gle.com>,
"David S . Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
netdev@...r.kernel.org
Subject: Re: [RFC PATCH 2/6] net: wwan: core: split port creation and registration
Hi Sergey,
On Wed, Apr 9, 2025 at 1:31 AM Sergey Ryazanov <ryazanov.s.a@...il.com> wrote:
>
> 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.
>
> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@...il.com>
> ---
> drivers/net/wwan/wwan_core.c | 86 ++++++++++++++++++++++--------------
> 1 file changed, 53 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
> index ade8bbffc93e..045246d7cd50 100644
> --- a/drivers/net/wwan/wwan_core.c
> +++ b/drivers/net/wwan/wwan_core.c
> @@ -357,16 +357,19 @@ static struct attribute *wwan_port_attrs[] = {
> };
> ATTRIBUTE_GROUPS(wwan_port);
>
> -static void wwan_port_destroy(struct device *dev)
> +static void __wwan_port_destroy(struct wwan_port *port)
> {
> - struct wwan_port *port = to_wwan_port(dev);
> -
> - ida_free(&minors, MINOR(port->dev.devt));
> mutex_destroy(&port->data_lock);
> mutex_destroy(&port->ops_lock);
> kfree(port);
> }
>
> +static void wwan_port_destroy(struct device *dev)
> +{
> + ida_free(&minors, MINOR(dev->devt));
> + __wwan_port_destroy(to_wwan_port(dev));
> +}
> +
> static const struct device_type wwan_port_dev_type = {
> .name = "wwan_port",
> .release = wwan_port_destroy,
> @@ -440,6 +443,49 @@ 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.)
> + *
> + * NB: in case of error function frees the port memory.
> + */
> +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) {
> + __wwan_port_destroy(port);
I see this is documented above, but it's a bit weird that the port is
freed inside the register function, it should be up to the caller to
do this. Is there a reason for this?
> + return minor;
> + }
> +
> + port->dev.class = &wwan_class;
> + port->dev.type = &wwan_port_dev_type;
> + 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_register(&port->dev);
> +
> + mutex_unlock(&wwan_register_lock);
> +
> + if (err) {
> + put_device(&port->dev);
> + 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,
> @@ -448,8 +494,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 +506,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,31 +522,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);
>
> - /* 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;
> + goto error_wwandev_remove;
>
> - dev_info(&wwandev->dev, "port %s attached\n", dev_name(&port->dev));
> return port;
>
> -error_put_device:
> - put_device(&port->dev);
> error_wwandev_remove:
> wwan_remove_dev(wwandev);
>
> --
> 2.45.3
>
Powered by blists - more mailing lists