[RFC PATCH 0/6] net: wwan: add NMEA port type support

[RFC PATCH 0/6] net: wwan: add NMEA port type support

[Sergey Ryazanov]

The series introduces a long discussed NMEA port type support for the
WWAN subsystem. There are two goals. From the WWAN driver perspective,
NMEA exported as any other port type (e.g. AT, MBIM, QMI, etc.). From
user space software perspective, the exported chardev belongs to the
GNSS class what makes it easy to distinguish desired port and the WWAN
device common to both NMEA and control (AT, MBIM, etc.) ports makes it
easy to locate a control port for the GNSS receiver activation.

Done by exporting the NMEA port via the GNSS subsystem with the WWAN
core acting as proxy between the WWAN modem driver and the GNSS
subsystem.

The series starts from a cleanup patch. Then two patches prepares the
WWAN core for the proxy style operation. Followed by a patch introding a
new WWNA port type, integration with the GNSS subsystem and demux. The
series ends with a couple of patches that introduce emulated EMEA port
to the WWAN HW simulator.

The series is the product of the discussion with Loic about the pros and
cons of possible models and implementation. Also Muhammad and Slark did
a great job defining the problem, sharing the code and pushing me to
finish the implementation. Many thanks.

Comments are welcomed.

CC: Slark Xiao <slark_xiao@163.com>
CC: Muhammad Nuzaihan <zaihan@unrealasia.net>
CC: Qiang Yu <quic_qianyu@quicinc.com>
CC: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
CC: Johan Hovold <johan@kernel.org>

Sergey Ryazanov (6):
  net: wwan: core: remove unused port_id field
  net: wwan: core: split port creation and registration
  net: wwan: core: split port unregister and stop
  net: wwan: add NMEA port support
  net: wwan: hwsim: refactor to support more port types
  net: wwan: hwsim: support NMEA port emulation

 drivers/net/wwan/Kconfig      |   1 +
 drivers/net/wwan/wwan_core.c  | 260 ++++++++++++++++++++++++++++------
 drivers/net/wwan/wwan_hwsim.c | 201 +++++++++++++++++++++-----
 include/linux/wwan.h          |   2 +
 4 files changed, 389 insertions(+), 75 deletions(-)

-- 
2.45.3

[RFC PATCH 1/6] net: wwan: core: remove unused port_id field

[Sergey Ryazanov]

It was used initially for a port id allocation, then removed, and then
accidently introduced again, but it is still unused. Drop it again to
keep code clean.

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

diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
index 63a47d420bc5..ade8bbffc93e 100644
--- a/drivers/net/wwan/wwan_core.c
+++ b/drivers/net/wwan/wwan_core.c
@@ -43,7 +43,6 @@ static struct dentry *wwan_debugfs_dir;
  *
  * @id: WWAN device unique ID.
  * @dev: Underlying device.
- * @port_id: Current available port ID to pick.
  * @ops: wwan device ops
  * @ops_ctxt: context to pass to ops
  * @debugfs_dir:  WWAN device debugfs dir
@@ -51,7 +50,6 @@ static struct dentry *wwan_debugfs_dir;
 struct wwan_device {
 	unsigned int id;
 	struct device dev;
-	atomic_t port_id;
 	const struct wwan_ops *ops;
 	void *ops_ctxt;
 #ifdef CONFIG_WWAN_DEBUGFS
-- 
2.45.3

[RFC PATCH 2/6] net: wwan: core: split port creation and registration

[Sergey Ryazanov]

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@gmail.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);
+		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

[RFC PATCH 3/6] net: wwan: core: split port unregister and stop

[Sergey Ryazanov]

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: call the port stop operation, purge queues, release the parent
WWAN device, etc. To reuse as much code as possible, split the port
unregistering function into the deregistration of a regular WWAN port
device, and the common port tearing down code.

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

diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
index 045246d7cd50..439a57bc2b9c 100644
--- a/drivers/net/wwan/wwan_core.c
+++ b/drivers/net/wwan/wwan_core.c
@@ -486,6 +486,18 @@ static int wwan_port_register_wwan(struct wwan_port *port)
 	return 0;
 }
 
+/* Unregister regular WWAN port (e.g. AT, MBIM, etc) */
+static void wwan_port_unregister_wwan(struct wwan_port *port)
+{
+	struct wwan_device *wwandev = to_wwan_dev(port->dev.parent);
+
+	dev_set_drvdata(&port->dev, NULL);
+
+	dev_info(&wwandev->dev, "port %s disconnected\n", dev_name(&port->dev));
+
+	device_unregister(&port->dev);
+}
+
 struct wwan_port *wwan_create_port(struct device *parent,
 				   enum wwan_port_type type,
 				   const struct wwan_port_ops *ops,
@@ -542,18 +554,17 @@ void wwan_remove_port(struct wwan_port *port)
 	struct wwan_device *wwandev = to_wwan_dev(port->dev.parent);
 
 	mutex_lock(&port->ops_lock);
-	if (port->start_count)
+	if (port->start_count) {
 		port->ops->stop(port);
+		port->start_count = 0;
+	}
 	port->ops = NULL; /* Prevent any new port operations (e.g. from fops) */
 	mutex_unlock(&port->ops_lock);
 
 	wake_up_interruptible(&port->waitqueue);
-
 	skb_queue_purge(&port->rxq);
-	dev_set_drvdata(&port->dev, NULL);
 
-	dev_info(&wwandev->dev, "port %s disconnected\n", dev_name(&port->dev));
-	device_unregister(&port->dev);
+	wwan_port_unregister_wwan(port);
 
 	/* Release related wwan device */
 	wwan_remove_dev(wwandev);
-- 
2.45.3

[RFC PATCH 4/6] net: wwan: add NMEA port support

[Sergey Ryazanov]

Many WWAN modems come with embedded GNSS receiver inside and have a
dedicated port to output geopositioning data. On the one hand, the
GNSS receiver has little in common with WWAN modem and just shares a
host interface and should be exported using the GNSS subsystem. On the
other hand, GNSS receiver is not automatically activated and needs a
generic WWAN control port (AT, MBIM, etc.) to be turned on. And a user
space software needs extra information to find the control port.

Introduce the new type of WWAN port - NMEA. When driver asks to register
a NMEA port, the core allocates common parent WWAN device as usual, but
exports the NMEA port via the GNSS subsystem and acts as a proxy between
the device driver and the GNSS subsystem.

From the WWAN device driver perspective, a NMEA port is registered as a
regular WWAN port without any difference. And the driver interacts only
with the WWAN core. From the user space perspective, the NMEA port is a
GNSS device which parent can be used to enumerate and select the proper
control port for the GNSS receiver management.

CC: Slark Xiao <slark_xiao@163.com>
CC: Muhammad Nuzaihan <zaihan@unrealasia.net>
CC: Qiang Yu <quic_qianyu@quicinc.com>
CC: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
CC: Johan Hovold <johan@kernel.org>
Suggested-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
 drivers/net/wwan/Kconfig     |   1 +
 drivers/net/wwan/wwan_core.c | 157 ++++++++++++++++++++++++++++++++++-
 include/linux/wwan.h         |   2 +
 3 files changed, 156 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wwan/Kconfig b/drivers/net/wwan/Kconfig
index 410b0245114e..88df55d78d90 100644
--- a/drivers/net/wwan/Kconfig
+++ b/drivers/net/wwan/Kconfig
@@ -7,6 +7,7 @@ menu "Wireless WAN"
 
 config WWAN
 	tristate "WWAN Driver Core"
+	depends on GNSS || GNSS = n
 	help
 	  Say Y here if you want to use the WWAN driver core. This driver
 	  provides a common framework for WWAN drivers.
diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
index 439a57bc2b9c..a30f0c89aa82 100644
--- a/drivers/net/wwan/wwan_core.c
+++ b/drivers/net/wwan/wwan_core.c
@@ -16,6 +16,7 @@
 #include <linux/types.h>
 #include <linux/uaccess.h>
 #include <linux/termios.h>
+#include <linux/gnss.h>
 #include <linux/wwan.h>
 #include <net/rtnetlink.h>
 #include <uapi/linux/wwan.h>
@@ -89,9 +90,16 @@ struct wwan_port {
 			struct ktermios termios;
 			int mdmbits;
 		} at_data;
+		struct gnss_device *gnss;
 	};
 };
 
+static int wwan_port_op_start(struct wwan_port *port);
+static void wwan_port_op_stop(struct wwan_port *port);
+static int wwan_port_op_tx(struct wwan_port *port, struct sk_buff *skb,
+			   bool nonblock);
+static int wwan_wait_tx(struct wwan_port *port, bool nonblock);
+
 static ssize_t index_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct wwan_device *wwan = to_wwan_dev(dev);
@@ -340,6 +348,7 @@ static const struct {
 		.name = "MIPC",
 		.devsuf = "mipc",
 	},
+	/* WWAN_PORT_NMEA is exported via the GNSS subsystem */
 };
 
 static ssize_t type_show(struct device *dev, struct device_attribute *attr,
@@ -498,6 +507,132 @@ static void wwan_port_unregister_wwan(struct wwan_port *port)
 	device_unregister(&port->dev);
 }
 
+#if IS_ENABLED(CONFIG_GNSS)
+static int wwan_gnss_open(struct gnss_device *gdev)
+{
+	return wwan_port_op_start(gnss_get_drvdata(gdev));
+}
+
+static void wwan_gnss_close(struct gnss_device *gdev)
+{
+	wwan_port_op_stop(gnss_get_drvdata(gdev));
+}
+
+static int wwan_gnss_write(struct gnss_device *gdev, const unsigned char *buf,
+			   size_t count)
+{
+	struct wwan_port *port = gnss_get_drvdata(gdev);
+	struct sk_buff *skb, *head = NULL, *tail = NULL;
+	size_t frag_len, remain = count;
+	int ret;
+
+	ret = wwan_wait_tx(port, false);
+	if (ret)
+		return ret;
+
+	do {
+		frag_len = min(remain, port->frag_len);
+		skb = alloc_skb(frag_len + port->headroom_len, GFP_KERNEL);
+		if (!skb) {
+			ret = -ENOMEM;
+			goto freeskb;
+		}
+		skb_reserve(skb, port->headroom_len);
+		memcpy(skb_put(skb, frag_len), buf + count - remain, frag_len);
+
+		if (!head) {
+			head = skb;
+		} else {
+			if (!tail)
+				skb_shinfo(head)->frag_list = skb;
+			else
+				tail->next = skb;
+
+			tail = skb;
+			head->data_len += skb->len;
+			head->len += skb->len;
+			head->truesize += skb->truesize;
+		}
+	} while (remain -= frag_len);
+
+	ret = wwan_port_op_tx(port, head, false);
+	if (!ret)
+		return count;
+
+freeskb:
+	kfree_skb(head);
+	return ret;
+}
+
+static struct gnss_operations wwan_gnss_ops = {
+	.open = wwan_gnss_open,
+	.close = wwan_gnss_close,
+	.write_raw = wwan_gnss_write,
+};
+
+static int wwan_port_register_gnss(struct wwan_port *port)
+{
+	struct wwan_device *wwandev = to_wwan_dev(port->dev.parent);
+	struct gnss_device *gdev;
+	int err;
+
+	gdev = gnss_allocate_device(&wwandev->dev);
+	if (!gdev) {
+		err = -ENOMEM;
+		goto error_destroy_port;
+	}
+
+	/* NB: for now we support only NMEA WWAN port type, so hardcode
+	 * the GNSS port type. If more GNSS WWAN port types will be added,
+	 * then we should dynamically mapt WWAN port type to GNSS type.
+	 */
+	gdev->type = GNSS_TYPE_NMEA;
+	gdev->ops = &wwan_gnss_ops;
+	gnss_set_drvdata(gdev, port);
+
+	port->gnss = gdev;
+
+	err = gnss_register_device(gdev);
+	if (err) {
+		gnss_put_device(gdev);
+		goto error_destroy_port;
+	}
+
+	dev_info(&wwandev->dev, "port %s attached\n", dev_name(&gdev->dev));
+
+	return 0;
+
+error_destroy_port:
+	__wwan_port_destroy(port);
+
+	return err;
+}
+
+static void wwan_port_unregister_gnss(struct wwan_port *port)
+{
+	struct wwan_device *wwandev = to_wwan_dev(port->dev.parent);
+	struct gnss_device *gdev = port->gnss;
+
+	dev_info(&wwandev->dev, "port %s disconnected\n", dev_name(&gdev->dev));
+
+	gnss_deregister_device(gdev);
+	gnss_put_device(gdev);
+
+	__wwan_port_destroy(port);
+}
+#else
+static inline int wwan_port_register_gnss(struct wwan_port *port)
+{
+	__wwan_port_destroy(port);
+	return -EOPNOTSUPP;
+}
+
+static inline void wwan_port_unregister_gnss(struct wwan_port *port)
+{
+	WARN_ON(1);	/* This handler cannot be called */
+}
+#endif
+
 struct wwan_port *wwan_create_port(struct device *parent,
 				   enum wwan_port_type type,
 				   const struct wwan_port_ops *ops,
@@ -536,7 +671,11 @@ struct wwan_port *wwan_create_port(struct device *parent,
 	port->dev.parent = &wwandev->dev;
 	dev_set_drvdata(&port->dev, drvdata);
 
-	err = wwan_port_register_wwan(port);
+	if (port->type == WWAN_PORT_NMEA)
+		err = wwan_port_register_gnss(port);
+	else
+		err = wwan_port_register_wwan(port);
+
 	if (err)
 		goto error_wwandev_remove;
 
@@ -564,7 +703,10 @@ void wwan_remove_port(struct wwan_port *port)
 	wake_up_interruptible(&port->waitqueue);
 	skb_queue_purge(&port->rxq);
 
-	wwan_port_unregister_wwan(port);
+	if (port->type == WWAN_PORT_NMEA)
+		wwan_port_unregister_gnss(port);
+	else
+		wwan_port_unregister_wwan(port);
 
 	/* Release related wwan device */
 	wwan_remove_dev(wwandev);
@@ -573,8 +715,15 @@ EXPORT_SYMBOL_GPL(wwan_remove_port);
 
 void wwan_port_rx(struct wwan_port *port, struct sk_buff *skb)
 {
-	skb_queue_tail(&port->rxq, skb);
-	wake_up_interruptible(&port->waitqueue);
+	if (port->type == WWAN_PORT_NMEA) {
+#if IS_ENABLED(CONFIG_GNSS)
+		gnss_insert_raw(port->gnss, skb->data, skb->len);
+#endif
+		consume_skb(skb);
+	} else {
+		skb_queue_tail(&port->rxq, skb);
+		wake_up_interruptible(&port->waitqueue);
+	}
 }
 EXPORT_SYMBOL_GPL(wwan_port_rx);
 
diff --git a/include/linux/wwan.h b/include/linux/wwan.h
index a4d6cc0c9f68..1e0e2cb53579 100644
--- a/include/linux/wwan.h
+++ b/include/linux/wwan.h
@@ -19,6 +19,7 @@
  * @WWAN_PORT_FASTBOOT: Fastboot protocol control
  * @WWAN_PORT_ADB: ADB protocol control
  * @WWAN_PORT_MIPC: MTK MIPC diagnostic interface
+ * @WWAN_PORT_NMEA: embedded GNSS receiver with NMEA output
  *
  * @WWAN_PORT_MAX: Highest supported port types
  * @WWAN_PORT_UNKNOWN: Special value to indicate an unknown port type
@@ -34,6 +35,7 @@ enum wwan_port_type {
 	WWAN_PORT_FASTBOOT,
 	WWAN_PORT_ADB,
 	WWAN_PORT_MIPC,
+	WWAN_PORT_NMEA,
 
 	/* Add new port types above this line */
 
-- 
2.45.3

[RFC PATCH 5/6] net: wwan: hwsim: refactor to support more port types

[Sergey Ryazanov]

Just introduced WWAN NMEA port type needs a testing option. The WWAN HW
simulator was developed with the AT port type in mind and cannot be
easily extended. Refactor it now to make it capable to support more port
types.

No big functional changes, mostly renaming with a little code
rearrangement.

Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
 drivers/net/wwan/wwan_hwsim.c | 73 ++++++++++++++++++++---------------
 1 file changed, 41 insertions(+), 32 deletions(-)

diff --git a/drivers/net/wwan/wwan_hwsim.c b/drivers/net/wwan/wwan_hwsim.c
index b02befd1b6fb..20277ba88433 100644
--- a/drivers/net/wwan/wwan_hwsim.c
+++ b/drivers/net/wwan/wwan_hwsim.c
@@ -56,12 +56,16 @@ struct wwan_hwsim_port {
 	struct wwan_port *wwan;
 	struct work_struct del_work;
 	struct dentry *debugfs_topdir;
-	enum {			/* AT command parser state */
-		AT_PARSER_WAIT_A,
-		AT_PARSER_WAIT_T,
-		AT_PARSER_WAIT_TERM,
-		AT_PARSER_SKIP_LINE,
-	} pstate;
+	union {
+		struct {
+			enum {	/* AT command parser state */
+				AT_PARSER_WAIT_A,
+				AT_PARSER_WAIT_T,
+				AT_PARSER_WAIT_TERM,
+				AT_PARSER_SKIP_LINE,
+			} pstate;
+		} at_emul;
+	};
 };
 
 static const struct file_operations wwan_hwsim_debugfs_portdestroy_fops;
@@ -101,16 +105,16 @@ static const struct wwan_ops wwan_hwsim_wwan_rtnl_ops = {
 	.setup = wwan_hwsim_netdev_setup,
 };
 
-static int wwan_hwsim_port_start(struct wwan_port *wport)
+static int wwan_hwsim_at_emul_start(struct wwan_port *wport)
 {
 	struct wwan_hwsim_port *port = wwan_port_get_drvdata(wport);
 
-	port->pstate = AT_PARSER_WAIT_A;
+	port->at_emul.pstate = AT_PARSER_WAIT_A;
 
 	return 0;
 }
 
-static void wwan_hwsim_port_stop(struct wwan_port *wport)
+static void wwan_hwsim_at_emul_stop(struct wwan_port *wport)
 {
 }
 
@@ -120,7 +124,7 @@ static void wwan_hwsim_port_stop(struct wwan_port *wport)
  *
  * Be aware that this processor is not fully V.250 compliant.
  */
-static int wwan_hwsim_port_tx(struct wwan_port *wport, struct sk_buff *in)
+static int wwan_hwsim_at_emul_tx(struct wwan_port *wport, struct sk_buff *in)
 {
 	struct wwan_hwsim_port *port = wwan_port_get_drvdata(wport);
 	struct sk_buff *out;
@@ -142,17 +146,17 @@ static int wwan_hwsim_port_tx(struct wwan_port *wport, struct sk_buff *in)
 	for (i = 0, s = 0; i < in->len; ++i) {
 		char c = in->data[i];
 
-		if (port->pstate == AT_PARSER_WAIT_A) {
+		if (port->at_emul.pstate == AT_PARSER_WAIT_A) {
 			if (c == 'A' || c == 'a')
-				port->pstate = AT_PARSER_WAIT_T;
+				port->at_emul.pstate = AT_PARSER_WAIT_T;
 			else if (c != '\n')	/* Ignore formating char */
-				port->pstate = AT_PARSER_SKIP_LINE;
-		} else if (port->pstate == AT_PARSER_WAIT_T) {
+				port->at_emul.pstate = AT_PARSER_SKIP_LINE;
+		} else if (port->at_emul.pstate == AT_PARSER_WAIT_T) {
 			if (c == 'T' || c == 't')
-				port->pstate = AT_PARSER_WAIT_TERM;
+				port->at_emul.pstate = AT_PARSER_WAIT_TERM;
 			else
-				port->pstate = AT_PARSER_SKIP_LINE;
-		} else if (port->pstate == AT_PARSER_WAIT_TERM) {
+				port->at_emul.pstate = AT_PARSER_SKIP_LINE;
+		} else if (port->at_emul.pstate == AT_PARSER_WAIT_TERM) {
 			if (c != '\r')
 				continue;
 			/* Consume the trailing formatting char as well */
@@ -162,11 +166,11 @@ static int wwan_hwsim_port_tx(struct wwan_port *wport, struct sk_buff *in)
 			skb_put_data(out, &in->data[s], n);/* Echo */
 			skb_put_data(out, "\r\nOK\r\n", 6);
 			s = i + 1;
-			port->pstate = AT_PARSER_WAIT_A;
-		} else if (port->pstate == AT_PARSER_SKIP_LINE) {
+			port->at_emul.pstate = AT_PARSER_WAIT_A;
+		} else if (port->at_emul.pstate == AT_PARSER_SKIP_LINE) {
 			if (c != '\r')
 				continue;
-			port->pstate = AT_PARSER_WAIT_A;
+			port->at_emul.pstate = AT_PARSER_WAIT_A;
 		}
 	}
 
@@ -183,18 +187,25 @@ static int wwan_hwsim_port_tx(struct wwan_port *wport, struct sk_buff *in)
 	return 0;
 }
 
-static const struct wwan_port_ops wwan_hwsim_port_ops = {
-	.start = wwan_hwsim_port_start,
-	.stop = wwan_hwsim_port_stop,
-	.tx = wwan_hwsim_port_tx,
+static const struct wwan_port_ops wwan_hwsim_at_emul_port_ops = {
+	.start = wwan_hwsim_at_emul_start,
+	.stop = wwan_hwsim_at_emul_stop,
+	.tx = wwan_hwsim_at_emul_tx,
 };
 
-static struct wwan_hwsim_port *wwan_hwsim_port_new(struct wwan_hwsim_dev *dev)
+static struct wwan_hwsim_port *wwan_hwsim_port_new(struct wwan_hwsim_dev *dev,
+						   enum wwan_port_type type)
 {
+	const struct wwan_port_ops *ops;
 	struct wwan_hwsim_port *port;
 	char name[0x10];
 	int err;
 
+	if (type == WWAN_PORT_AT)
+		ops = &wwan_hwsim_at_emul_port_ops;
+	else
+		return ERR_PTR(-EINVAL);
+
 	port = kzalloc(sizeof(*port), GFP_KERNEL);
 	if (!port)
 		return ERR_PTR(-ENOMEM);
@@ -205,9 +216,7 @@ static struct wwan_hwsim_port *wwan_hwsim_port_new(struct wwan_hwsim_dev *dev)
 	port->id = dev->port_idx++;
 	spin_unlock(&dev->ports_lock);
 
-	port->wwan = wwan_create_port(&dev->dev, WWAN_PORT_AT,
-				      &wwan_hwsim_port_ops,
-				      NULL, port);
+	port->wwan = wwan_create_port(&dev->dev, type, ops, NULL, port);
 	if (IS_ERR(port->wwan)) {
 		err = PTR_ERR(port->wwan);
 		goto err_free_port;
@@ -392,7 +401,7 @@ static ssize_t wwan_hwsim_debugfs_portcreate_write(struct file *file,
 	struct wwan_hwsim_dev *dev = file->private_data;
 	struct wwan_hwsim_port *port;
 
-	port = wwan_hwsim_port_new(dev);
+	port = wwan_hwsim_port_new(dev, WWAN_PORT_AT);
 	if (IS_ERR(port))
 		return PTR_ERR(port);
 
@@ -459,6 +468,8 @@ static int __init wwan_hwsim_init_devs(void)
 	int i, j;
 
 	for (i = 0; i < wwan_hwsim_devsnum; ++i) {
+		struct wwan_hwsim_port *port;
+
 		dev = wwan_hwsim_dev_new();
 		if (IS_ERR(dev))
 			return PTR_ERR(dev);
@@ -471,9 +482,7 @@ static int __init wwan_hwsim_init_devs(void)
 		 * the simulator readiness time.
 		 */
 		for (j = 0; j < 2; ++j) {
-			struct wwan_hwsim_port *port;
-
-			port = wwan_hwsim_port_new(dev);
+			port = wwan_hwsim_port_new(dev, WWAN_PORT_AT);
 			if (IS_ERR(port))
 				return PTR_ERR(port);
 
-- 
2.45.3

[RFC PATCH 6/6] net: wwan: hwsim: support NMEA port emulation

[Sergey Ryazanov]

Support NMEA port emulation for the WWAN core GNSS port testing purpose.
Emulator produces pair of GGA + RMC sentences every second what should
be enough to fool gpsd into believing it is working with a NMEA GNSS
receiver.

If the GNSS system is enabled then one NMEA port will be created
automatically for the simulated WWAN device. Manual NMEA port creation
is not supported at the moment.

Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
 drivers/net/wwan/wwan_hwsim.c | 128 +++++++++++++++++++++++++++++++++-
 1 file changed, 126 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wwan/wwan_hwsim.c b/drivers/net/wwan/wwan_hwsim.c
index 20277ba88433..5b5af2c9a63d 100644
--- a/drivers/net/wwan/wwan_hwsim.c
+++ b/drivers/net/wwan/wwan_hwsim.c
@@ -2,7 +2,7 @@
 /*
  * WWAN device simulator for WWAN framework testing.
  *
- * Copyright (c) 2021, Sergey Ryazanov <ryazanov.s.a@gmail.com>
+ * Copyright (c) 2021, 2025, Sergey Ryazanov <ryazanov.s.a@gmail.com>
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -12,8 +12,10 @@
 #include <linux/slab.h>
 #include <linux/device.h>
 #include <linux/spinlock.h>
+#include <linux/time.h>
 #include <linux/list.h>
 #include <linux/skbuff.h>
+#include <linux/timer.h>
 #include <linux/netdevice.h>
 #include <linux/wwan.h>
 #include <linux/debugfs.h>
@@ -65,6 +67,9 @@ struct wwan_hwsim_port {
 				AT_PARSER_SKIP_LINE,
 			} pstate;
 		} at_emul;
+		struct {
+			struct timer_list timer;
+		} nmea_emul;
 	};
 };
 
@@ -193,6 +198,108 @@ static const struct wwan_port_ops wwan_hwsim_at_emul_port_ops = {
 	.tx = wwan_hwsim_at_emul_tx,
 };
 
+#if IS_ENABLED(CONFIG_GNSS)
+#define NMEA_MAX_LEN		82	/* Max sentence length */
+#define NMEA_TRAIL_LEN		5	/* '*' + Checksum + <CR><LF> */
+#define NMEA_MAX_DATA_LEN	(NMEA_MAX_LEN - NMEA_TRAIL_LEN)
+
+static __printf(2, 3)
+void wwan_hwsim_nmea_skb_push_sentence(struct sk_buff *skb,
+				       const char *fmt, ...)
+{
+	unsigned char *s, *p;
+	va_list ap;
+	u8 cs = 0;
+	int len;
+
+	s = skb_put(skb, NMEA_MAX_LEN + 1);	/* +'\0' */
+	if (!s)
+		return;
+
+	va_start(ap, fmt);
+	len = vsnprintf(s, NMEA_MAX_DATA_LEN + 1, fmt, ap);
+	va_end(ap);
+	if (WARN_ON_ONCE(len > NMEA_MAX_DATA_LEN))/* No space for trailer */
+		return;
+
+	for (p = s + 1; *p != '\0'; ++p)/* Skip leading '$' or '!' */
+		cs ^= *p;
+	p += snprintf(p, 5 + 1, "*%02X\r\n", cs);
+
+	len = (p - s) - (NMEA_MAX_LEN + 1);	/* exp. vs real length diff */
+	skb->tail += len;			/* Adjust tail to real length */
+	skb->len += len;
+}
+
+static void wwan_hwsim_nmea_emul_timer(struct timer_list *t)
+{
+	/* 43.74754722298909 N 11.25759835922875 E in DMM format */
+	static const unsigned int coord[4 * 2] = { 43, 44, 8528, 0,
+						   11, 15, 4559, 0 };
+	struct wwan_hwsim_port *port = from_timer(port, t, nmea_emul.timer);
+	struct sk_buff *skb;
+	struct tm tm;
+
+	time64_to_tm(ktime_get_real_seconds(), 0, &tm);
+
+	mod_timer(&port->nmea_emul.timer, jiffies + HZ);	/* 1 second */
+
+	skb = alloc_skb(NMEA_MAX_LEN * 2, GFP_KERNEL);	/* GGA + RMC */
+	if (!skb)
+		return;
+
+	wwan_hwsim_nmea_skb_push_sentence(skb,
+					  "$GPGGA,%02u%02u%02u.000,%02u%02u.%04u,%c,%03u%02u.%04u,%c,1,7,1.03,176.2,M,55.2,M,,",
+					  tm.tm_hour, tm.tm_min, tm.tm_sec,
+					  coord[0], coord[1], coord[2],
+					  coord[3] ? 'S' : 'N',
+					  coord[4], coord[5], coord[6],
+					  coord[7] ? 'W' : 'E');
+
+	wwan_hwsim_nmea_skb_push_sentence(skb,
+					  "$GPRMC,%02u%02u%02u.000,A,%02u%02u.%04u,%c,%03u%02u.%04u,%c,0.02,31.66,%02u%02u%02u,,,A",
+					  tm.tm_hour, tm.tm_min, tm.tm_sec,
+					  coord[0], coord[1], coord[2],
+					  coord[3] ? 'S' : 'N',
+					  coord[4], coord[5], coord[6],
+					  coord[7] ? 'W' : 'E',
+					  tm.tm_mday, tm.tm_mon + 1,
+					  (unsigned int)tm.tm_year - 100);
+
+	wwan_port_rx(port->wwan, skb);
+}
+
+static int wwan_hwsim_nmea_emul_start(struct wwan_port *wport)
+{
+	struct wwan_hwsim_port *port = wwan_port_get_drvdata(wport);
+
+	timer_setup(&port->nmea_emul.timer, wwan_hwsim_nmea_emul_timer, 0);
+	wwan_hwsim_nmea_emul_timer(&port->nmea_emul.timer);
+
+	return 0;
+}
+
+static void wwan_hwsim_nmea_emul_stop(struct wwan_port *wport)
+{
+	struct wwan_hwsim_port *port = wwan_port_get_drvdata(wport);
+
+	timer_delete_sync(&port->nmea_emul.timer);
+}
+
+static int wwan_hwsim_nmea_emul_tx(struct wwan_port *wport, struct sk_buff *in)
+{
+	consume_skb(in);
+
+	return 0;
+}
+
+static const struct wwan_port_ops wwan_hwsim_nmea_emul_port_ops = {
+	.start = wwan_hwsim_nmea_emul_start,
+	.stop = wwan_hwsim_nmea_emul_stop,
+	.tx = wwan_hwsim_nmea_emul_tx,
+};
+#endif
+
 static struct wwan_hwsim_port *wwan_hwsim_port_new(struct wwan_hwsim_dev *dev,
 						   enum wwan_port_type type)
 {
@@ -203,6 +310,10 @@ static struct wwan_hwsim_port *wwan_hwsim_port_new(struct wwan_hwsim_dev *dev,
 
 	if (type == WWAN_PORT_AT)
 		ops = &wwan_hwsim_at_emul_port_ops;
+#if IS_ENABLED(CONFIG_GNSS)
+	else if (type == WWAN_PORT_NMEA)
+		ops = &wwan_hwsim_nmea_emul_port_ops;
+#endif
 	else
 		return ERR_PTR(-EINVAL);
 
@@ -478,9 +589,10 @@ static int __init wwan_hwsim_init_devs(void)
 		list_add_tail(&dev->list, &wwan_hwsim_devs);
 		spin_unlock(&wwan_hwsim_devs_lock);
 
-		/* Create a couple of ports per each device to accelerate
+		/* Create a few various ports per each device to accelerate
 		 * the simulator readiness time.
 		 */
+
 		for (j = 0; j < 2; ++j) {
 			port = wwan_hwsim_port_new(dev, WWAN_PORT_AT);
 			if (IS_ERR(port))
@@ -490,6 +602,18 @@ static int __init wwan_hwsim_init_devs(void)
 			list_add_tail(&port->list, &dev->ports);
 			spin_unlock(&dev->ports_lock);
 		}
+
+#if IS_ENABLED(CONFIG_GNSS)
+		port = wwan_hwsim_port_new(dev, WWAN_PORT_NMEA);
+		if (IS_ERR(port)) {
+			dev_warn(&dev->dev, "failed to create initial NMEA port: %d\n",
+				 (int)PTR_ERR(port));
+		} else {
+			spin_lock(&dev->ports_lock);
+			list_add_tail(&port->list, &dev->ports);
+			spin_unlock(&dev->ports_lock);
+		}
+#endif
 	}
 
 	return 0;
-- 
2.45.3

Re:[RFC PATCH 4/6] net: wwan: add NMEA port support

[Slark Xiao]

Hi Sergey,
I saw you add WWAN_PORT_NMEA here. And I have a concern, shall we
add it into mhi_wwan_ctrl.c for mapping "NMEA" channel in 
mhi_wwan_ctrl_match_table since previous QDU100 device has added
NMEA channel(So only "NMEA" channel is valid in pci_generic.c, but not
"GNSS" in future).

I am testing your patch now. Will update later.
Thanks

At 2025-04-09 07:31:16, "Sergey Ryazanov" <ryazanov.s.a@gmail.com> wrote:
>Many WWAN modems come with embedded GNSS receiver inside and have a
>dedicated port to output geopositioning data. On the one hand, the
>GNSS receiver has little in common with WWAN modem and just shares a
>host interface and should be exported using the GNSS subsystem. On the
>other hand, GNSS receiver is not automatically activated and needs a
>generic WWAN control port (AT, MBIM, etc.) to be turned on. And a user
>space software needs extra information to find the control port.
>
>Introduce the new type of WWAN port - NMEA. When driver asks to register
>a NMEA port, the core allocates common parent WWAN device as usual, but
>exports the NMEA port via the GNSS subsystem and acts as a proxy between
>the device driver and the GNSS subsystem.
>
>From the WWAN device driver perspective, a NMEA port is registered as a
>regular WWAN port without any difference. And the driver interacts only
>with the WWAN core. From the user space perspective, the NMEA port is a
>GNSS device which parent can be used to enumerate and select the proper
>control port for the GNSS receiver management.
>
>CC: Slark Xiao <slark_xiao@163.com>
>CC: Muhammad Nuzaihan <zaihan@unrealasia.net>
>CC: Qiang Yu <quic_qianyu@quicinc.com>
>CC: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>CC: Johan Hovold <johan@kernel.org>
>Suggested-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
>Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
>---
> drivers/net/wwan/Kconfig     |   1 +
> drivers/net/wwan/wwan_core.c | 157 ++++++++++++++++++++++++++++++++++-
> include/linux/wwan.h         |   2 +
> 3 files changed, 156 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/net/wwan/Kconfig b/drivers/net/wwan/Kconfig
>index 410b0245114e..88df55d78d90 100644
>--- a/drivers/net/wwan/Kconfig
>+++ b/drivers/net/wwan/Kconfig
>@@ -7,6 +7,7 @@ menu "Wireless WAN"
> 
> config WWAN
> 	tristate "WWAN Driver Core"
>+	depends on GNSS || GNSS = n
> 	help
> 	  Say Y here if you want to use the WWAN driver core. This driver
> 	  provides a common framework for WWAN drivers.
>diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
>index 439a57bc2b9c..a30f0c89aa82 100644
>--- a/drivers/net/wwan/wwan_core.c
>+++ b/drivers/net/wwan/wwan_core.c
>@@ -16,6 +16,7 @@
> #include <linux/types.h>
> #include <linux/uaccess.h>
> #include <linux/termios.h>
>+#include <linux/gnss.h>
> #include <linux/wwan.h>
> #include <net/rtnetlink.h>
> #include <uapi/linux/wwan.h>
>@@ -89,9 +90,16 @@ struct wwan_port {
> 			struct ktermios termios;
> 			int mdmbits;
> 		} at_data;
>+		struct gnss_device *gnss;
> 	};
> };
> 
>+static int wwan_port_op_start(struct wwan_port *port);
>+static void wwan_port_op_stop(struct wwan_port *port);
>+static int wwan_port_op_tx(struct wwan_port *port, struct sk_buff *skb,
>+			   bool nonblock);
>+static int wwan_wait_tx(struct wwan_port *port, bool nonblock);
>+
> static ssize_t index_show(struct device *dev, struct device_attribute *attr, char *buf)
> {
> 	struct wwan_device *wwan = to_wwan_dev(dev);
>@@ -340,6 +348,7 @@ static const struct {
> 		.name = "MIPC",
> 		.devsuf = "mipc",
> 	},
>+	/* WWAN_PORT_NMEA is exported via the GNSS subsystem */
> };
> 
> static ssize_t type_show(struct device *dev, struct device_attribute *attr,
>@@ -498,6 +507,132 @@ static void wwan_port_unregister_wwan(struct wwan_port *port)
> 	device_unregister(&port->dev);
> }
> 
>+#if IS_ENABLED(CONFIG_GNSS)
>+static int wwan_gnss_open(struct gnss_device *gdev)
>+{
>+	return wwan_port_op_start(gnss_get_drvdata(gdev));
>+}
>+
>+static void wwan_gnss_close(struct gnss_device *gdev)
>+{
>+	wwan_port_op_stop(gnss_get_drvdata(gdev));
>+}
>+
>+static int wwan_gnss_write(struct gnss_device *gdev, const unsigned char *buf,
>+			   size_t count)
>+{
>+	struct wwan_port *port = gnss_get_drvdata(gdev);
>+	struct sk_buff *skb, *head = NULL, *tail = NULL;
>+	size_t frag_len, remain = count;
>+	int ret;
>+
>+	ret = wwan_wait_tx(port, false);
>+	if (ret)
>+		return ret;
>+
>+	do {
>+		frag_len = min(remain, port->frag_len);
>+		skb = alloc_skb(frag_len + port->headroom_len, GFP_KERNEL);
>+		if (!skb) {
>+			ret = -ENOMEM;
>+			goto freeskb;
>+		}
>+		skb_reserve(skb, port->headroom_len);
>+		memcpy(skb_put(skb, frag_len), buf + count - remain, frag_len);
>+
>+		if (!head) {
>+			head = skb;
>+		} else {
>+			if (!tail)
>+				skb_shinfo(head)->frag_list = skb;
>+			else
>+				tail->next = skb;
>+
>+			tail = skb;
>+			head->data_len += skb->len;
>+			head->len += skb->len;
>+			head->truesize += skb->truesize;
>+		}
>+	} while (remain -= frag_len);
>+
>+	ret = wwan_port_op_tx(port, head, false);
>+	if (!ret)
>+		return count;
>+
>+freeskb:
>+	kfree_skb(head);
>+	return ret;
>+}
>+
>+static struct gnss_operations wwan_gnss_ops = {
>+	.open = wwan_gnss_open,
>+	.close = wwan_gnss_close,
>+	.write_raw = wwan_gnss_write,
>+};
>+
>+static int wwan_port_register_gnss(struct wwan_port *port)
>+{
>+	struct wwan_device *wwandev = to_wwan_dev(port->dev.parent);
>+	struct gnss_device *gdev;
>+	int err;
>+
>+	gdev = gnss_allocate_device(&wwandev->dev);
>+	if (!gdev) {
>+		err = -ENOMEM;
>+		goto error_destroy_port;
>+	}
>+
>+	/* NB: for now we support only NMEA WWAN port type, so hardcode
>+	 * the GNSS port type. If more GNSS WWAN port types will be added,
>+	 * then we should dynamically mapt WWAN port type to GNSS type.
>+	 */
>+	gdev->type = GNSS_TYPE_NMEA;
>+	gdev->ops = &wwan_gnss_ops;
>+	gnss_set_drvdata(gdev, port);
>+
>+	port->gnss = gdev;
>+
>+	err = gnss_register_device(gdev);
>+	if (err) {
>+		gnss_put_device(gdev);
>+		goto error_destroy_port;
>+	}
>+
>+	dev_info(&wwandev->dev, "port %s attached\n", dev_name(&gdev->dev));
>+
>+	return 0;
>+
>+error_destroy_port:
>+	__wwan_port_destroy(port);
>+
>+	return err;
>+}
>+
>+static void wwan_port_unregister_gnss(struct wwan_port *port)
>+{
>+	struct wwan_device *wwandev = to_wwan_dev(port->dev.parent);
>+	struct gnss_device *gdev = port->gnss;
>+
>+	dev_info(&wwandev->dev, "port %s disconnected\n", dev_name(&gdev->dev));
>+
>+	gnss_deregister_device(gdev);
>+	gnss_put_device(gdev);
>+
>+	__wwan_port_destroy(port);
>+}
>+#else
>+static inline int wwan_port_register_gnss(struct wwan_port *port)
>+{
>+	__wwan_port_destroy(port);
>+	return -EOPNOTSUPP;
>+}
>+
>+static inline void wwan_port_unregister_gnss(struct wwan_port *port)
>+{
>+	WARN_ON(1);	/* This handler cannot be called */
>+}
>+#endif
>+
> struct wwan_port *wwan_create_port(struct device *parent,
> 				   enum wwan_port_type type,
> 				   const struct wwan_port_ops *ops,
>@@ -536,7 +671,11 @@ struct wwan_port *wwan_create_port(struct device *parent,
> 	port->dev.parent = &wwandev->dev;
> 	dev_set_drvdata(&port->dev, drvdata);
> 
>-	err = wwan_port_register_wwan(port);
>+	if (port->type == WWAN_PORT_NMEA)
>+		err = wwan_port_register_gnss(port);
>+	else
>+		err = wwan_port_register_wwan(port);
>+
> 	if (err)
> 		goto error_wwandev_remove;
> 
>@@ -564,7 +703,10 @@ void wwan_remove_port(struct wwan_port *port)
> 	wake_up_interruptible(&port->waitqueue);
> 	skb_queue_purge(&port->rxq);
> 
>-	wwan_port_unregister_wwan(port);
>+	if (port->type == WWAN_PORT_NMEA)
>+		wwan_port_unregister_gnss(port);
>+	else
>+		wwan_port_unregister_wwan(port);
> 
> 	/* Release related wwan device */
> 	wwan_remove_dev(wwandev);
>@@ -573,8 +715,15 @@ EXPORT_SYMBOL_GPL(wwan_remove_port);
> 
> void wwan_port_rx(struct wwan_port *port, struct sk_buff *skb)
> {
>-	skb_queue_tail(&port->rxq, skb);
>-	wake_up_interruptible(&port->waitqueue);
>+	if (port->type == WWAN_PORT_NMEA) {
>+#if IS_ENABLED(CONFIG_GNSS)
>+		gnss_insert_raw(port->gnss, skb->data, skb->len);
>+#endif
>+		consume_skb(skb);
>+	} else {
>+		skb_queue_tail(&port->rxq, skb);
>+		wake_up_interruptible(&port->waitqueue);
>+	}
> }
> EXPORT_SYMBOL_GPL(wwan_port_rx);
> 
>diff --git a/include/linux/wwan.h b/include/linux/wwan.h
>index a4d6cc0c9f68..1e0e2cb53579 100644
>--- a/include/linux/wwan.h
>+++ b/include/linux/wwan.h
>@@ -19,6 +19,7 @@
>  * @WWAN_PORT_FASTBOOT: Fastboot protocol control
>  * @WWAN_PORT_ADB: ADB protocol control
>  * @WWAN_PORT_MIPC: MTK MIPC diagnostic interface
>+ * @WWAN_PORT_NMEA: embedded GNSS receiver with NMEA output
>  *
>  * @WWAN_PORT_MAX: Highest supported port types
>  * @WWAN_PORT_UNKNOWN: Special value to indicate an unknown port type
>@@ -34,6 +35,7 @@ enum wwan_port_type {
> 	WWAN_PORT_FASTBOOT,
> 	WWAN_PORT_ADB,
> 	WWAN_PORT_MIPC,
>+	WWAN_PORT_NMEA,
> 
> 	/* Add new port types above this line */
> 
>-- 
>2.45.3

Re:Re:[RFC PATCH 4/6] net: wwan: add NMEA port support

[Slark Xiao]

Hi Sergey,
Device port /dev/gnss0 is enumerated . Does it be expected?
I can get the NMEA data from this port by cat or minicom command.
But the gpsd.service also can not be initialized normally. It reports:

TriggeredBy: ● gpsd.socket
    Process: 3824 ExecStartPre=/bin/stty speed 115200 -F $DEVICES (code=exited, status=1/FAILURE)
        CPU: 7ms

4月 09 16:04:16 jbd systemd[1]: Starting GPS (Global Positioning System) Daemon...
4月 09 16:04:17 jbd stty[3824]: /bin/stty: /dev/gnss0: Inappropriate ioctl for device
4月 09 16:04:17 jbd systemd[1]: gpsd.service: Control process exited, code=exited, status=1/FAILURE
4月 09 16:04:17 jbd systemd[1]: gpsd.service: Failed with result 'exit-code'.
4月 09 16:04:17 jbd systemd[1]: Failed to start GPS (Global Positioning System) Daemon.

Seems it's not a serial port.
Any advice?

Thanks

At 2025-04-09 11:54:50, "Slark Xiao" <slark_xiao@163.com> wrote:
>
>Hi Sergey,
>I saw you add WWAN_PORT_NMEA here. And I have a concern, shall we
>add it into mhi_wwan_ctrl.c for mapping "NMEA" channel in 
>mhi_wwan_ctrl_match_table since previous QDU100 device has added
>NMEA channel(So only "NMEA" channel is valid in pci_generic.c, but not
>"GNSS" in future).
>
>I am testing your patch now. Will update later.
>Thanks
>
>At 2025-04-09 07:31:16, "Sergey Ryazanov" <ryazanov.s.a@gmail.com> wrote:
>>Many WWAN modems come with embedded GNSS receiver inside and have a
>>dedicated port to output geopositioning data. On the one hand, the
>>GNSS receiver has little in common with WWAN modem and just shares a
>>host interface and should be exported using the GNSS subsystem. On the
>>other hand, GNSS receiver is not automatically activated and needs a
>>generic WWAN control port (AT, MBIM, etc.) to be turned on. And a user
>>space software needs extra information to find the control port.
>>
>>Introduce the new type of WWAN port - NMEA. When driver asks to register
>>a NMEA port, the core allocates common parent WWAN device as usual, but
>>exports the NMEA port via the GNSS subsystem and acts as a proxy between
>>the device driver and the GNSS subsystem.
>>
>>From the WWAN device driver perspective, a NMEA port is registered as a
>>regular WWAN port without any difference. And the driver interacts only
>>with the WWAN core. From the user space perspective, the NMEA port is a
>>GNSS device which parent can be used to enumerate and select the proper
>>control port for the GNSS receiver management.
>>
>>CC: Slark Xiao <slark_xiao@163.com>
>>CC: Muhammad Nuzaihan <zaihan@unrealasia.net>
>>CC: Qiang Yu <quic_qianyu@quicinc.com>
>>CC: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>CC: Johan Hovold <johan@kernel.org>
>>Suggested-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
>>Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
>>---
>> drivers/net/wwan/Kconfig     |   1 +
>> drivers/net/wwan/wwan_core.c | 157 ++++++++++++++++++++++++++++++++++-
>> include/linux/wwan.h         |   2 +
>> 3 files changed, 156 insertions(+), 4 deletions(-)
>>
>>diff --git a/drivers/net/wwan/Kconfig b/drivers/net/wwan/Kconfig
>>index 410b0245114e..88df55d78d90 100644
>>--- a/drivers/net/wwan/Kconfig
>>+++ b/drivers/net/wwan/Kconfig
>>@@ -7,6 +7,7 @@ menu "Wireless WAN"
>> 
>> config WWAN
>> 	tristate "WWAN Driver Core"
>>+	depends on GNSS || GNSS = n
>> 	help
>> 	  Say Y here if you want to use the WWAN driver core. This driver
>> 	  provides a common framework for WWAN drivers.
>>diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
>>index 439a57bc2b9c..a30f0c89aa82 100644
>>--- a/drivers/net/wwan/wwan_core.c
>>+++ b/drivers/net/wwan/wwan_core.c
>>@@ -16,6 +16,7 @@
>> #include <linux/types.h>
>> #include <linux/uaccess.h>
>> #include <linux/termios.h>
>>+#include <linux/gnss.h>
>> #include <linux/wwan.h>
>> #include <net/rtnetlink.h>
>> #include <uapi/linux/wwan.h>
>>@@ -89,9 +90,16 @@ struct wwan_port {
>> 			struct ktermios termios;
>> 			int mdmbits;
>> 		} at_data;
>>+		struct gnss_device *gnss;
>> 	};
>> };
>> 
>>+static int wwan_port_op_start(struct wwan_port *port);
>>+static void wwan_port_op_stop(struct wwan_port *port);
>>+static int wwan_port_op_tx(struct wwan_port *port, struct sk_buff *skb,
>>+			   bool nonblock);
>>+static int wwan_wait_tx(struct wwan_port *port, bool nonblock);
>>+
>> static ssize_t index_show(struct device *dev, struct device_attribute *attr, char *buf)
>> {
>> 	struct wwan_device *wwan = to_wwan_dev(dev);
>>@@ -340,6 +348,7 @@ static const struct {
>> 		.name = "MIPC",
>> 		.devsuf = "mipc",
>> 	},
>>+	/* WWAN_PORT_NMEA is exported via the GNSS subsystem */
>> };
>> 
>> static ssize_t type_show(struct device *dev, struct device_attribute *attr,
>>@@ -498,6 +507,132 @@ static void wwan_port_unregister_wwan(struct wwan_port *port)
>> 	device_unregister(&port->dev);
>> }
>> 
>>+#if IS_ENABLED(CONFIG_GNSS)
>>+static int wwan_gnss_open(struct gnss_device *gdev)
>>+{
>>+	return wwan_port_op_start(gnss_get_drvdata(gdev));
>>+}
>>+
>>+static void wwan_gnss_close(struct gnss_device *gdev)
>>+{
>>+	wwan_port_op_stop(gnss_get_drvdata(gdev));
>>+}
>>+
>>+static int wwan_gnss_write(struct gnss_device *gdev, const unsigned char *buf,
>>+			   size_t count)
>>+{
>>+	struct wwan_port *port = gnss_get_drvdata(gdev);
>>+	struct sk_buff *skb, *head = NULL, *tail = NULL;
>>+	size_t frag_len, remain = count;
>>+	int ret;
>>+
>>+	ret = wwan_wait_tx(port, false);
>>+	if (ret)
>>+		return ret;
>>+
>>+	do {
>>+		frag_len = min(remain, port->frag_len);
>>+		skb = alloc_skb(frag_len + port->headroom_len, GFP_KERNEL);
>>+		if (!skb) {
>>+			ret = -ENOMEM;
>>+			goto freeskb;
>>+		}
>>+		skb_reserve(skb, port->headroom_len);
>>+		memcpy(skb_put(skb, frag_len), buf + count - remain, frag_len);
>>+
>>+		if (!head) {
>>+			head = skb;
>>+		} else {
>>+			if (!tail)
>>+				skb_shinfo(head)->frag_list = skb;
>>+			else
>>+				tail->next = skb;
>>+
>>+			tail = skb;
>>+			head->data_len += skb->len;
>>+			head->len += skb->len;
>>+			head->truesize += skb->truesize;
>>+		}
>>+	} while (remain -= frag_len);
>>+
>>+	ret = wwan_port_op_tx(port, head, false);
>>+	if (!ret)
>>+		return count;
>>+
>>+freeskb:
>>+	kfree_skb(head);
>>+	return ret;
>>+}
>>+
>>+static struct gnss_operations wwan_gnss_ops = {
>>+	.open = wwan_gnss_open,
>>+	.close = wwan_gnss_close,
>>+	.write_raw = wwan_gnss_write,
>>+};
>>+
>>+static int wwan_port_register_gnss(struct wwan_port *port)
>>+{
>>+	struct wwan_device *wwandev = to_wwan_dev(port->dev.parent);
>>+	struct gnss_device *gdev;
>>+	int err;
>>+
>>+	gdev = gnss_allocate_device(&wwandev->dev);
>>+	if (!gdev) {
>>+		err = -ENOMEM;
>>+		goto error_destroy_port;
>>+	}
>>+
>>+	/* NB: for now we support only NMEA WWAN port type, so hardcode
>>+	 * the GNSS port type. If more GNSS WWAN port types will be added,
>>+	 * then we should dynamically mapt WWAN port type to GNSS type.
>>+	 */
>>+	gdev->type = GNSS_TYPE_NMEA;
>>+	gdev->ops = &wwan_gnss_ops;
>>+	gnss_set_drvdata(gdev, port);
>>+
>>+	port->gnss = gdev;
>>+
>>+	err = gnss_register_device(gdev);
>>+	if (err) {
>>+		gnss_put_device(gdev);
>>+		goto error_destroy_port;
>>+	}
>>+
>>+	dev_info(&wwandev->dev, "port %s attached\n", dev_name(&gdev->dev));
>>+
>>+	return 0;
>>+
>>+error_destroy_port:
>>+	__wwan_port_destroy(port);
>>+
>>+	return err;
>>+}
>>+
>>+static void wwan_port_unregister_gnss(struct wwan_port *port)
>>+{
>>+	struct wwan_device *wwandev = to_wwan_dev(port->dev.parent);
>>+	struct gnss_device *gdev = port->gnss;
>>+
>>+	dev_info(&wwandev->dev, "port %s disconnected\n", dev_name(&gdev->dev));
>>+
>>+	gnss_deregister_device(gdev);
>>+	gnss_put_device(gdev);
>>+
>>+	__wwan_port_destroy(port);
>>+}
>>+#else
>>+static inline int wwan_port_register_gnss(struct wwan_port *port)
>>+{
>>+	__wwan_port_destroy(port);
>>+	return -EOPNOTSUPP;
>>+}
>>+
>>+static inline void wwan_port_unregister_gnss(struct wwan_port *port)
>>+{
>>+	WARN_ON(1);	/* This handler cannot be called */
>>+}
>>+#endif
>>+
>> struct wwan_port *wwan_create_port(struct device *parent,
>> 				   enum wwan_port_type type,
>> 				   const struct wwan_port_ops *ops,
>>@@ -536,7 +671,11 @@ struct wwan_port *wwan_create_port(struct device *parent,
>> 	port->dev.parent = &wwandev->dev;
>> 	dev_set_drvdata(&port->dev, drvdata);
>> 
>>-	err = wwan_port_register_wwan(port);
>>+	if (port->type == WWAN_PORT_NMEA)
>>+		err = wwan_port_register_gnss(port);
>>+	else
>>+		err = wwan_port_register_wwan(port);
>>+
>> 	if (err)
>> 		goto error_wwandev_remove;
>> 
>>@@ -564,7 +703,10 @@ void wwan_remove_port(struct wwan_port *port)
>> 	wake_up_interruptible(&port->waitqueue);
>> 	skb_queue_purge(&port->rxq);
>> 
>>-	wwan_port_unregister_wwan(port);
>>+	if (port->type == WWAN_PORT_NMEA)
>>+		wwan_port_unregister_gnss(port);
>>+	else
>>+		wwan_port_unregister_wwan(port);
>> 
>> 	/* Release related wwan device */
>> 	wwan_remove_dev(wwandev);
>>@@ -573,8 +715,15 @@ EXPORT_SYMBOL_GPL(wwan_remove_port);
>> 
>> void wwan_port_rx(struct wwan_port *port, struct sk_buff *skb)
>> {
>>-	skb_queue_tail(&port->rxq, skb);
>>-	wake_up_interruptible(&port->waitqueue);
>>+	if (port->type == WWAN_PORT_NMEA) {
>>+#if IS_ENABLED(CONFIG_GNSS)
>>+		gnss_insert_raw(port->gnss, skb->data, skb->len);
>>+#endif
>>+		consume_skb(skb);
>>+	} else {
>>+		skb_queue_tail(&port->rxq, skb);
>>+		wake_up_interruptible(&port->waitqueue);
>>+	}
>> }
>> EXPORT_SYMBOL_GPL(wwan_port_rx);
>> 
>>diff --git a/include/linux/wwan.h b/include/linux/wwan.h
>>index a4d6cc0c9f68..1e0e2cb53579 100644
>>--- a/include/linux/wwan.h
>>+++ b/include/linux/wwan.h
>>@@ -19,6 +19,7 @@
>>  * @WWAN_PORT_FASTBOOT: Fastboot protocol control
>>  * @WWAN_PORT_ADB: ADB protocol control
>>  * @WWAN_PORT_MIPC: MTK MIPC diagnostic interface
>>+ * @WWAN_PORT_NMEA: embedded GNSS receiver with NMEA output
>>  *
>>  * @WWAN_PORT_MAX: Highest supported port types
>>  * @WWAN_PORT_UNKNOWN: Special value to indicate an unknown port type
>>@@ -34,6 +35,7 @@ enum wwan_port_type {
>> 	WWAN_PORT_FASTBOOT,
>> 	WWAN_PORT_ADB,
>> 	WWAN_PORT_MIPC,
>>+	WWAN_PORT_NMEA,
>> 
>> 	/* Add new port types above this line */
>> 
>>-- 
>>2.45.3

Re:Re:[RFC PATCH 4/6] net: wwan: add NMEA port support

[Sergey Ryazanov]

On April 9, 2025 11:30:58 AM GMT+03:00, Slark Xiao <slark_xiao@163.com> wrote:
>
>Hi Sergey,
>Device port /dev/gnss0 is enumerated . Does it be expected?
>I can get the NMEA data from this port by cat or minicom command.
>But the gpsd.service also can not be initialized normally. It reports:
>
>TriggeredBy: ● gpsd.socket
>    Process: 3824 ExecStartPre=/bin/stty speed 115200 -F $DEVICES (code=exited, status=1/FAILURE)
>        CPU: 7ms
>
>4月 09 16:04:16 jbd systemd[1]: Starting GPS (Global Positioning System) Daemon...
>4月 09 16:04:17 jbd stty[3824]: /bin/stty: /dev/gnss0: Inappropriate ioctl for device
>4月 09 16:04:17 jbd systemd[1]: gpsd.service: Control process exited, code=exited, status=1/FAILURE
>4月 09 16:04:17 jbd systemd[1]: gpsd.service: Failed with result 'exit-code'.
>4月 09 16:04:17 jbd systemd[1]: Failed to start GPS (Global Positioning System) Daemon.
>
>Seems it's not a serial port.

It is a char dev lacking some IOCTLs support. Yeah.

>Any advice?

Yep. Remove that stty invocation from the service definition. For me, gpsd works flawlessly. You can try to start it manually from a terminal.

--
Sergey
Hi Slark,

Re: [RFC PATCH 4/6] net: wwan: add NMEA port support

[Sergey Ryazanov]

Hi Slark,

On 09.04.2025 06:54, Slark Xiao wrote:
> Hi Sergey,
> I saw you add WWAN_PORT_NMEA here. And I have a concern, shall we
> add it into mhi_wwan_ctrl.c for mapping "NMEA" channel in
> mhi_wwan_ctrl_match_table since previous QDU100 device has added
> NMEA channel(So only "NMEA" channel is valid in pci_generic.c, but not
> "GNSS" in future).

This patch introduces NMEA port support to the WWAN core which is 
generic entity. Introduced enum item needs only for a driver to interact 
with the WWAN core. How the driver implements support is up to its 
author. If it needs a new definition for MHI implementation, then feel 
free to do it.

--
Sergey

Re: [RFC PATCH 1/6] net: wwan: core: remove unused port_id field

[Loic Poulain]

On Wed, Apr 9, 2025 at 1:31 AM Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>
> It was used initially for a port id allocation, then removed, and then
> accidently introduced again, but it is still unused. Drop it again to
> keep code clean.
>
> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>

Reviewed-by: Loic Poulain <loic.poulain@oss.qualcomm.com>

Re: [RFC PATCH 2/6] net: wwan: core: split port creation and registration

[Loic Poulain]

Hi Sergey,

On Wed, Apr 9, 2025 at 1:31 AM Sergey Ryazanov <ryazanov.s.a@gmail.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@gmail.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
>

Re: [RFC PATCH 3/6] net: wwan: core: split port unregister and stop

[Loic Poulain]

On Wed, Apr 9, 2025 at 1:31 AM Sergey Ryazanov <ryazanov.s.a@gmail.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: call the port stop operation, purge queues, release the parent
> WWAN device, etc. To reuse as much code as possible, split the port
> unregistering function into the deregistration of a regular WWAN port
> device, and the common port tearing down code.
>
> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
> ---
>  drivers/net/wwan/wwan_core.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
> index 045246d7cd50..439a57bc2b9c 100644
> --- a/drivers/net/wwan/wwan_core.c
> +++ b/drivers/net/wwan/wwan_core.c
> @@ -486,6 +486,18 @@ static int wwan_port_register_wwan(struct wwan_port *port)
>         return 0;
>  }
>
> +/* Unregister regular WWAN port (e.g. AT, MBIM, etc) */
> +static void wwan_port_unregister_wwan(struct wwan_port *port)

Wouldn't it be simpler to name it  `wwan_port_unregister` ?

> +{
> +       struct wwan_device *wwandev = to_wwan_dev(port->dev.parent);
> +
> +       dev_set_drvdata(&port->dev, NULL);
> +
> +       dev_info(&wwandev->dev, "port %s disconnected\n", dev_name(&port->dev));
> +
> +       device_unregister(&port->dev);
> +}
> +
>  struct wwan_port *wwan_create_port(struct device *parent,
>                                    enum wwan_port_type type,
>                                    const struct wwan_port_ops *ops,
> @@ -542,18 +554,17 @@ void wwan_remove_port(struct wwan_port *port)
>         struct wwan_device *wwandev = to_wwan_dev(port->dev.parent);
>
>         mutex_lock(&port->ops_lock);
> -       if (port->start_count)
> +       if (port->start_count) {
>                 port->ops->stop(port);
> +               port->start_count = 0;
> +       }
>         port->ops = NULL; /* Prevent any new port operations (e.g. from fops) */
>         mutex_unlock(&port->ops_lock);
>
>         wake_up_interruptible(&port->waitqueue);
> -
>         skb_queue_purge(&port->rxq);
> -       dev_set_drvdata(&port->dev, NULL);
>
> -       dev_info(&wwandev->dev, "port %s disconnected\n", dev_name(&port->dev));
> -       device_unregister(&port->dev);
> +       wwan_port_unregister_wwan(port);
>
>         /* Release related wwan device */
>         wwan_remove_dev(wwandev);
> --
> 2.45.3
>

Re: [RFC PATCH 5/6] net: wwan: hwsim: refactor to support more port types

[Loic Poulain]

On Wed, Apr 9, 2025 at 1:31 AM Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>
> Just introduced WWAN NMEA port type needs a testing option. The WWAN HW
> simulator was developed with the AT port type in mind and cannot be
> easily extended. Refactor it now to make it capable to support more port
> types.
>
> No big functional changes, mostly renaming with a little code
> rearrangement.
>
> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>

Reviewed-by: Loic Poulain <loic.poulain@oss.qualcomm.com>

Re: [RFC PATCH 2/6] net: wwan: core: split port creation and registration

[Sergey Ryazanov]

Hi Loic,

thank you that you found a time to check it. See the explanation below, 
might be you can suggest a better solution.

On 14.04.2025 21:50, Loic Poulain wrote:
> Hi Sergey,
> 
> On Wed, Apr 9, 2025 at 1:31 AM Sergey Ryazanov <ryazanov.s.a@gmail.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@gmail.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?

I agree that this looks weird and asymmetrical. I left the port 
allocation in wwan_create_port() because both WWAN-exported and 
GNSS-exported types of port share the same port allocation. And the port 
struct is used as a container to keep all the port registration arguments.

I did the port freeing inside this function because we free the port 
differently depending of the device registration state. If we fail to 
initialize the port at earlier stage then we use __wwan_port_destroy() 
which basically just releases the memory.

But if device_register() fails then we are required to use put_device() 
which does more job.

I do not think it is acceptable to skip put_device() call and just 
release the memory. Also I do not find maintainable to partially open 
code put_device() here in the WWAN-exportable handler and release the 
memory in caller function wwan_create_port().

We could somehow try to return this information from 
wwan_port_register_wwan() to wwan_create_port(), so the caller could 
decide, shall it use __wwan_port_destroy() or put_device() in case of 
failure.

But I can not see a way to clearly indicate, which releasing approach 
should be used by the caller. And even in this case it going to look 
weird since the called function controls the caller.

Another solution for the asymmetry problem is to move the allocation 
from the caller to the called function. So the memory will be allocated 
and released in the same function. But in this case we will need to pass 
all the parameters from wwan_create_port() to wwan_port_register_wwan(). 
Even if we consolidate the port basic allocation/initialization in a 
common routine, the final solution going to look a duplication. E.g.

struct wwan_port *wwan_port_allocate(struct wwan_device *wwandev,
                                      enum wwan_port_type type,
                                      const struct wwan_port_ops *ops,
                                      struct wwan_port_caps *caps,
                                      void *drvdata)
{
     /* Do the mem allocation and init here */
     return port;
}

struct wwan_port *wwan_port_register_wwan(struct wwan_device *wwandev,
                        enum wwan_port_type type,
                        const struct wwan_port_ops *ops,
                        struct wwan_port_caps *caps,
                        void *drvdata)
{
     port = wwan_port_allocate(wwandev, type, ops, caps, drvdata);
     /* Proceed with chardev registration or release on failure */
     /* return port; or return ERR_PTR(-err); */
}

struct wwan_port *wwan_port_register_gnss(struct wwan_device *wwandev,
                        enum wwan_port_type type,
                        const struct wwan_port_ops *ops,
                        struct wwan_port_caps *caps,
                        void *drvdata)
{
     port = wwan_port_allocate(wwandev, type, ops, caps, drvdata);
     /* Proceed with GNSS registration or release on failure */
     /* return port; or return ERR_PTR(-err); */
}

struct wwan_port *wwan_create_port(struct device *parent,
                                    enum wwan_port_type type,
                                    const struct wwan_port_ops *ops,
                                    struct wwan_port_caps *caps,
                                    void *drvdata)
{
     ...
     wwandev = wwan_create_dev(parent);
     if (type == WWAN_PORT_NMEA)
         port = wwan_port_register_gnss(wwandev, type, ops,
                                        caps, drvdata);
     else
         port = wwan_port_register_wwan(wwandev, type, ops,
                                        caps, drvdata);
     if (!IS_ERR(port))
         return port;
     wwan_remove_dev(wwandev);
     return ERR_CAST(port);
}

wwan_create_port() looks better in prices of passing a list of arguments 
and allocating the port in multiple places.

Maybe some other design approach, what was overseen?


For me, the ideal solution would be a routine that works like 
put_device() except calling the device type release handler. Then we can 
use it to cleanup leftovers of the failed device_register() call and 
then release the memory in the calling wwan_create_port() function.

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

--
Sergey

Re: [RFC PATCH 3/6] net: wwan: core: split port unregister and stop

[Sergey Ryazanov]

On 14.04.2025 21:54, Loic Poulain wrote:
> On Wed, Apr 9, 2025 at 1:31 AM Sergey Ryazanov <ryazanov.s.a@gmail.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: call the port stop operation, purge queues, release the parent
>> WWAN device, etc. To reuse as much code as possible, split the port
>> unregistering function into the deregistration of a regular WWAN port
>> device, and the common port tearing down code.
>>
>> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
>> ---
>>   drivers/net/wwan/wwan_core.c | 21 ++++++++++++++++-----
>>   1 file changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
>> index 045246d7cd50..439a57bc2b9c 100644
>> --- a/drivers/net/wwan/wwan_core.c
>> +++ b/drivers/net/wwan/wwan_core.c
>> @@ -486,6 +486,18 @@ static int wwan_port_register_wwan(struct wwan_port *port)
>>          return 0;
>>   }
>>
>> +/* Unregister regular WWAN port (e.g. AT, MBIM, etc) */
>> +static void wwan_port_unregister_wwan(struct wwan_port *port)
> 
> Wouldn't it be simpler to name it  `wwan_port_unregister` ?

I came with this complex name for a symmetry purpose. The next patch 
going to introduce wwan_port_unregister_gnss() handler.

The prefix indicates the module and the suffix indicates the type of the 
unregistering port.

>> +{
>> +       struct wwan_device *wwandev = to_wwan_dev(port->dev.parent);
>> +
>> +       dev_set_drvdata(&port->dev, NULL);
>> +
>> +       dev_info(&wwandev->dev, "port %s disconnected\n", dev_name(&port->dev));
>> +
>> +       device_unregister(&port->dev);
>> +}
>> +
>>   struct wwan_port *wwan_create_port(struct device *parent,
>>                                     enum wwan_port_type type,
>>                                     const struct wwan_port_ops *ops,
>> @@ -542,18 +554,17 @@ void wwan_remove_port(struct wwan_port *port)
>>          struct wwan_device *wwandev = to_wwan_dev(port->dev.parent);
>>
>>          mutex_lock(&port->ops_lock);
>> -       if (port->start_count)
>> +       if (port->start_count) {
>>                  port->ops->stop(port);
>> +               port->start_count = 0;
>> +       }
>>          port->ops = NULL; /* Prevent any new port operations (e.g. from fops) */
>>          mutex_unlock(&port->ops_lock);
>>
>>          wake_up_interruptible(&port->waitqueue);
>> -
>>          skb_queue_purge(&port->rxq);
>> -       dev_set_drvdata(&port->dev, NULL);
>>
>> -       dev_info(&wwandev->dev, "port %s disconnected\n", dev_name(&port->dev));
>> -       device_unregister(&port->dev);
>> +       wwan_port_unregister_wwan(port);
>>
>>          /* Release related wwan device */
>>          wwan_remove_dev(wwandev);
>> --
>> 2.45.3
>>

Re: [RFC PATCH 4/6] net: wwan: add NMEA port support

[Loic Poulain]

On Wed, Apr 9, 2025 at 1:31 AM Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>
> Many WWAN modems come with embedded GNSS receiver inside and have a
> dedicated port to output geopositioning data. On the one hand, the
> GNSS receiver has little in common with WWAN modem and just shares a
> host interface and should be exported using the GNSS subsystem. On the
> other hand, GNSS receiver is not automatically activated and needs a
> generic WWAN control port (AT, MBIM, etc.) to be turned on. And a user
> space software needs extra information to find the control port.
>
> Introduce the new type of WWAN port - NMEA. When driver asks to register
> a NMEA port, the core allocates common parent WWAN device as usual, but
> exports the NMEA port via the GNSS subsystem and acts as a proxy between
> the device driver and the GNSS subsystem.
>
> From the WWAN device driver perspective, a NMEA port is registered as a
> regular WWAN port without any difference. And the driver interacts only
> with the WWAN core. From the user space perspective, the NMEA port is a
> GNSS device which parent can be used to enumerate and select the proper
> control port for the GNSS receiver management.
>
> CC: Slark Xiao <slark_xiao@163.com>
> CC: Muhammad Nuzaihan <zaihan@unrealasia.net>
> CC: Qiang Yu <quic_qianyu@quicinc.com>
> CC: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> CC: Johan Hovold <johan@kernel.org>
> Suggested-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
> ---
>  drivers/net/wwan/Kconfig     |   1 +
>  drivers/net/wwan/wwan_core.c | 157 ++++++++++++++++++++++++++++++++++-
>  include/linux/wwan.h         |   2 +
>  3 files changed, 156 insertions(+), 4 deletions(-)
>
[...]
> +static void wwan_port_unregister_gnss(struct wwan_port *port)
> +{
> +       struct wwan_device *wwandev = to_wwan_dev(port->dev.parent);
> +       struct gnss_device *gdev = port->gnss;
> +
> +       dev_info(&wwandev->dev, "port %s disconnected\n", dev_name(&gdev->dev));
> +
> +       gnss_deregister_device(gdev);
> +       gnss_put_device(gdev);
> +
> +       __wwan_port_destroy(port);
> +}
> +#else
> +static inline int wwan_port_register_gnss(struct wwan_port *port)
> +{
> +       __wwan_port_destroy(port);
> +       return -EOPNOTSUPP;
> +}

I don't think the wwan core should return an error in case GNSS_CONFIG
is not enabled, a wwan driver may consider aborting the full
probing/registration if one of the port registrations is failing.
Maybe we should silently ignore such ports, and/or simply print a
warning.

> +
> +static inline void wwan_port_unregister_gnss(struct wwan_port *port)
> +{
> +       WARN_ON(1);     /* This handler cannot be called */
> +}
> +#endif

Re: [RFC PATCH 3/6] net: wwan: core: split port unregister and stop

[Loic Poulain]

On Mon, Apr 14, 2025 at 11:33 PM Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>
> On 14.04.2025 21:54, Loic Poulain wrote:
> > On Wed, Apr 9, 2025 at 1:31 AM Sergey Ryazanov <ryazanov.s.a@gmail.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: call the port stop operation, purge queues, release the parent
> >> WWAN device, etc. To reuse as much code as possible, split the port
> >> unregistering function into the deregistration of a regular WWAN port
> >> device, and the common port tearing down code.
> >>
> >> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
> >> ---
> >>   drivers/net/wwan/wwan_core.c | 21 ++++++++++++++++-----
> >>   1 file changed, 16 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
> >> index 045246d7cd50..439a57bc2b9c 100644
> >> --- a/drivers/net/wwan/wwan_core.c
> >> +++ b/drivers/net/wwan/wwan_core.c
> >> @@ -486,6 +486,18 @@ static int wwan_port_register_wwan(struct wwan_port *port)
> >>          return 0;
> >>   }
> >>
> >> +/* Unregister regular WWAN port (e.g. AT, MBIM, etc) */
> >> +static void wwan_port_unregister_wwan(struct wwan_port *port)
> >
> > Wouldn't it be simpler to name it  `wwan_port_unregister` ?
>
> I came with this complex name for a symm>
> >> +{
> >> +       struct wwan_device *wwandev = to_wwan_dev(port->dev.parent);
> >> +
> >> +       dev_set_drvdata(&port->dev, NULL);
> >> +
> >> +       dev_info(&wwandev->dev, "port %s disconnected\n", dev_name(&port->dev));
> >> +
> >> +       device_unregister(&port->dev);
> >> +}
> >> +
> >>   struct wwan_port *wwan_create_port(struct device *parent,
> >>                                     enum wwan_port_type type,
> >>                                     const struct wwan_port_ops *ops,
> >> @@ -542,18 +554,17 @@ void wwan_remove_port(struct wwan_port *port)
> >>          struct wwan_device *wwandev = to_wwan_dev(port->dev.parent);
> >>
> >>          mutex_lock(&port->ops_lock);
> >> -       if (port->start_count)
> >> +       if (port->start_count) {
> >>                  port->ops->stop(port);
> >> +               port->start_count = 0;
> >> +       }
> >>          port->ops = NULL; /* Prevent any new port operations (e.g. from fops) */
> >>          mutex_unlock(&port->ops_lock);
> >>
> >>          wake_up_interruptible(&port->waitqueue);
> >> -
> >>          skb_queue_purge(&port->rxq);
> >> -       dev_set_drvdata(&port->dev, NULL);
> >>
> >> -       dev_info(&wwandev->dev, "port %s disconnected\n", dev_name(&port->dev));
> >> -       device_unregister(&port->dev);
> >> +       wwan_port_unregister_wwan(port);
> >>
> >>          /* Release related wwan device */
> >>          wwan_remove_dev(wwandev);
> >> --
> >> 2.45.3
> >>
>etry purpose. The next patch
> going to introduce wwan_port_unregister_gnss() handler.
>
> The prefix indicates the module and the suffix indicates the type of the
> unregistering port.

Ok, fair enough.

Re: [RFC PATCH 2/6] net: wwan: core: split port creation and registration

[Loic Poulain]

Hi Sergey,

On Mon, Apr 14, 2025 at 11:28 PM Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>
> Hi Loic,
>
> thank you that you found a time to check it. See the explanation below,
> might be you can suggest a better solution.
>
> On 14.04.2025 21:50, Loic Poulain wrote:
> > Hi Sergey,
> >
> > On Wed, Apr 9, 2025 at 1:31 AM Sergey Ryazanov <ryazanov.s.a@gmail.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@gmail.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?
>
> I agree that this looks weird and asymmetrical. I left the port
> allocation in wwan_create_port() because both WWAN-exported and
> GNSS-exported types of port share the same port allocation. And the port
> struct is used as a container to keep all the port registration arguments.
>
> I did the port freeing inside this function because we free the port
> differently depending of the device registration state. If we fail to
> initialize the port at earlier stage then we use __wwan_port_destroy()
> which basically just releases the memory.
>
> But if device_register() fails then we are required to use put_device()
> which does more job.
>
> I do not think it is acceptable to skip put_device() call and just
> release the memory. Also I do not find maintainable to partially open
> code put_device() here in the WWAN-exportable handler and release the
> memory in caller function wwan_create_port().
>
> We could somehow try to return this information from
> wwan_port_register_wwan() to wwan_create_port(), so the caller could
> decide, shall it use __wwan_port_destroy() or put_device() in case of
> failure.
>
> But I can not see a way to clearly indicate, which releasing approach
> should be used by the caller. And even in this case it going to look
> weird since the called function controls the caller.
>
> Another solution for the asymmetry problem is to move the allocation
> from the caller to the called function. So the memory will be allocated
> and released in the same function. But in this case we will need to pass
> all the parameters from wwan_create_port() to wwan_port_register_wwan().
> Even if we consolidate the port basic allocation/initialization in a
> common routine, the final solution going to look a duplication. E.g.
>
> struct wwan_port *wwan_port_allocate(struct wwan_device *wwandev,
>                                       enum wwan_port_type type,
>                                       const struct wwan_port_ops *ops,
>                                       struct wwan_port_caps *caps,
>                                       void *drvdata)
> {
>      /* Do the mem allocation and init here */
>      return port;
> }
>
> struct wwan_port *wwan_port_register_wwan(struct wwan_device *wwandev,
>                         enum wwan_port_type type,
>                         const struct wwan_port_ops *ops,
>                         struct wwan_port_caps *caps,
>                         void *drvdata)
> {
>      port = wwan_port_allocate(wwandev, type, ops, caps, drvdata);
>      /* Proceed with chardev registration or release on failure */
>      /* return port; or return ERR_PTR(-err); */
> }
>
> struct wwan_port *wwan_port_register_gnss(struct wwan_device *wwandev,
>                         enum wwan_port_type type,
>                         const struct wwan_port_ops *ops,
>                         struct wwan_port_caps *caps,
>                         void *drvdata)
> {
>      port = wwan_port_allocate(wwandev, type, ops, caps, drvdata);
>      /* Proceed with GNSS registration or release on failure */
>      /* return port; or return ERR_PTR(-err); */
> }
>
> struct wwan_port *wwan_create_port(struct device *parent,
>                                     enum wwan_port_type type,
>                                     const struct wwan_port_ops *ops,
>                                     struct wwan_port_caps *caps,
>                                     void *drvdata)
> {
>      ...
>      wwandev = wwan_create_dev(parent);
>      if (type == WWAN_PORT_NMEA)
>          port = wwan_port_register_gnss(wwandev, type, ops,
>                                         caps, drvdata);
>      else
>          port = wwan_port_register_wwan(wwandev, type, ops,
>                                         caps, drvdata);
>      if (!IS_ERR(port))
>          return port;
>      wwan_remove_dev(wwandev);
>      return ERR_CAST(port);
> }
>
> wwan_create_port() looks better in prices of passing a list of arguments
> and allocating the port in multiple places.
>
> Maybe some other design approach, what was overseen?
>
>
> For me, the ideal solution would be a routine that works like
> put_device() except calling the device type release handler. Then we can
> use it to cleanup leftovers of the failed device_register() call and
> then release the memory in the calling wwan_create_port() function.

Ok I see, thanks for the clear explanation, I don't see a perfect
solution here without over complication. So the current approach is
acceptable, can you add a comment in the caller function as well,so
that it's clear why we don't have to release the port on error.

Re: [RFC PATCH 4/6] net: wwan: add NMEA port support

[Loic Poulain]

On Wed, Apr 9, 2025 at 1:31 AM Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>
> Many WWAN modems come with embedded GNSS receiver inside and have a
> dedicated port to output geopositioning data. On the one hand, the
> GNSS receiver has little in common with WWAN modem and just shares a
> host interface and should be exported using the GNSS subsystem. On the
> other hand, GNSS receiver is not automatically activated and needs a
> generic WWAN control port (AT, MBIM, etc.) to be turned on. And a user
> space software needs extra information to find the control port.
>
> Introduce the new type of WWAN port - NMEA. When driver asks to register
> a NMEA port, the core allocates common parent WWAN device as usual, but
> exports the NMEA port via the GNSS subsystem and acts as a proxy between
> the device driver and the GNSS subsystem.
>
> From the WWAN device driver perspective, a NMEA port is registered as a
> regular WWAN port without any difference. And the driver interacts only
> with the WWAN core. From the user space perspective, the NMEA port is a
> GNSS device which parent can be used to enumerate and select the proper
> control port for the GNSS receiver management.
>
> CC: Slark Xiao <slark_xiao@163.com>
> CC: Muhammad Nuzaihan <zaihan@unrealasia.net>
> CC: Qiang Yu <quic_qianyu@quicinc.com>
> CC: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> CC: Johan Hovold <johan@kernel.org>
> Suggested-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
> ---
>  drivers/net/wwan/Kconfig     |   1 +
>  drivers/net/wwan/wwan_core.c | 157 ++++++++++++++++++++++++++++++++++-
>  include/linux/wwan.h         |   2 +
>  3 files changed, 156 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wwan/Kconfig b/drivers/net/wwan/Kconfig
> index 410b0245114e..88df55d78d90 100644
> --- a/drivers/net/wwan/Kconfig
> +++ b/drivers/net/wwan/Kconfig
> @@ -7,6 +7,7 @@ menu "Wireless WAN"
>
>  config WWAN
>         tristate "WWAN Driver Core"
> +       depends on GNSS || GNSS = n
>         help
>           Say Y here if you want to use the WWAN driver core. This driver
>           provides a common framework for WWAN drivers.
> diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
> index 439a57bc2b9c..a30f0c89aa82 100644
[...]
> +static int wwan_port_register_gnss(struct wwan_port *port)
> +{
> +       struct wwan_device *wwandev = to_wwan_dev(port->dev.parent);
> +       struct gnss_device *gdev;
> +       int err;
> +
> +       gdev = gnss_allocate_device(&wwandev->dev);
> +       if (!gdev) {
> +               err = -ENOMEM;
> +               goto error_destroy_port;
> +       }
> +
> +       /* NB: for now we support only NMEA WWAN port type, so hardcode
> +        * the GNSS port type. If more GNSS WWAN port types will be added,
> +        * then we should dynamically mapt WWAN port type to GNSS type.

typo: mapt

Re: [RFC PATCH 2/6] net: wwan: core: split port creation and registration

[Sergey Ryazanov]

Hi Loic,

please find one extra option below.

On 17.04.2025 23:35, Loic Poulain wrote:
> Hi Sergey,
> 
> On Mon, Apr 14, 2025 at 11:28 PM Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>>
>> Hi Loic,
>>
>> thank you that you found a time to check it. See the explanation below,
>> might be you can suggest a better solution.
>>
>> On 14.04.2025 21:50, Loic Poulain wrote:
>>> Hi Sergey,
>>>
>>> On Wed, Apr 9, 2025 at 1:31 AM Sergey Ryazanov <ryazanov.s.a@gmail.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@gmail.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?
>>
>> I agree that this looks weird and asymmetrical. I left the port
>> allocation in wwan_create_port() because both WWAN-exported and
>> GNSS-exported types of port share the same port allocation. And the port
>> struct is used as a container to keep all the port registration arguments.
>>
>> I did the port freeing inside this function because we free the port
>> differently depending of the device registration state. If we fail to
>> initialize the port at earlier stage then we use __wwan_port_destroy()
>> which basically just releases the memory.
>>
>> But if device_register() fails then we are required to use put_device()
>> which does more job.
>>
>> I do not think it is acceptable to skip put_device() call and just
>> release the memory. Also I do not find maintainable to partially open
>> code put_device() here in the WWAN-exportable handler and release the
>> memory in caller function wwan_create_port().
>>
>> We could somehow try to return this information from
>> wwan_port_register_wwan() to wwan_create_port(), so the caller could
>> decide, shall it use __wwan_port_destroy() or put_device() in case of
>> failure.
>>
>> But I can not see a way to clearly indicate, which releasing approach
>> should be used by the caller. And even in this case it going to look
>> weird since the called function controls the caller.
>>
>> Another solution for the asymmetry problem is to move the allocation
>> from the caller to the called function. So the memory will be allocated
>> and released in the same function. But in this case we will need to pass
>> all the parameters from wwan_create_port() to wwan_port_register_wwan().
>> Even if we consolidate the port basic allocation/initialization in a
>> common routine, the final solution going to look a duplication. E.g.
>>
>> struct wwan_port *wwan_port_allocate(struct wwan_device *wwandev,
>>                                        enum wwan_port_type type,
>>                                        const struct wwan_port_ops *ops,
>>                                        struct wwan_port_caps *caps,
>>                                        void *drvdata)
>> {
>>       /* Do the mem allocation and init here */
>>       return port;
>> }
>>
>> struct wwan_port *wwan_port_register_wwan(struct wwan_device *wwandev,
>>                          enum wwan_port_type type,
>>                          const struct wwan_port_ops *ops,
>>                          struct wwan_port_caps *caps,
>>                          void *drvdata)
>> {
>>       port = wwan_port_allocate(wwandev, type, ops, caps, drvdata);
>>       /* Proceed with chardev registration or release on failure */
>>       /* return port; or return ERR_PTR(-err); */
>> }
>>
>> struct wwan_port *wwan_port_register_gnss(struct wwan_device *wwandev,
>>                          enum wwan_port_type type,
>>                          const struct wwan_port_ops *ops,
>>                          struct wwan_port_caps *caps,
>>                          void *drvdata)
>> {
>>       port = wwan_port_allocate(wwandev, type, ops, caps, drvdata);
>>       /* Proceed with GNSS registration or release on failure */
>>       /* return port; or return ERR_PTR(-err); */
>> }
>>
>> struct wwan_port *wwan_create_port(struct device *parent,
>>                                      enum wwan_port_type type,
>>                                      const struct wwan_port_ops *ops,
>>                                      struct wwan_port_caps *caps,
>>                                      void *drvdata)
>> {
>>       ...
>>       wwandev = wwan_create_dev(parent);
>>       if (type == WWAN_PORT_NMEA)
>>           port = wwan_port_register_gnss(wwandev, type, ops,
>>                                          caps, drvdata);
>>       else
>>           port = wwan_port_register_wwan(wwandev, type, ops,
>>                                          caps, drvdata);
>>       if (!IS_ERR(port))
>>           return port;
>>       wwan_remove_dev(wwandev);
>>       return ERR_CAST(port);
>> }
>>
>> wwan_create_port() looks better in prices of passing a list of arguments
>> and allocating the port in multiple places.
>>
>> Maybe some other design approach, what was overseen?
>>
>>
>> For me, the ideal solution would be a routine that works like
>> put_device() except calling the device type release handler. Then we can
>> use it to cleanup leftovers of the failed device_register() call and
>> then release the memory in the calling wwan_create_port() function.
> 
> Ok I see, thanks for the clear explanation, I don't see a perfect
> solution here without over complication. So the current approach is
> acceptable, can you add a comment in the caller function as well,so
> that it's clear why we don't have to release the port on error.

Looks like I've found another one solution to move the port resources 
(memory) release back to the function allocating it. It is also a bit 
hackish and I would like to hear a feedback from you.

The port is released inside wwan_port_register_wwan() just because we 
release it differently depending on the initialization stage, right?

We cannot call put_device() before the device_register() call. And that 
is why I've created that __wwan_port_destroy() routine. What if we make 
port eligible to be released with put_device() ASAP? Internally, 
device_register() just sequentially calls device_initialize() and then 
device_add(). Indeed, device_initialize() makes a device struct eligible 
to be released with put_device().

We can avoid using device_register() and instead of it, do the 
registration step by step. Just after the port memory allocation and 
very basic initialization, we can call device_initialize(). And then 
call device_add() when it is time to register the port with the kernel. 
And if something going to go wrong, we can return just an error from 
wwan_port_register_wwan() and release the port with put_device() in 
wwan_create_port() where it was allocated. Something like this:

static int wwan_port_register_wwan(struct wwan_port *port)
{
     ...
     if (something_wrong)
         return -E<ERROR_TYPE>;
     ...
     return 0;
}

struct wwan_port *wwan_create_port(struct device *parent,
                                    enum wwan_port_type type,
                                    const struct wwan_port_ops *ops,
                                    struct wwan_port_caps *caps,
                                    void *drvdata)
{
     ...
     port = kzalloc(sizeof(*port), GFP_KERNEL);
     /* Do basic port init here */
     port->dev.type = &wwan_port_dev_type;
     device_initialize(&port->dev);  /* allows put_device() usage */

     if (port->type == WWAN_PORT_NMEA)
         err = wwan_port_register_gnss(port);
     else
         err = wwan_port_register_wwan(port);

     if (err) {
         put_device(&port->dev);
         goto error_wwandev_remove;
     }

     return port;
     ...
}

The only drawback I see here is that we have to use put_device() to 
release the port memory even in case of GNSS port. We don't actually 
register the port as device, but I believe, this can be explained with a 
proper comment.

What do you think, is it worth to try to rework the code in this way?

--
Sergey

Re: [RFC PATCH 4/6] net: wwan: add NMEA port support

[Sergey Ryazanov]

On 17.04.2025 23:41, Loic Poulain wrote:
> On Wed, Apr 9, 2025 at 1:31 AM Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
[...]
>> +static int wwan_port_register_gnss(struct wwan_port *port)
>> +{
>> +       /* NB: for now we support only NMEA WWAN port type, so hardcode
>> +        * the GNSS port type. If more GNSS WWAN port types will be added,
>> +        * then we should dynamically mapt WWAN port type to GNSS type.
> 
> typo: mapt

Nice catch. Will fix it.

--
Sergey

Re: [RFC PATCH 4/6] net: wwan: add NMEA port support

[Sergey Ryazanov]

On 16.04.2025 23:04, Loic Poulain wrote:
> On Wed, Apr 9, 2025 at 1:31 AM Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>> Many WWAN modems come with embedded GNSS receiver inside and have a
>> dedicated port to output geopositioning data. On the one hand, the
>> GNSS receiver has little in common with WWAN modem and just shares a
>> host interface and should be exported using the GNSS subsystem. On the
>> other hand, GNSS receiver is not automatically activated and needs a
>> generic WWAN control port (AT, MBIM, etc.) to be turned on. And a user
>> space software needs extra information to find the control port.
>>
>> Introduce the new type of WWAN port - NMEA. When driver asks to register
>> a NMEA port, the core allocates common parent WWAN device as usual, but
>> exports the NMEA port via the GNSS subsystem and acts as a proxy between
>> the device driver and the GNSS subsystem.
>>
>>  From the WWAN device driver perspective, a NMEA port is registered as a
>> regular WWAN port without any difference. And the driver interacts only
>> with the WWAN core. From the user space perspective, the NMEA port is a
>> GNSS device which parent can be used to enumerate and select the proper
>> control port for the GNSS receiver management.
>>
>> CC: Slark Xiao <slark_xiao@163.com>
>> CC: Muhammad Nuzaihan <zaihan@unrealasia.net>
>> CC: Qiang Yu <quic_qianyu@quicinc.com>
>> CC: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>> CC: Johan Hovold <johan@kernel.org>
>> Suggested-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
>> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
>> ---
>>   drivers/net/wwan/Kconfig     |   1 +
>>   drivers/net/wwan/wwan_core.c | 157 ++++++++++++++++++++++++++++++++++-
>>   include/linux/wwan.h         |   2 +
>>   3 files changed, 156 insertions(+), 4 deletions(-)
>>
> [...]
>> +static void wwan_port_unregister_gnss(struct wwan_port *port)
>> +{
>> +       struct wwan_device *wwandev = to_wwan_dev(port->dev.parent);
>> +       struct gnss_device *gdev = port->gnss;
>> +
>> +       dev_info(&wwandev->dev, "port %s disconnected\n", dev_name(&gdev->dev));
>> +
>> +       gnss_deregister_device(gdev);
>> +       gnss_put_device(gdev);
>> +
>> +       __wwan_port_destroy(port);
>> +}
>> +#else
>> +static inline int wwan_port_register_gnss(struct wwan_port *port)
>> +{
>> +       __wwan_port_destroy(port);
>> +       return -EOPNOTSUPP;
>> +}
> 
> I don't think the wwan core should return an error in case GNSS_CONFIG
> is not enabled, a wwan driver may consider aborting the full
> probing/registration if one of the port registrations is failing.
> Maybe we should silently ignore such ports, and/or simply print a
> warning.

Good point. We can end up with the case of driver aborting the whole 
registration.

On the other hand, a driver author is supposed to know that for the GNSS 
functionality the corresponding subsystem should be enabled. And either 
don't even try to register the GNSS (NMEA) port if the system is 
disabled or handle the error properly. See for example, the Intel's NIC 
driver, which skips an embedded GNSS receiver exporting if the GNSS 
subsystem is disabled. We can catch such issues on the review stage or 
treat like bug if something like this going to sneak into the code.

So here, the WWAN core does its best to kindly inform that functionality 
is not enabled. This is what many other components do, and I personally 
like this approach.

Is it any good justification to clearly indicate the error in case of 
GNSS subsystem is not enabled?

--
Sergey

Re: [RFC PATCH 2/6] net: wwan: core: split port creation and registration

[Loic Poulain]

On Sat, Apr 19, 2025 at 1:04 AM Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>
> Hi Loic,
>
> please find one extra option below.
>
> On 17.04.2025 23:35, Loic Poulain wrote:
> > Hi Sergey,
> >
> > On Mon, Apr 14, 2025 at 11:28 PM Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
> >>
> >> Hi Loic,
> >>
> >> thank you that you found a time to check it. See the explanation below,
> >> might be you can suggest a better solution.
> >>
> >> On 14.04.2025 21:50, Loic Poulain wrote:
> >>> Hi Sergey,
> >>>
> >>> On Wed, Apr 9, 2025 at 1:31 AM Sergey Ryazanov <ryazanov.s.a@gmail.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@gmail.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?
> >>
> >> I agree that this looks weird and asymmetrical. I left the port
> >> allocation in wwan_create_port() because both WWAN-exported and
> >> GNSS-exported types of port share the same port allocation. And the port
> >> struct is used as a container to keep all the port registration arguments.
> >>
> >> I did the port freeing inside this function because we free the port
> >> differently depending of the device registration state. If we fail to
> >> initialize the port at earlier stage then we use __wwan_port_destroy()
> >> which basically just releases the memory.
> >>
> >> But if device_register() fails then we are required to use put_device()
> >> which does more job.
> >>
> >> I do not think it is acceptable to skip put_device() call and just
> >> release the memory. Also I do not find maintainable to partially open
> >> code put_device() here in the WWAN-exportable handler and release the
> >> memory in caller function wwan_create_port().
> >>
> >> We could somehow try to return this information from
> >> wwan_port_register_wwan() to wwan_create_port(), so the caller could
> >> decide, shall it use __wwan_port_destroy() or put_device() in case of
> >> failure.
> >>
> >> But I can not see a way to clearly indicate, which releasing approach
> >> should be used by the caller. And even in this case it going to look
> >> weird since the called function controls the caller.
> >>
> >> Another solution for the asymmetry problem is to move the allocation
> >> from the caller to the called function. So the memory will be allocated
> >> and released in the same function. But in this case we will need to pass
> >> all the parameters from wwan_create_port() to wwan_port_register_wwan().
> >> Even if we consolidate the port basic allocation/initialization in a
> >> common routine, the final solution going to look a duplication. E.g.
> >>
> >> struct wwan_port *wwan_port_allocate(struct wwan_device *wwandev,
> >>                                        enum wwan_port_type type,
> >>                                        const struct wwan_port_ops *ops,
> >>                                        struct wwan_port_caps *caps,
> >>                                        void *drvdata)
> >> {
> >>       /* Do the mem allocation and init here */
> >>       return port;
> >> }
> >>
> >> struct wwan_port *wwan_port_register_wwan(struct wwan_device *wwandev,
> >>                          enum wwan_port_type type,
> >>                          const struct wwan_port_ops *ops,
> >>                          struct wwan_port_caps *caps,
> >>                          void *drvdata)
> >> {
> >>       port = wwan_port_allocate(wwandev, type, ops, caps, drvdata);
> >>       /* Proceed with chardev registration or release on failure */
> >>       /* return port; or return ERR_PTR(-err); */
> >> }
> >>
> >> struct wwan_port *wwan_port_register_gnss(struct wwan_device *wwandev,
> >>                          enum wwan_port_type type,
> >>                          const struct wwan_port_ops *ops,
> >>                          struct wwan_port_caps *caps,
> >>                          void *drvdata)
> >> {
> >>       port = wwan_port_allocate(wwandev, type, ops, caps, drvdata);
> >>       /* Proceed with GNSS registration or release on failure */
> >>       /* return port; or return ERR_PTR(-err); */
> >> }
> >>
> >> struct wwan_port *wwan_create_port(struct device *parent,
> >>                                      enum wwan_port_type type,
> >>                                      const struct wwan_port_ops *ops,
> >>                                      struct wwan_port_caps *caps,
> >>                                      void *drvdata)
> >> {
> >>       ...
> >>       wwandev = wwan_create_dev(parent);
> >>       if (type == WWAN_PORT_NMEA)
> >>           port = wwan_port_register_gnss(wwandev, type, ops,
> >>                                          caps, drvdata);
> >>       else
> >>           port = wwan_port_register_wwan(wwandev, type, ops,
> >>                                          caps, drvdata);
> >>       if (!IS_ERR(port))
> >>           return port;
> >>       wwan_remove_dev(wwandev);
> >>       return ERR_CAST(port);
> >> }
> >>
> >> wwan_create_port() looks better in prices of passing a list of arguments
> >> and allocating the port in multiple places.
> >>
> >> Maybe some other design approach, what was overseen?
> >>
> >>
> >> For me, the ideal solution would be a routine that works like
> >> put_device() except calling the device type release handler. Then we can
> >> use it to cleanup leftovers of the failed device_register() call and
> >> then release the memory in the calling wwan_create_port() function.
> >
> > Ok I see, thanks for the clear explanation, I don't see a perfect
> > solution here without over complication. So the current approach is
> > acceptable, can you add a comment in the caller function as well,so
> > that it's clear why we don't have to release the port on error.
>
> Looks like I've found another one solution to move the port resources
> (memory) release back to the function allocating it. It is also a bit
> hackish and I would like to hear a feedback from you.
>
> The port is released inside wwan_port_register_wwan() just because we
> release it differently depending on the initialization stage, right?
>
> We cannot call put_device() before the device_register() call. And that
> is why I've created that __wwan_port_destroy() routine. What if we make
> port eligible to be released with put_device() ASAP? Internally,
> device_register() just sequentially calls device_initialize() and then
> device_add(). Indeed, device_initialize() makes a device struct eligible
> to be released with put_device().
>
> We can avoid using device_register() and instead of it, do the
> registration step by step. Just after the port memory allocation and
> very basic initialization, we can call device_initialize(). And then
> call device_add() when it is time to register the port with the kernel.
> And if something going to go wrong, we can return just an error from
> wwan_port_register_wwan() and release the port with put_device() in
> wwan_create_port() where it was allocated. Something like this:
>
> static int wwan_port_register_wwan(struct wwan_port *port)
> {
>      ...
>      if (something_wrong)
>          return -E<ERROR_TYPE>;
>      ...
>      return 0;
> }
>
> struct wwan_port *wwan_create_port(struct device *parent,
>                                     enum wwan_port_type type,
>                                     const struct wwan_port_ops *ops,
>                                     struct wwan_port_caps *caps,
>                                     void *drvdata)
> {
>      ...
>      port = kzalloc(sizeof(*port), GFP_KERNEL);
>      /* Do basic port init here */
>      port->dev.type = &wwan_port_dev_type;
>      device_initialize(&port->dev);  /* allows put_device() usage */
>
>      if (port->type == WWAN_PORT_NMEA)
>          err = wwan_port_register_gnss(port);
>      else
>          err = wwan_port_register_wwan(port);
>
>      if (err) {
>          put_device(&port->dev);
>          goto error_wwandev_remove;
>      }
>
>      return port;
>      ...
> }
>
> The only drawback I see here is that we have to use put_device() to
> release the port memory even in case of GNSS port. We don't actually
> register the port as device, but I believe, this can be explained with a
> proper comment.

Yes, that's a good alternative, so you would also have to use
put_device in gnss_unregister, or do something like:

void wwan_remove_port(struct wwan_port *port)
{
[...]
    if (port->type == WWAN_PORT_NMEA)
                wwan_port_unregister_gnss(port);
    /* common unregistering (put_device), not necessarily in a
separate function */
   wwan_port_unregister(port);
[...]
}

And probably have a common wwan_port_destroy function:
static void wwan_port_destroy(struct device *dev)
{
    if (port->type != WWAN_PORT_NMEA)
        ida_free(&minors, MINOR(dev->devt));
    [...]
    kfree(port);
}

Regards,
Loic

Re: [RFC PATCH 2/6] net: wwan: core: split port creation and registration

[Sergey Ryazanov]

On 19.04.2025 14:44, Loic Poulain wrote:
> On Sat, Apr 19, 2025 at 1:04 AM Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>> ...
>> Looks like I've found another one solution to move the port resources
>> (memory) release back to the function allocating it. It is also a bit
>> hackish and I would like to hear a feedback from you.
>>
>> The port is released inside wwan_port_register_wwan() just because we
>> release it differently depending on the initialization stage, right?
>>
>> We cannot call put_device() before the device_register() call. And that
>> is why I've created that __wwan_port_destroy() routine. What if we make
>> port eligible to be released with put_device() ASAP? Internally,
>> device_register() just sequentially calls device_initialize() and then
>> device_add(). Indeed, device_initialize() makes a device struct eligible
>> to be released with put_device().
>>
>> We can avoid using device_register() and instead of it, do the
>> registration step by step. Just after the port memory allocation and
>> very basic initialization, we can call device_initialize(). And then
>> call device_add() when it is time to register the port with the kernel.
>> And if something going to go wrong, we can return just an error from
>> wwan_port_register_wwan() and release the port with put_device() in
>> wwan_create_port() where it was allocated. Something like this:
>>
>> static int wwan_port_register_wwan(struct wwan_port *port)
>> {
>>       ...
>>       if (something_wrong)
>>           return -E<ERROR_TYPE>;
>>       ...
>>       return 0;
>> }
>>
>> struct wwan_port *wwan_create_port(struct device *parent,
>>                                      enum wwan_port_type type,
>>                                      const struct wwan_port_ops *ops,
>>                                      struct wwan_port_caps *caps,
>>                                      void *drvdata)
>> {
>>       ...
>>       port = kzalloc(sizeof(*port), GFP_KERNEL);
>>       /* Do basic port init here */
>>       port->dev.type = &wwan_port_dev_type;
>>       device_initialize(&port->dev);  /* allows put_device() usage */
>>
>>       if (port->type == WWAN_PORT_NMEA)
>>           err = wwan_port_register_gnss(port);
>>       else
>>           err = wwan_port_register_wwan(port);
>>
>>       if (err) {
>>           put_device(&port->dev);
>>           goto error_wwandev_remove;
>>       }
>>
>>       return port;
>>       ...
>> }
>>
>> The only drawback I see here is that we have to use put_device() to
>> release the port memory even in case of GNSS port. We don't actually
>> register the port as device, but I believe, this can be explained with a
>> proper comment.
> 
> Yes, that's a good alternative, so you would also have to use
> put_device in gnss_unregister, or do something like:
> 
> void wwan_remove_port(struct wwan_port *port)
> {
> [...]
>      if (port->type == WWAN_PORT_NMEA)
>                  wwan_port_unregister_gnss(port);
>      /* common unregistering (put_device), not necessarily in a
> separate function */
>     wwan_port_unregister(port);
> [...]
> }
> 
> And probably have a common wwan_port_destroy function:
> static void wwan_port_destroy(struct device *dev)
> {
>      if (port->type != WWAN_PORT_NMEA)
>          ida_free(&minors, MINOR(dev->devt));
>      [...]
>      kfree(port);
> }

Yep. This change also makes sense if we manage to release the port in a 
unified way. Will try to implement it next week to see how it's going to 
look like in as a code.

--
Sergey

Re:Re:Re:[RFC PATCH 4/6] net: wwan: add NMEA port support

[Slark Xiao]

At 2025-04-09 18:42:59, "Sergey Ryazanov" <ryazanov.s.a@gmail.com> wrote:
>On April 9, 2025 11:30:58 AM GMT+03:00, Slark Xiao <slark_xiao@163.com> wrote:
>>
>>Hi Sergey,
>>Device port /dev/gnss0 is enumerated . Does it be expected?
>>I can get the NMEA data from this port by cat or minicom command.
>>But the gpsd.service also can not be initialized normally. It reports:
>>
>>TriggeredBy: ● gpsd.socket
>>    Process: 3824 ExecStartPre=/bin/stty speed 115200 -F $DEVICES (code=exited, status=1/FAILURE)
>>        CPU: 7ms
>>
>>4月 09 16:04:16 jbd systemd[1]: Starting GPS (Global Positioning System) Daemon...
>>4月 09 16:04:17 jbd stty[3824]: /bin/stty: /dev/gnss0: Inappropriate ioctl for device
>>4月 09 16:04:17 jbd systemd[1]: gpsd.service: Control process exited, code=exited, status=1/FAILURE
>>4月 09 16:04:17 jbd systemd[1]: gpsd.service: Failed with result 'exit-code'.
>>4月 09 16:04:17 jbd systemd[1]: Failed to start GPS (Global Positioning System) Daemon.
>>
>>Seems it's not a serial port.
>
>It is a char dev lacking some IOCTLs support. Yeah.
>
>>Any advice?
>
>Yep. Remove that stty invocation from the service definition. For me, gpsd works flawlessly. You can try to start it manually from a terminal.
>
>--
>Sergey
Hi Sergey,
My device could output the NMEA data by port /dev/gnss0. Something like below:

$GPRMC,071634.00,A,2239.372067,N,11402.653048,E,,,280425,,,A,V*2D
$GARMC,071634.00,A,2239.372067,N,11402.653048,E,,,280425,,,A,V*3C
$GBRMC,071634.00,A,2239.372067,N,11402.653048,E,,,280425,,,A,V*3F
$GNRMC,071634.00,A,2239.372067,N,11402.653048,E,,,280425,,,A,V*33
$GNGNS,071634.00,2239.372067,N,11402.653048,E,NAANNN,02,500.0,,,,,V*15
$GPGGA,071634.00,2239.372067,N,11402.653048,E,1,00,500.0,,,,,,*59
$GAGGA,071634.00,2239.372067,N,11402.653048,E,1,01,500.0,,,,,,*49
$GBGGA,071634.00,2239.372067,N,11402.653048,E,1,00,500.0,,,,,,*4B
$GNGGA,071634.00,2239.372067,N,11402.653048,E,1,02,500.0,,,,,,*45
$GPGSV,4,1,13,04,00,038,,05,33,240,,06,41,033,,09,25,058,,1*6F
$GPGSV,4,2,13,11,47,344,,12,33,286,,13,09,185,,17,29,128,,1*6E
$GPGSV,4,3,13,19,54,113,,20,62,284,,22,15,174,,25,09,311,,1*68
$GPGSV,4,4,13,40,00,000,28,1*58
$GLGSV,3,1,09,10,19,245,,06,38,185,,09,06,203,,11,13,296,,1*79
$GLGSV,3,2,09,05,60,064,,20,39,013,,19,17,084,,21,15,321,,1*7E
$GLGSV,3,3,09,04,16,030,,1*41
$GAGSV,3,1,11,02,30,297,,04,32,076,,05,10,188,,06,41,107,,7*78
$GAGSV,3,2,11,09,39,140,,10,26,055,,11,42,027,,12,09,071,,7*7E
$GAGSV,3,3,11,16,36,198,,24,19,176,,36,39,317,,7*45
$GBGSV,4,1,15,01,47,122,,02,46,234,,03,63,189,,04,34,108,,1*7D
$GBGSV,4,2,15,05,23,253,,06,04,187,,07,86,194,,08,68,284,,1*75
$GBGSV,4,3,15,09,03,201,,10,78,299,,11,56,025,,12,29,094,,1*71

But the gpsd progress were stuck with below errors:
● gpsd.service - GPS (Global Positioning System) Daemon
     Loaded: loaded (/lib/systemd/system/gpsd.service; enabled; vendor preset: enabled)
     Active: active (running) since Mon 2025-04-28 23:16:47 CST; 20s ago
TriggeredBy: ● gpsd.socket
    Process: 5281 ExecStart=/usr/sbin/gpsd $GPSD_OPTIONS $OPTIONS $DEVICES (code=exited, status=0/SUCCESS)
   Main PID: 5283 (gpsd)
      Tasks: 1 (limit: 37272)
     Memory: 652.0K
        CPU: 10ms
     CGroup: /system.slice/gpsd.service
             └─5283 /usr/sbin/gpsd -F /var/run/gpsd.sock /dev/gnss0

4月 28 23:16:47 jbd systemd[1]: Starting GPS (Global Positioning System) Daemon...
4月 28 23:16:47 jbd systemd[1]: Started GPS (Global Positioning System) Daemon.
4月 28 23:17:02 jbd gpsd[5283]: gpsd:ERROR: SER: device open of /dev/gnss0 failed: Permission denied - retrying read-only
4月 28 23:17:02 jbd gpsd[5283]: gpsd:ERROR: SER: read-only device open of /dev/gnss0 failed: Permission denied
4月 28 23:17:02 jbd gpsd[5283]: gpsd:ERROR: /dev/gnss0: device activation failed, freeing device.

And I checked the gnss device attribute, it reports:
crw------- 1 root root 237, 0  4月 28  2025 /dev/gnss0

May I know how do you fix this?

Thanks

Re: [RFC PATCH 4/6] net: wwan: add NMEA port support

[Muhammad Nuzaihan Kamal Luddin]

Hi Slark,

you cab add udev rules to set the permissions.

Regards,
Zaihan
Sent from my iPhone

> On 28 Apr 2025, at 4:28 PM, Slark Xiao <slark_xiao@163.com> wrote:
> 
> 
> At 2025-04-09 18:42:59, "Sergey Ryazanov" <ryazanov.s.a@gmail.com> wrote:
>>> On April 9, 2025 11:30:58 AM GMT+03:00, Slark Xiao <slark_xiao@163.com> wrote:
>>> 
>>> Hi Sergey,
>>> Device port /dev/gnss0 is enumerated . Does it be expected?
>>> I can get the NMEA data from this port by cat or minicom command.
>>> But the gpsd.service also can not be initialized normally. It reports:
>>> 
>>> TriggeredBy: ● gpsd.socket
>>>   Process: 3824 ExecStartPre=/bin/stty speed 115200 -F $DEVICES (code=exited, status=1/FAILURE)
>>>       CPU: 7ms
>>> 
>>> 4月 09 16:04:16 jbd systemd[1]: Starting GPS (Global Positioning System) Daemon...
>>> 4月 09 16:04:17 jbd stty[3824]: /bin/stty: /dev/gnss0: Inappropriate ioctl for device
>>> 4月 09 16:04:17 jbd systemd[1]: gpsd.service: Control process exited, code=exited, status=1/FAILURE
>>> 4月 09 16:04:17 jbd systemd[1]: gpsd.service: Failed with result 'exit-code'.
>>> 4月 09 16:04:17 jbd systemd[1]: Failed to start GPS (Global Positioning System) Daemon.
>>> 
>>> Seems it's not a serial port.
>> 
>> It is a char dev lacking some IOCTLs support. Yeah.
>> 
>>> Any advice?
>> 
>> Yep. Remove that stty invocation from the service definition. For me, gpsd works flawlessly. You can try to start it manually from a terminal.
>> 
>> --
>> Sergey
> Hi Sergey,
> My device could output the NMEA data by port /dev/gnss0. Something like below:
> 
> $GPRMC,071634.00,A,2239.372067,N,11402.653048,E,,,280425,,,A,V*2D
> $GARMC,071634.00,A,2239.372067,N,11402.653048,E,,,280425,,,A,V*3C
> $GBRMC,071634.00,A,2239.372067,N,11402.653048,E,,,280425,,,A,V*3F
> $GNRMC,071634.00,A,2239.372067,N,11402.653048,E,,,280425,,,A,V*33
> $GNGNS,071634.00,2239.372067,N,11402.653048,E,NAANNN,02,500.0,,,,,V*15
> $GPGGA,071634.00,2239.372067,N,11402.653048,E,1,00,500.0,,,,,,*59
> $GAGGA,071634.00,2239.372067,N,11402.653048,E,1,01,500.0,,,,,,*49
> $GBGGA,071634.00,2239.372067,N,11402.653048,E,1,00,500.0,,,,,,*4B
> $GNGGA,071634.00,2239.372067,N,11402.653048,E,1,02,500.0,,,,,,*45
> $GPGSV,4,1,13,04,00,038,,05,33,240,,06,41,033,,09,25,058,,1*6F
> $GPGSV,4,2,13,11,47,344,,12,33,286,,13,09,185,,17,29,128,,1*6E
> $GPGSV,4,3,13,19,54,113,,20,62,284,,22,15,174,,25,09,311,,1*68
> $GPGSV,4,4,13,40,00,000,28,1*58
> $GLGSV,3,1,09,10,19,245,,06,38,185,,09,06,203,,11,13,296,,1*79
> $GLGSV,3,2,09,05,60,064,,20,39,013,,19,17,084,,21,15,321,,1*7E
> $GLGSV,3,3,09,04,16,030,,1*41
> $GAGSV,3,1,11,02,30,297,,04,32,076,,05,10,188,,06,41,107,,7*78
> $GAGSV,3,2,11,09,39,140,,10,26,055,,11,42,027,,12,09,071,,7*7E
> $GAGSV,3,3,11,16,36,198,,24,19,176,,36,39,317,,7*45
> $GBGSV,4,1,15,01,47,122,,02,46,234,,03,63,189,,04,34,108,,1*7D
> $GBGSV,4,2,15,05,23,253,,06,04,187,,07,86,194,,08,68,284,,1*75
> $GBGSV,4,3,15,09,03,201,,10,78,299,,11,56,025,,12,29,094,,1*71
> 
> But the gpsd progress were stuck with below errors:
> ● gpsd.service - GPS (Global Positioning System) Daemon
>     Loaded: loaded (/lib/systemd/system/gpsd.service; enabled; vendor preset: enabled)
>     Active: active (running) since Mon 2025-04-28 23:16:47 CST; 20s ago
> TriggeredBy: ● gpsd.socket
>    Process: 5281 ExecStart=/usr/sbin/gpsd $GPSD_OPTIONS $OPTIONS $DEVICES (code=exited, status=0/SUCCESS)
>   Main PID: 5283 (gpsd)
>      Tasks: 1 (limit: 37272)
>     Memory: 652.0K
>        CPU: 10ms
>     CGroup: /system.slice/gpsd.service
>             └─5283 /usr/sbin/gpsd -F /var/run/gpsd.sock /dev/gnss0
> 
> 4月 28 23:16:47 jbd systemd[1]: Starting GPS (Global Positioning System) Daemon...
> 4月 28 23:16:47 jbd systemd[1]: Started GPS (Global Positioning System) Daemon.
> 4月 28 23:17:02 jbd gpsd[5283]: gpsd:ERROR: SER: device open of /dev/gnss0 failed: Permission denied - retrying read-only
> 4月 28 23:17:02 jbd gpsd[5283]: gpsd:ERROR: SER: read-only device open of /dev/gnss0 failed: Permission denied
> 4月 28 23:17:02 jbd gpsd[5283]: gpsd:ERROR: /dev/gnss0: device activation failed, freeing device.
> 
> And I checked the gnss device attribute, it reports:
> crw------- 1 root root 237, 0  4月 28  2025 /dev/gnss0
> 
> May I know how do you fix this?
> 
> Thanks
> 
> 

Re:Re: [RFC PATCH 4/6] net: wwan: add NMEA port support

[Slark Xiao]

At 2025-04-28 21:23:53, "Muhammad Nuzaihan Kamal Luddin" <zaihan@unrealasia.net> wrote:
>Hi Slark,
>
>you cab add udev rules to set the permissions.
>
>Regards,
>Zaihan
>Sent from my iPhone
>
Hi Zaihan,
Thanks for your advice.
But I tried in my local, no matter the mode is 644 or 755, similar issue still exist.
BTW, this error only was reported when I call "gpsmon" or "cgps" command.

test reference as below:
jbd@jbd:~$ cat /etc/udev/rules.d/99-gps.rules 
KERNEL=="gnss0", MODE="0755", GROUP="dialout"

4月 29 14:08:17 jbd gpsd[3544]: gpsd:ERROR: SER: device open of /dev/gnss0 failed: Permission denied - retrying read-only
4月 29 14:08:17 jbd gpsd[3544]: gpsd:ERROR: SER: read-only device open of /dev/gnss0 failed: Permission denied
4月 29 14:08:17 jbd gpsd[3544]: gpsd:ERROR: /dev/gnss0: device activation failed, freeing device.
jbd@jbd:~$ 
jbd@jbd:~$ ls -l /dev/gnss0 
crwxr-xr-x 1 root dialout 237, 0  4月 29 14:05 /dev/gnss0

Thanks
>> On 28 Apr 2025, at 4:28 PM, Slark Xiao <slark_xiao@163.com> wrote:
>> 
>> 
>> At 2025-04-09 18:42:59, "Sergey Ryazanov" <ryazanov.s.a@gmail.com> wrote:
>>>> On April 9, 2025 11:30:58 AM GMT+03:00, Slark Xiao <slark_xiao@163.com> wrote:
>>>> 
>>>> Hi Sergey,
>>>> Device port /dev/gnss0 is enumerated . Does it be expected?
>>>> I can get the NMEA data from this port by cat or minicom command.
>>>> But the gpsd.service also can not be initialized normally. It reports:
>>>> 
>>>> TriggeredBy: ● gpsd.socket
>>>>   Process: 3824 ExecStartPre=/bin/stty speed 115200 -F $DEVICES (code=exited, status=1/FAILURE)
>>>>       CPU: 7ms
>>>> 
>>>> 4月 09 16:04:16 jbd systemd[1]: Starting GPS (Global Positioning System) Daemon...
>>>> 4月 09 16:04:17 jbd stty[3824]: /bin/stty: /dev/gnss0: Inappropriate ioctl for device
>>>> 4月 09 16:04:17 jbd systemd[1]: gpsd.service: Control process exited, code=exited, status=1/FAILURE
>>>> 4月 09 16:04:17 jbd systemd[1]: gpsd.service: Failed with result 'exit-code'.
>>>> 4月 09 16:04:17 jbd systemd[1]: Failed to start GPS (Global Positioning System) Daemon.
>>>> 
>>>> Seems it's not a serial port.
>>> 
>>> It is a char dev lacking some IOCTLs support. Yeah.
>>> 
>>>> Any advice?
>>> 
>>> Yep. Remove that stty invocation from the service definition. For me, gpsd works flawlessly. You can try to start it manually from a terminal.
>>> 
>>> --
>>> Sergey
>> Hi Sergey,
>> My device could output the NMEA data by port /dev/gnss0. Something like below:
>> 
>> $GPRMC,071634.00,A,2239.372067,N,11402.653048,E,,,280425,,,A,V*2D
>> $GARMC,071634.00,A,2239.372067,N,11402.653048,E,,,280425,,,A,V*3C
>> $GBRMC,071634.00,A,2239.372067,N,11402.653048,E,,,280425,,,A,V*3F
>> $GNRMC,071634.00,A,2239.372067,N,11402.653048,E,,,280425,,,A,V*33
>> $GNGNS,071634.00,2239.372067,N,11402.653048,E,NAANNN,02,500.0,,,,,V*15
>> $GPGGA,071634.00,2239.372067,N,11402.653048,E,1,00,500.0,,,,,,*59
>> $GAGGA,071634.00,2239.372067,N,11402.653048,E,1,01,500.0,,,,,,*49
>> $GBGGA,071634.00,2239.372067,N,11402.653048,E,1,00,500.0,,,,,,*4B
>> $GNGGA,071634.00,2239.372067,N,11402.653048,E,1,02,500.0,,,,,,*45
>> $GPGSV,4,1,13,04,00,038,,05,33,240,,06,41,033,,09,25,058,,1*6F
>> $GPGSV,4,2,13,11,47,344,,12,33,286,,13,09,185,,17,29,128,,1*6E
>> $GPGSV,4,3,13,19,54,113,,20,62,284,,22,15,174,,25,09,311,,1*68
>> $GPGSV,4,4,13,40,00,000,28,1*58
>> $GLGSV,3,1,09,10,19,245,,06,38,185,,09,06,203,,11,13,296,,1*79
>> $GLGSV,3,2,09,05,60,064,,20,39,013,,19,17,084,,21,15,321,,1*7E
>> $GLGSV,3,3,09,04,16,030,,1*41
>> $GAGSV,3,1,11,02,30,297,,04,32,076,,05,10,188,,06,41,107,,7*78
>> $GAGSV,3,2,11,09,39,140,,10,26,055,,11,42,027,,12,09,071,,7*7E
>> $GAGSV,3,3,11,16,36,198,,24,19,176,,36,39,317,,7*45
>> $GBGSV,4,1,15,01,47,122,,02,46,234,,03,63,189,,04,34,108,,1*7D
>> $GBGSV,4,2,15,05,23,253,,06,04,187,,07,86,194,,08,68,284,,1*75
>> $GBGSV,4,3,15,09,03,201,,10,78,299,,11,56,025,,12,29,094,,1*71
>> 
>> But the gpsd progress were stuck with below errors:
>> ● gpsd.service - GPS (Global Positioning System) Daemon
>>     Loaded: loaded (/lib/systemd/system/gpsd.service; enabled; vendor preset: enabled)
>>     Active: active (running) since Mon 2025-04-28 23:16:47 CST; 20s ago
>> TriggeredBy: ● gpsd.socket
>>    Process: 5281 ExecStart=/usr/sbin/gpsd $GPSD_OPTIONS $OPTIONS $DEVICES (code=exited, status=0/SUCCESS)
>>   Main PID: 5283 (gpsd)
>>      Tasks: 1 (limit: 37272)
>>     Memory: 652.0K
>>        CPU: 10ms
>>     CGroup: /system.slice/gpsd.service
>>             └─5283 /usr/sbin/gpsd -F /var/run/gpsd.sock /dev/gnss0
>> 
>> 4月 28 23:16:47 jbd systemd[1]: Starting GPS (Global Positioning System) Daemon...
>> 4月 28 23:16:47 jbd systemd[1]: Started GPS (Global Positioning System) Daemon.
>> 4月 28 23:17:02 jbd gpsd[5283]: gpsd:ERROR: SER: device open of /dev/gnss0 failed: Permission denied - retrying read-only
>> 4月 28 23:17:02 jbd gpsd[5283]: gpsd:ERROR: SER: read-only device open of /dev/gnss0 failed: Permission denied
>> 4月 28 23:17:02 jbd gpsd[5283]: gpsd:ERROR: /dev/gnss0: device activation failed, freeing device.
>> 
>> And I checked the gnss device attribute, it reports:
>> crw------- 1 root root 237, 0  4月 28  2025 /dev/gnss0
>> 
>> May I know how do you fix this?
>> 
>> Thanks
>> 
>> 

Re:Re:Re: [RFC PATCH 4/6] net: wwan: add NMEA port support

[Slark Xiao]

Hi Sergey,
Do we have any updates about this feature? I didn't see any new patch or review suggestion.
Seems this task is pending without any push.

Thanks
Slark
At 2025-04-29 14:34:04, "Slark Xiao" <slark_xiao@163.com> wrote:
>
>
>At 2025-04-28 21:23:53, "Muhammad Nuzaihan Kamal Luddin" <zaihan@unrealasia.net> wrote:
>>Hi Slark,
>>
>>you cab add udev rules to set the permissions.
>>
>>Regards,
>>Zaihan
>>Sent from my iPhone
>>
>Hi Zaihan,
>Thanks for your advice.
>But I tried in my local, no matter the mode is 644 or 755, similar issue still exist.
>BTW, this error only was reported when I call "gpsmon" or "cgps" command.
>
>test reference as below:
>jbd@jbd:~$ cat /etc/udev/rules.d/99-gps.rules 
>KERNEL=="gnss0", MODE="0755", GROUP="dialout"
>
>4月 29 14:08:17 jbd gpsd[3544]: gpsd:ERROR: SER: device open of /dev/gnss0 failed: Permission denied - retrying read-only
>4月 29 14:08:17 jbd gpsd[3544]: gpsd:ERROR: SER: read-only device open of /dev/gnss0 failed: Permission denied
>4月 29 14:08:17 jbd gpsd[3544]: gpsd:ERROR: /dev/gnss0: device activation failed, freeing device.
>jbd@jbd:~$ 
>jbd@jbd:~$ ls -l /dev/gnss0 
>crwxr-xr-x 1 root dialout 237, 0  4月 29 14:05 /dev/gnss0
>
>Thanks
>>> On 28 Apr 2025, at 4:28 PM, Slark Xiao <slark_xiao@163.com> wrote:
>>> 
>>> 
>>> At 2025-04-09 18:42:59, "Sergey Ryazanov" <ryazanov.s.a@gmail.com> wrote:
>>>>> On April 9, 2025 11:30:58 AM GMT+03:00, Slark Xiao <slark_xiao@163.com> wrote:
>>>>> 
>>>>> Hi Sergey,
>>>>> Device port /dev/gnss0 is enumerated . Does it be expected?
>>>>> I can get the NMEA data from this port by cat or minicom command.
>>>>> But the gpsd.service also can not be initialized normally. It reports:
>>>>> 
>>>>> TriggeredBy: ● gpsd.socket
>>>>>   Process: 3824 ExecStartPre=/bin/stty speed 115200 -F $DEVICES (code=exited, status=1/FAILURE)
>>>>>       CPU: 7ms
>>>>> 
>>>>> 4月 09 16:04:16 jbd systemd[1]: Starting GPS (Global Positioning System) Daemon...
>>>>> 4月 09 16:04:17 jbd stty[3824]: /bin/stty: /dev/gnss0: Inappropriate ioctl for device
>>>>> 4月 09 16:04:17 jbd systemd[1]: gpsd.service: Control process exited, code=exited, status=1/FAILURE
>>>>> 4月 09 16:04:17 jbd systemd[1]: gpsd.service: Failed with result 'exit-code'.
>>>>> 4月 09 16:04:17 jbd systemd[1]: Failed to start GPS (Global Positioning System) Daemon.
>>>>> 
>>>>> Seems it's not a serial port.
>>>> 
>>>> It is a char dev lacking some IOCTLs support. Yeah.
>>>> 
>>>>> Any advice?
>>>> 
>>>> Yep. Remove that stty invocation from the service definition. For me, gpsd works flawlessly. You can try to start it manually from a terminal.
>>>> 
>>>> --
>>>> Sergey
>>> Hi Sergey,
>>> My device could output the NMEA data by port /dev/gnss0. Something like below:
>>> 
>>> $GPRMC,071634.00,A,2239.372067,N,11402.653048,E,,,280425,,,A,V*2D
>>> $GARMC,071634.00,A,2239.372067,N,11402.653048,E,,,280425,,,A,V*3C
>>> $GBRMC,071634.00,A,2239.372067,N,11402.653048,E,,,280425,,,A,V*3F
>>> $GNRMC,071634.00,A,2239.372067,N,11402.653048,E,,,280425,,,A,V*33
>>> $GNGNS,071634.00,2239.372067,N,11402.653048,E,NAANNN,02,500.0,,,,,V*15
>>> $GPGGA,071634.00,2239.372067,N,11402.653048,E,1,00,500.0,,,,,,*59
>>> $GAGGA,071634.00,2239.372067,N,11402.653048,E,1,01,500.0,,,,,,*49
>>> $GBGGA,071634.00,2239.372067,N,11402.653048,E,1,00,500.0,,,,,,*4B
>>> $GNGGA,071634.00,2239.372067,N,11402.653048,E,1,02,500.0,,,,,,*45
>>> $GPGSV,4,1,13,04,00,038,,05,33,240,,06,41,033,,09,25,058,,1*6F
>>> $GPGSV,4,2,13,11,47,344,,12,33,286,,13,09,185,,17,29,128,,1*6E
>>> $GPGSV,4,3,13,19,54,113,,20,62,284,,22,15,174,,25,09,311,,1*68
>>> $GPGSV,4,4,13,40,00,000,28,1*58
>>> $GLGSV,3,1,09,10,19,245,,06,38,185,,09,06,203,,11,13,296,,1*79
>>> $GLGSV,3,2,09,05,60,064,,20,39,013,,19,17,084,,21,15,321,,1*7E
>>> $GLGSV,3,3,09,04,16,030,,1*41
>>> $GAGSV,3,1,11,02,30,297,,04,32,076,,05,10,188,,06,41,107,,7*78
>>> $GAGSV,3,2,11,09,39,140,,10,26,055,,11,42,027,,12,09,071,,7*7E
>>> $GAGSV,3,3,11,16,36,198,,24,19,176,,36,39,317,,7*45
>>> $GBGSV,4,1,15,01,47,122,,02,46,234,,03,63,189,,04,34,108,,1*7D
>>> $GBGSV,4,2,15,05,23,253,,06,04,187,,07,86,194,,08,68,284,,1*75
>>> $GBGSV,4,3,15,09,03,201,,10,78,299,,11,56,025,,12,29,094,,1*71
>>> 
>>> But the gpsd progress were stuck with below errors:
>>> ● gpsd.service - GPS (Global Positioning System) Daemon
>>>     Loaded: loaded (/lib/systemd/system/gpsd.service; enabled; vendor preset: enabled)
>>>     Active: active (running) since Mon 2025-04-28 23:16:47 CST; 20s ago
>>> TriggeredBy: ● gpsd.socket
>>>    Process: 5281 ExecStart=/usr/sbin/gpsd $GPSD_OPTIONS $OPTIONS $DEVICES (code=exited, status=0/SUCCESS)
>>>   Main PID: 5283 (gpsd)
>>>      Tasks: 1 (limit: 37272)
>>>     Memory: 652.0K
>>>        CPU: 10ms
>>>     CGroup: /system.slice/gpsd.service
>>>             └─5283 /usr/sbin/gpsd -F /var/run/gpsd.sock /dev/gnss0
>>> 
>>> 4月 28 23:16:47 jbd systemd[1]: Starting GPS (Global Positioning System) Daemon...
>>> 4月 28 23:16:47 jbd systemd[1]: Started GPS (Global Positioning System) Daemon.
>>> 4月 28 23:17:02 jbd gpsd[5283]: gpsd:ERROR: SER: device open of /dev/gnss0 failed: Permission denied - retrying read-only
>>> 4月 28 23:17:02 jbd gpsd[5283]: gpsd:ERROR: SER: read-only device open of /dev/gnss0 failed: Permission denied
>>> 4月 28 23:17:02 jbd gpsd[5283]: gpsd:ERROR: /dev/gnss0: device activation failed, freeing device.
>>> 
>>> And I checked the gnss device attribute, it reports:
>>> crw------- 1 root root 237, 0  4月 28  2025 /dev/gnss0
>>> 
>>> May I know how do you fix this?
>>> 
>>> Thanks
>>> 
>>> 

Re: [RFC PATCH 4/6] net: wwan: add NMEA port support

[Sergey Ryazanov]

Hi Slark,

On 6/13/25 12:23, Slark Xiao wrote:
> Hi Sergey,
> Do we have any updates about this feature? I didn't see any new patch or review suggestion.
> Seems this task is pending without any push.

You are right. I've missed this case. Let me finish the updated patch 
development as suggested by Loic. Will send in couple of days, I hope.

I'll send it as RFCv2. If it will be Ok with your driver changes, than 
fill free to (re-)send it as a complete series: NMEA port support in 
WWAN core + your patches for the modem driver.

--
Sergey