On Thu, Mar 11, 2021 at 09:41:03PM +0100, Loic Poulain wrote:
> This change introduces initial support for a WWAN subsystem. Given the
> complexity and heterogeneity of existing WWAN hardwares and interfaces,
> there is no strict definition of what a WWAN device is and how it should
> be represented. It's often a collection of multiple components/devices
> that perform the global WWAN feature (netdev, tty, chardev, etc).
>
> One usual way to expose modem controls and configuration is via high
> level protocols such as the well known AT command protocol, MBIM or
> QMI. The USB modems started to expose that as character devices, and
> user daemons such as ModemManager learnt how to deal with that. This
> initial version adds the concept of WWAN port, which can be registered
> by any driver to expose one of these protocols. The WWAN core takes
> care of the generic part, including character device creation and lets
> the driver implementing access (fops) to the selected protocol.
>
> Since the different components/devices do no necesserarly know about
> each others, and can be created/removed in different orders, the
> WWAN core ensures that devices being part of the same hardware are
> also represented as a unique WWAN device, relying on the provided
> parent device (e.g. mhi controller, USB device). It's a 'trick' I
> copied from Johannes's earlier WWAN subsystem proposal.
>
> This initial version is purposely minimalist, it's essentially moving
> the generic part of the previously proposed mhi_wwan_ctrl driver inside
> a common WWAN framework, but the implementation is open and flexible
> enough to allow extension for further drivers.
>
> Signed-off-by: Loic Poulain <loic.poulain@xxxxxxxxxx>
> ---
> v2: not part of the series
> v3: not part of the series
> v4: Introduce WWAN framework/subsystem
> v5: Specify WWAN_CORE module name in Kconfig
>
> drivers/net/Kconfig | 2 +
> drivers/net/Makefile | 1 +
> drivers/net/wwan/Kconfig | 22 +++++++
> drivers/net/wwan/Makefile | 8 +++
> drivers/net/wwan/wwan_core.c | 150 +++++++++++++++++++++++++++++++++++++++++++
> drivers/net/wwan/wwan_core.h | 20 ++++++
> drivers/net/wwan/wwan_port.c | 136 +++++++++++++++++++++++++++++++++++++++
> include/linux/wwan.h | 121 ++++++++++++++++++++++++++++++++++
> 8 files changed, 460 insertions(+)
> create mode 100644 drivers/net/wwan/Kconfig
> create mode 100644 drivers/net/wwan/Makefile
> create mode 100644 drivers/net/wwan/wwan_core.c
> create mode 100644 drivers/net/wwan/wwan_core.h
> create mode 100644 drivers/net/wwan/wwan_port.c
> create mode 100644 include/linux/wwan.h
>
> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index 260f9f4..ec00f92 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -500,6 +500,8 @@ source "drivers/net/wan/Kconfig"
>
> source "drivers/net/ieee802154/Kconfig"
>
> +source "drivers/net/wwan/Kconfig"
> +
> config XEN_NETDEV_FRONTEND
> tristate "Xen network device frontend driver"
> depends on XEN
> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> index f4990ff..5da6424 100644
> --- a/drivers/net/Makefile
> +++ b/drivers/net/Makefile
> @@ -68,6 +68,7 @@ obj-$(CONFIG_SUNGEM_PHY) += sungem_phy.o
> obj-$(CONFIG_WAN) += wan/
> obj-$(CONFIG_WLAN) += wireless/
> obj-$(CONFIG_IEEE802154) += ieee802154/
> +obj-$(CONFIG_WWAN) += wwan/
>
> obj-$(CONFIG_VMXNET3) += vmxnet3/
> obj-$(CONFIG_XEN_NETDEV_FRONTEND) += xen-netfront.o
> diff --git a/drivers/net/wwan/Kconfig b/drivers/net/wwan/Kconfig
> new file mode 100644
> index 0000000..545fe54
> --- /dev/null
> +++ b/drivers/net/wwan/Kconfig
> @@ -0,0 +1,22 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# Wireless WAN device configuration
> +#
> +
> +menuconfig WWAN
> + bool "Wireless WAN"
> + help
> + This section contains Wireless WAN driver configurations.
> +
> +if WWAN
> +
> +config WWAN_CORE
> + tristate "WWAN Driver Core"
> + help
> + Say Y here if you want to use the WWAN driver core. This driver
> + provides a common framework for WWAN drivers.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called wwan.
> +
> +endif # WWAN
> diff --git a/drivers/net/wwan/Makefile b/drivers/net/wwan/Makefile
> new file mode 100644
> index 0000000..ca8bb5a
> --- /dev/null
> +++ b/drivers/net/wwan/Makefile
> @@ -0,0 +1,8 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Makefile for the Linux WWAN device drivers.
> +#
> +
> +obj-$(CONFIG_WWAN_CORE) += wwan.o
> +wwan-objs += wwan_core.o wwan_port.o
> +
> diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
> new file mode 100644
> index 0000000..42ba6c0
> --- /dev/null
> +++ b/drivers/net/wwan/wwan_core.c
> @@ -0,0 +1,150 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (c) 2021, Linaro Ltd <loic.poulain@xxxxxxxxxx> */
> +
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/fs.h>
> +#include <linux/idr.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/wwan.h>
> +
> +#include "wwan_core.h"
> +
> +static LIST_HEAD(wwan_list); /* list of registered wwan devices */
Why do you need a list as you already have a list of them all in the
class structure?
> +static DEFINE_IDA(wwan_ida);
> +static DEFINE_MUTEX(wwan_global_lock);
What is this lock for? I don't think you need a lock for a ida/idr
structure if you use it in the "simple" mode, right?
> +struct class *wwan_class;
Why is this a global structure?
> +
> +static struct wwan_device *__wwan_find_by_parent(struct device *parent)
> +{
> + struct wwan_device *wwandev;
> +
> + if (!parent)
> + return NULL;
> +
> + list_for_each_entry(wwandev, &wwan_list, list) {
> + if (wwandev->dev.parent == parent)
> + return wwandev;
Nice, no locking!
:(
Again, why not use the driver core bits for this?
Also, no reference counting is used here, sure way to cause problems :(
> + }
> +
> + return NULL;
> +}
> +
> +static void wwan_dev_release(struct device *dev)
> +{
> + struct wwan_device *wwandev = to_wwan_dev(dev);
> +
> + kfree(wwandev);
> +}
> +
> +static const struct device_type wwan_type = {
> + .name = "wwan",
> + .release = wwan_dev_release,
> +};
> +
> +struct wwan_device *wwan_create_dev(struct device *parent)
> +{
> + struct wwan_device *wwandev;
> + int err, id;
> +
> + mutex_lock(&wwan_global_lock);
> +
> + wwandev = __wwan_find_by_parent(parent);
> + if (wwandev) {
> + get_device(&wwandev->dev);
Ah, you lock outside of the function, and increment the reference count,
that's a sure way to cause auditing problems over time. Don't do that,
you know better.
> + wwandev->usage++;
Hah, why? You now have 2 reference counts for the same structure?
> + goto done_unlock;
> + }
> +
> + id = ida_alloc(&wwan_ida, GFP_KERNEL);
Again, I do not think you need a lock if you use this structure in a
safe way.
> + if (id < 0)
> + goto done_unlock;
> +
> + wwandev = kzalloc(sizeof(*wwandev), GFP_KERNEL);
> + if (!wwandev) {
> + ida_free(&wwan_ida, id);
> + goto done_unlock;
> + }
> +
> + wwandev->dev.parent = parent;
> + wwandev->dev.class = wwan_class;
> + wwandev->dev.type = &wwan_type;
> + wwandev->id = id;
> + dev_set_name(&wwandev->dev, "wwan%d", wwandev->id);
> + wwandev->usage = 1;
> + INIT_LIST_HEAD(&wwandev->ports);
> +
> + err = device_register(&wwandev->dev);
> + if (err) {
> + put_device(&wwandev->dev);
> + ida_free(&wwan_ida, id);
> + wwandev = NULL;
> + goto done_unlock;
> + }
> +
> + list_add_tail(&wwandev->list, &wwan_list);
> +
> +done_unlock:
> + mutex_unlock(&wwan_global_lock);
> +
> + return wwandev;
> +}
> +EXPORT_SYMBOL_GPL(wwan_create_dev);
> +
> +void wwan_destroy_dev(struct wwan_device *wwandev)
> +{
> + mutex_lock(&wwan_global_lock);
> + wwandev->usage--;
Nice, 2 references! :(
> +
> + if (wwandev->usage)
> + goto done_unlock;
No, you don't need this.
> +
> + /* Someone destroyed the wwan device without removing ports */
> + WARN_ON(!list_empty(&wwandev->ports));
why?
Did you just reboot a system?
> +
> + list_del(&wwandev->list);
> + device_unregister(&wwandev->dev);
> + ida_free(&wwan_ida, wwandev->id);
> + put_device(&wwandev->dev);
> +
> +done_unlock:
> + mutex_unlock(&wwan_global_lock);
> +}
> +EXPORT_SYMBOL_GPL(wwan_destroy_dev);
> +
> +static int __init wwan_init(void)
> +{
> + int err;
> +
> + wwan_class = class_create(THIS_MODULE, "wwan");
> + if (IS_ERR(wwan_class))
> + return PTR_ERR(wwan_class);
> +
> + err = wwan_port_init();
> + if (err)
> + goto err_class_destroy;
> +
> + return 0;
> +
> +err_class_destroy:
> + class_destroy(wwan_class);
> + return err;
> +}
> +
> +static void __exit wwan_exit(void)
> +{
> + wwan_port_deinit();
> + class_destroy(wwan_class);
> +}
> +
> +//subsys_initcall(wwan_init);
???
Debugging code left around?
> +module_init(wwan_init);
> +module_exit(wwan_exit);
> +
> +MODULE_AUTHOR("Loic Poulain <loic.poulain@xxxxxxxxxx>");
> +MODULE_DESCRIPTION("WWAN core");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/net/wwan/wwan_core.h b/drivers/net/wwan/wwan_core.h
> new file mode 100644
> index 0000000..21d187a
> --- /dev/null
> +++ b/drivers/net/wwan/wwan_core.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright (c) 2021, Linaro Ltd <loic.poulain@xxxxxxxxxx> */
> +
> +#ifndef __WWAN_CORE_H
> +#define __WWAN_CORE_H
> +
> +#include <linux/device.h>
> +#include <linux/wwan.h>
> +
> +#define to_wwan_dev(d) container_of(d, struct wwan_device, dev)
> +
> +struct wwan_device *wwan_create_dev(struct device *parent);
> +void wwan_destroy_dev(struct wwan_device *wwandev);
> +
> +int wwan_port_init(void);
> +void wwan_port_deinit(void);
> +
> +extern struct class *wwan_class;
> +
> +#endif /* WWAN_CORE_H */
> diff --git a/drivers/net/wwan/wwan_port.c b/drivers/net/wwan/wwan_port.c
> new file mode 100644
> index 0000000..b32da8f
> --- /dev/null
> +++ b/drivers/net/wwan/wwan_port.c
> @@ -0,0 +1,136 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (c) 2021, Linaro Ltd <loic.poulain@xxxxxxxxxx> */
> +
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/fs.h>
> +#include <linux/idr.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/wwan.h>
> +
> +#include "wwan_core.h"
> +
> +#define WWAN_MAX_MINORS 32
Why only 32?
> +
> +static int wwan_major;
> +static DEFINE_IDR(wwan_port_idr);
> +static DEFINE_MUTEX(wwan_port_idr_lock);
More idrs?
> +
> +static const char * const wwan_port_type_str[] = {
> + "AT",
> + "MBIM",
> + "QMI",
> + "QCDM",
> + "FIREHOSE"
> +};
> +
> +int wwan_add_port(struct wwan_port *port)
> +{
> + struct wwan_device *wwandev = port->wwandev;
> + struct device *dev;
> + int minor, err;
> +
> + if (port->type >= WWAN_PORT_MAX || !port->fops || !wwandev)
> + return -EINVAL;
> +
> + mutex_lock(&wwan_port_idr_lock);
> + minor = idr_alloc(&wwan_port_idr, port, 0, WWAN_MAX_MINORS, GFP_KERNEL);
> + mutex_unlock(&wwan_port_idr_lock);
Again, I do not think you need a lock. I could be wrong though, you
might want to look into it...
> +
> + if (minor < 0)
> + return minor;
> +
> + mutex_lock(&wwandev->lock);
> +
> + dev = device_create(wwan_class, &wwandev->dev,
> + MKDEV(wwan_major, minor), port,
> + "wwan%dp%u%s", wwandev->id, wwandev->port_idx,
> + wwan_port_type_str[port->type]);
> + if (IS_ERR(dev)) {
> + err = PTR_ERR(dev);
> + mutex_unlock(&wwandev->lock);
> + goto error_free_idr;
> + }
> +
> + port->id = wwandev->port_idx++;
You increment the id after creating it?
> + port->minor = minor;
> +
> + list_add(&port->list, &wwandev->ports);
> +
> + mutex_unlock(&port->wwandev->lock);
> +
> + return 0;
> +
> +error_free_idr:
> + mutex_lock(&wwan_port_idr_lock);
> + idr_remove(&wwan_port_idr, minor);
> + mutex_unlock(&wwan_port_idr_lock);
> +
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(wwan_add_port);
> +
> +void wwan_remove_port(struct wwan_port *port)
> +{
> + struct wwan_device *wwandev = port->wwandev;
> +
> + WARN_ON(!wwandev);
WARN_ON is a huge crutch, never use it in new code.
> +
> + mutex_lock(&wwandev->lock);
> + device_destroy(wwan_class, MKDEV(wwan_major, port->minor));
> + list_del(&port->list);
> + mutex_unlock(&wwandev->lock);
> +
> + mutex_lock(&wwan_port_idr_lock);
> + idr_remove(&wwan_port_idr, port->minor);
> + mutex_unlock(&wwan_port_idr_lock);
> +}
> +EXPORT_SYMBOL_GPL(wwan_remove_port);
> +
> +static int wwan_port_open(struct inode *inode, struct file *file)
> +{
> + const struct file_operations *new_fops;
> + unsigned int minor = iminor(inode);
> + struct wwan_port *port;
> + int err = 0;
> +
> + mutex_lock(&wwan_port_idr_lock);
> + port = idr_find(&wwan_port_idr, minor);
> + if (!port) {
> + mutex_unlock(&wwan_port_idr_lock);
> + return -ENODEV;
> + }
> + mutex_unlock(&wwan_port_idr_lock);
> +
> + file->private_data = port->private_data ? port->private_data : port;
> + stream_open(inode, file);
> +
> + new_fops = fops_get(port->fops);
> + replace_fops(file, new_fops);
Why replace the fops?
> + if (file->f_op->open)
> + err = file->f_op->open(inode, file);
> +
> + return err;
> +}
> +
> +static const struct file_operations wwan_port_fops = {
> + .owner = THIS_MODULE,
> + .open = wwan_port_open,
> + .llseek = noop_llseek,
> +};
> +
> +int wwan_port_init(void)
> +{
> + wwan_major = register_chrdev(0, "wwanport", &wwan_port_fops);
> + if (wwan_major < 0)
> + return wwan_major;
> +
> + return 0;
> +}
> +
> +void wwan_port_deinit(void)
> +{
> + unregister_chrdev(wwan_major, "wwanport");
> + idr_destroy(&wwan_port_idr);
> +}
I'm confused, you have 1 class, but 2 different major numbers for this
class? You have a device and ports with different numbers, how are they
all tied together?
> diff --git a/include/linux/wwan.h b/include/linux/wwan.h
> new file mode 100644
> index 0000000..6caca5c
> --- /dev/null
> +++ b/include/linux/wwan.h
> @@ -0,0 +1,121 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright (c) 2021, Linaro Ltd <loic.poulain@xxxxxxxxxx> */
> +
> +#ifndef __WWAN_H
> +#define __WWAN_H
> +
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +
> +/**
> + * struct wwan_device - The structure that defines a WWAN device
> + *
> + * @id: WWAN device unique ID.
> + * @usage: WWAN device usage counter.
> + * @dev: underlying device.
> + * @list: list to chain WWAN devices.
> + * @ports: list of attached wwan_port.
> + * @port_idx: port index counter.
> + * @lock: mutex protecting members of this structure.
> + */
> +struct wwan_device {
> + int id;
> + unsigned int usage;
Again, not needed.
> +
> + struct device dev;
> + struct list_head list;
You should use the list in the class instead.
> +
> + struct list_head ports;
Are you sure you need this?
> + unsigned int port_idx;
> +
> + struct mutex lock;
> +};
> +
> +/**
> + * enum wwan_port_type - WWAN port types
> + * @WWAN_PORT_AT: AT commands.
> + * @WWAN_PORT_MBIM: Mobile Broadband Interface Model control.
> + * @WWAN_PORT_QMI: Qcom modem/MSM interface for modem control.
> + * @WWAN_PORT_QCDM: Qcom Modem diagnostic interface.
> + * @WWAN_PORT_FIREHOSE: XML based command protocol.
> + * @WWAN_PORT_MAX
> + */
> +enum wwan_port_type {
> + WWAN_PORT_AT,
> + WWAN_PORT_MBIM,
> + WWAN_PORT_QMI,
> + WWAN_PORT_QCDM,
> + WWAN_PORT_FIREHOSE,
> + WWAN_PORT_MAX,
> +};
> +
> +/**
> + * struct wwan_port - The structure that defines a WWAN port
> + *
> + * @wwandev: WWAN device this port belongs to.
> + * @fops: Port file operations.
> + * @private_data: underlying device.
> + * @type: port type.
> + * @id: port allocated ID.
> + * @minor: port allocated minor ID for cdev.
> + * @list: list to chain WWAN ports.
> + */
> +struct wwan_port {
> + struct wwan_device *wwandev;
> + const struct file_operations *fops;
> + void *private_data;
> + enum wwan_port_type type;
> +
> + /* private */
> + unsigned int id;
> + int minor;
> + struct list_head list;
So a port is not a device? Why not?
For such a tiny amount of code, I had a lot of questions :(
greg k-h
