From: Yassine Oudjana <y.oudjana@protonmail.com>

Implement a QRTR bus to allow for creating drivers for individual QRTR
services. With this in place, devices are dynamically registered for QRTR
services as they become available, and drivers for these devices are
matched using service and instance IDs.

Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
---
 include/linux/mod_devicetable.h   |   9 ++
 include/linux/soc/qcom/qrtr.h     |  36 +++++++
 net/qrtr/af_qrtr.c                |  23 +++-
 net/qrtr/qrtr.h                   |   3 +
 net/qrtr/smd.c                    | 218 +++++++++++++++++++++++++++++++++++++-
 scripts/mod/devicetable-offsets.c |   4 +
 scripts/mod/file2alias.c          |  10 ++
 7 files changed, 297 insertions(+), 6 deletions(-)

diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 6077972e8b45de3d07881c0226459d815dd1f83d..23c6a1a4bca54f871c78765f8d5401cca5c1e6eb 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -549,6 +549,15 @@ struct spmi_device_id {
 	kernel_ulong_t driver_data;	/* Data private to the driver */
 };
 
+#define QRTR_NAME_SIZE	32
+#define QRTR_MODULE_PREFIX "qrtr:"
+
+struct qrtr_device_id {
+	__u16 service;
+	__u16 instance;
+	kernel_ulong_t driver_data;	/* Data private to the driver */
+};
+
 /* dmi */
 enum dmi_field {
 	DMI_NONE,
diff --git a/include/linux/soc/qcom/qrtr.h b/include/linux/soc/qcom/qrtr.h
new file mode 100644
index 0000000000000000000000000000000000000000..e9249d9422b8ca96baa43073cf07c4a75c163219
--- /dev/null
+++ b/include/linux/soc/qcom/qrtr.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __QCOM_QRTR_H__
+#define __QCOM_QRTR_H__
+
+#include <linux/mod_devicetable.h>
+
+struct qrtr_device {
+	struct device dev;
+	unsigned int node;
+	unsigned int port;
+	u16 service;
+	u16 instance;
+};
+
+#define to_qrtr_device(d) container_of(d, struct qrtr_device, dev)
+
+struct qrtr_driver {
+	int (*probe)(struct qrtr_device *qdev);
+	void (*remove)(struct qrtr_device *qdev);
+	const struct qrtr_device_id *id_table;
+	struct device_driver driver;
+};
+
+#define to_qrtr_driver(d) container_of(d, struct qrtr_driver, driver)
+
+#define qrtr_driver_register(drv) __qrtr_driver_register(drv, THIS_MODULE)
+
+int __qrtr_driver_register(struct qrtr_driver *drv, struct module *owner);
+void qrtr_driver_unregister(struct qrtr_driver *drv);
+
+#define module_qrtr_driver(__qrtr_driver) \
+	module_driver(__qrtr_driver, qrtr_driver_register, \
+			qrtr_driver_unregister)
+
+#endif /* __QCOM_QRTR_H__ */
diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
index 00c51cf693f3d054f1771de5246498bf013775d0..e11682fd796083a9866527824faf428affba19bc 100644
--- a/net/qrtr/af_qrtr.c
+++ b/net/qrtr/af_qrtr.c
@@ -435,6 +435,7 @@ static void qrtr_node_assign(struct qrtr_node *node, unsigned int nid)
 int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
 {
 	struct qrtr_node *node = ep->node;
+	const struct qrtr_ctrl_pkt *pkt;
 	const struct qrtr_hdr_v1 *v1;
 	const struct qrtr_hdr_v2 *v2;
 	struct qrtr_sock *ipc;
@@ -443,6 +444,7 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
 	size_t size;
 	unsigned int ver;
 	size_t hdrlen;
+	int ret = 0;
 
 	if (len == 0 || len & 3)
 		return -EINVAL;
@@ -516,12 +518,24 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
 
 	qrtr_node_assign(node, cb->src_node);
 
+	pkt = data + hdrlen;
+
 	if (cb->type == QRTR_TYPE_NEW_SERVER) {
 		/* Remote node endpoint can bridge other distant nodes */
-		const struct qrtr_ctrl_pkt *pkt;
-
-		pkt = data + hdrlen;
 		qrtr_node_assign(node, le32_to_cpu(pkt->server.node));
+
+		/* Create a QRTR device */
+		ret = ep->add_device(ep, le32_to_cpu(pkt->server.node),
+					       le32_to_cpu(pkt->server.port),
+					       le32_to_cpu(pkt->server.service),
+					       le32_to_cpu(pkt->server.instance));
+		if (ret)
+			goto err;
+	} else if (cb->type == QRTR_TYPE_DEL_SERVER) {
+		/* Remove QRTR device corresponding to service */
+		ret = ep->del_device(ep, le32_to_cpu(pkt->server.port));
+		if (ret)
+			goto err;
 	}
 
 	if (cb->type == QRTR_TYPE_RESUME_TX) {
@@ -543,8 +557,7 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
 
 err:
 	kfree_skb(skb);
-	return -EINVAL;
-
+	return ret ? ret : -EINVAL;
 }
 EXPORT_SYMBOL_GPL(qrtr_endpoint_post);
 
diff --git a/net/qrtr/qrtr.h b/net/qrtr/qrtr.h
index 3f2d28696062a56201f8774ee50fd1c3daa50708..6590483367304b97239de939d4f0206aac52c527 100644
--- a/net/qrtr/qrtr.h
+++ b/net/qrtr/qrtr.h
@@ -19,6 +19,9 @@ struct sk_buff;
  */
 struct qrtr_endpoint {
 	int (*xmit)(struct qrtr_endpoint *ep, struct sk_buff *skb);
+	int (*add_device)(struct qrtr_endpoint *parent, unsigned int node, unsigned int port,
+			  u16 service, u16 instance);
+	int (*del_device)(struct qrtr_endpoint *parent, unsigned int port);
 	/* private: not for endpoint use */
 	struct qrtr_node *node;
 };
diff --git a/net/qrtr/smd.c b/net/qrtr/smd.c
index 940ee9349a9ce7438a01dd193c5c1d61c7b82ffd..40fd32fa0890799a88b947b2b7bc00c2249dea0d 100644
--- a/net/qrtr/smd.c
+++ b/net/qrtr/smd.c
@@ -7,6 +7,7 @@
 #include <linux/module.h>
 #include <linux/skbuff.h>
 #include <linux/rpmsg.h>
+#include <linux/soc/qcom/qrtr.h>
 
 #include "qrtr.h"
 
@@ -16,6 +17,197 @@ struct qrtr_smd_dev {
 	struct device *dev;
 };
 
+struct qrtr_new_server {
+	struct qrtr_smd_dev *parent;
+	unsigned int node;
+	unsigned int port;
+	u16 service;
+	u16 instance;
+
+	struct work_struct work;
+};
+
+struct qrtr_del_server {
+	struct qrtr_smd_dev *parent;
+	unsigned int port;
+
+	struct work_struct work;
+};
+
+static int qcom_smd_qrtr_device_match(struct device *dev, const struct device_driver *drv)
+{
+	struct qrtr_device *qdev = to_qrtr_device(dev);
+	struct qrtr_driver *qdrv = to_qrtr_driver(drv);
+	const struct qrtr_device_id *id = qdrv->id_table;
+
+	if (!id)
+		return 0;
+
+	while (id->service != 0) {
+		if (id->service == qdev->service && id->instance == qdev->instance)
+			return 1;
+		id++;
+	}
+
+	return 0;
+}
+
+static int qcom_smd_qrtr_uevent(const struct device *dev, struct kobj_uevent_env *env)
+{
+	const struct qrtr_device *qdev = to_qrtr_device(dev);
+
+	return add_uevent_var(env, "MODALIAS=%s%x:%x", QRTR_MODULE_PREFIX, qdev->service,
+			      qdev->instance);
+}
+
+static int qcom_smd_qrtr_device_probe(struct device *dev)
+{
+	struct qrtr_device *qdev = to_qrtr_device(dev);
+	struct qrtr_driver *qdrv = to_qrtr_driver(dev->driver);
+
+	return qdrv->probe(qdev);
+}
+
+static void qcom_smd_qrtr_device_remove(struct device *dev)
+{
+	struct qrtr_device *qdev = to_qrtr_device(dev);
+	struct qrtr_driver *qdrv = to_qrtr_driver(dev->driver);
+
+	if (qdrv->remove)
+		qdrv->remove(qdev);
+}
+
+const struct bus_type qrtr_bus = {
+	.name		= "qrtr",
+	.match		= qcom_smd_qrtr_device_match,
+	.uevent		= qcom_smd_qrtr_uevent,
+	.probe		= qcom_smd_qrtr_device_probe,
+	.remove		= qcom_smd_qrtr_device_remove,
+};
+EXPORT_SYMBOL_NS_GPL(qrtr_bus, "QRTR");
+
+int __qrtr_driver_register(struct qrtr_driver *drv, struct module *owner)
+{
+	drv->driver.bus = &qrtr_bus;
+	drv->driver.owner = owner;
+
+	return driver_register(&drv->driver);
+}
+EXPORT_SYMBOL_NS_GPL(__qrtr_driver_register, "QRTR");
+
+void qrtr_driver_unregister(struct qrtr_driver *drv)
+{
+	driver_unregister(&drv->driver);
+}
+EXPORT_SYMBOL_NS_GPL(qrtr_driver_unregister, "QRTR");
+
+static void qcom_smd_qrtr_dev_release(struct device *dev)
+{
+	struct qrtr_device *qdev = to_qrtr_device(dev);
+
+	kfree(qdev);
+}
+
+static int qcom_smd_qrtr_match_device_by_port(struct device *dev, const void *data)
+{
+	struct qrtr_device *qdev = to_qrtr_device(dev);
+	unsigned const int *port = data;
+
+	return qdev->port == *port;
+}
+
+static void qcom_smd_qrtr_add_device_worker(struct work_struct *work)
+{
+	struct qrtr_new_server *new_server = container_of(work, struct qrtr_new_server, work);
+	struct qrtr_smd_dev *qsdev = new_server->parent;
+	struct qrtr_device *qdev;
+	int ret;
+
+	qdev = kzalloc(sizeof(*qdev), GFP_KERNEL);
+	if (!qdev)
+		return;
+
+	*qdev = (struct qrtr_device) {
+		.node = new_server->node,
+		.port = new_server->port,
+		.service = new_server->service,
+		.instance = new_server->instance
+	};
+
+	devm_kfree(qsdev->dev, new_server);
+
+	dev_set_name(&qdev->dev, "%d-%d", qdev->node, qdev->port);
+
+	qdev->dev.bus = &qrtr_bus;
+	qdev->dev.parent = qsdev->dev;
+	qdev->dev.release = qcom_smd_qrtr_dev_release;
+
+	ret = device_register(&qdev->dev);
+	if (ret) {
+		dev_err(qsdev->dev, "Failed to register QRTR device: %pe\n", ERR_PTR(ret));
+		put_device(&qdev->dev);
+	}
+}
+
+static void qcom_smd_qrtr_del_device_worker(struct work_struct *work)
+{
+	struct qrtr_del_server *del_server = container_of(work, struct qrtr_del_server, work);
+	struct qrtr_smd_dev *qsdev = del_server->parent;
+	struct device *dev = device_find_child(qsdev->dev, &del_server->port,
+					       qcom_smd_qrtr_match_device_by_port);
+
+	device_unregister(dev);
+}
+
+static int qcom_smd_qrtr_add_device(struct qrtr_endpoint *parent, unsigned int node,
+				    unsigned int port, u16 service, u16 instance)
+{
+	struct qrtr_smd_dev *qsdev = container_of(parent, struct qrtr_smd_dev, ep);
+	struct qrtr_new_server *new_server;
+
+	new_server = devm_kzalloc(qsdev->dev, sizeof(*new_server), GFP_KERNEL);
+	if (!new_server)
+		return -ENOMEM;
+
+	*new_server = (struct qrtr_new_server) {
+		.parent = qsdev,
+		.node = node,
+		.port = port,
+		.service = service,
+		.instance = instance
+	};
+
+	INIT_WORK(&new_server->work, qcom_smd_qrtr_add_device_worker);
+	schedule_work(&new_server->work);
+
+	return 0;
+}
+
+static int qcom_smd_qrtr_del_device(struct qrtr_endpoint *parent, unsigned int port)
+{
+	struct qrtr_smd_dev *qsdev = container_of(parent, struct qrtr_smd_dev, ep);
+	struct qrtr_del_server *del_server;
+
+	del_server = devm_kzalloc(qsdev->dev, sizeof(*del_server), GFP_KERNEL);
+	if (!del_server)
+		return -ENOMEM;
+
+	del_server->parent = qsdev;
+	del_server->port = port;
+
+	INIT_WORK(&del_server->work, qcom_smd_qrtr_del_device_worker);
+	schedule_work(&del_server->work);
+
+	return 0;
+}
+
+static int qcom_smd_qrtr_device_unregister(struct device *dev, void *data)
+{
+	device_unregister(dev);
+
+	return 0;
+}
+
 /* from smd to qrtr */
 static int qcom_smd_qrtr_callback(struct rpmsg_device *rpdev,
 				  void *data, int len, void *priv, u32 addr)
@@ -86,6 +278,8 @@ static void qcom_smd_qrtr_remove(struct rpmsg_device *rpdev)
 {
 	struct qrtr_smd_dev *qsdev = dev_get_drvdata(&rpdev->dev);
 
+	device_for_each_child(qsdev->dev, NULL, qcom_smd_qrtr_device_unregister);
+
 	qrtr_endpoint_unregister(&qsdev->ep);
 
 	dev_set_drvdata(&rpdev->dev, NULL);
@@ -106,7 +300,29 @@ static struct rpmsg_driver qcom_smd_qrtr_driver = {
 	},
 };
 
-module_rpmsg_driver(qcom_smd_qrtr_driver);
+static int __init qcom_smd_qrtr_init(void)
+{
+	int ret;
+
+	ret = bus_register(&qrtr_bus);
+	if (ret)
+		return ret;
+
+	ret = register_rpmsg_driver(&qcom_smd_qrtr_driver);
+	if (ret)
+		bus_unregister(&qrtr_bus);
+
+	return ret;
+}
+
+static void __exit qcom_smd_qrtr_exit(void)
+{
+	unregister_rpmsg_driver(&qcom_smd_qrtr_driver);
+	bus_unregister(&qrtr_bus);
+}
+
+subsys_initcall(qcom_smd_qrtr_init);
+module_exit(qcom_smd_qrtr_exit);
 
 MODULE_ALIAS("rpmsg:IPCRTR");
 MODULE_DESCRIPTION("Qualcomm IPC-Router SMD interface driver");
diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
index d3d00e85edf73553ba3d9b5f9fccf1ff61c99026..0a90323c35d330f13a151948467d72b927f474f3 100644
--- a/scripts/mod/devicetable-offsets.c
+++ b/scripts/mod/devicetable-offsets.c
@@ -280,5 +280,9 @@ int main(void)
 	DEVID(coreboot_device_id);
 	DEVID_FIELD(coreboot_device_id, tag);
 
+	DEVID(qrtr_device_id);
+	DEVID_FIELD(qrtr_device_id, service);
+	DEVID_FIELD(qrtr_device_id, instance);
+
 	return 0;
 }
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 00586119a25b7fd399eeeef3760a26467ffbb50c..fef69db4d7023305f03fd8bf85ac60c47ae7d0ca 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -1370,6 +1370,15 @@ static void do_coreboot_entry(struct module *mod, void *symval)
 	module_alias_printf(mod, false, "coreboot:t%08X", tag);
 }
 
+/* Looks like: qrtr:N:N */
+static void do_qrtr_entry(struct module *mod, void *symval)
+{
+	DEF_FIELD(symval, qrtr_device_id, service);
+	DEF_FIELD(symval, qrtr_device_id, instance);
+
+	module_alias_printf(mod, false, "qrtr:%x:%x", service, instance);
+}
+
 /* Does namelen bytes of name exactly match the symbol? */
 static bool sym_is(const char *name, unsigned namelen, const char *symbol)
 {
@@ -1466,6 +1475,7 @@ static const struct devtable devtable[] = {
 	{"usb", SIZE_usb_device_id, do_usb_entry_multi},
 	{"pnp", SIZE_pnp_device_id, do_pnp_device_entry},
 	{"pnp_card", SIZE_pnp_card_device_id, do_pnp_card_entry},
+	{"qrtr", SIZE_qrtr_device_id, do_qrtr_entry},
 };
 
 /* Create MODULE_ALIAS() statements.

-- 
2.50.0

On Thu, Jul 10, 2025 at 11:06 AM Yassine Oudjana via B4 Relay
<devnull+y.oudjana.protonmail.com@kernel.org> wrote:
>
> Implement a QRTR bus to allow for creating drivers for individual QRTR
> services. With this in place, devices are dynamically registered for QRTR
> services as they become available, and drivers for these devices are
> matched using service and instance IDs.

...

> +struct qrtr_device_id {
> +       __u16 service;
> +       __u16 instance;
> +       kernel_ulong_t driver_data;     /* Data private to the driver */

Can we not repeat mistakes from the past and use const void * from day 1 please?

> +};
> +
>  /* dmi */

Wouldn't it be better to keep sections ordered alphabetically so 'q'
will go at least after 'd'?

...

> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __QCOM_QRTR_H__
> +#define __QCOM_QRTR_H__
> +
> +#include <linux/mod_devicetable.h>

Not enough. Please, follow IWYU principle and include / forward
declare all this header uses.

> +struct qrtr_device {
> +       struct device dev;
> +       unsigned int node;
> +       unsigned int port;
> +       u16 service;
> +       u16 instance;
> +};
> +
> +#define to_qrtr_device(d) container_of(d, struct qrtr_device, dev)
> +
> +struct qrtr_driver {
> +       int (*probe)(struct qrtr_device *qdev);
> +       void (*remove)(struct qrtr_device *qdev);
> +       const struct qrtr_device_id *id_table;
> +       struct device_driver driver;
> +};
> +
> +#define to_qrtr_driver(d) container_of(d, struct qrtr_driver, driver)
> +
> +#define qrtr_driver_register(drv) __qrtr_driver_register(drv, THIS_MODULE)
> +
> +int __qrtr_driver_register(struct qrtr_driver *drv, struct module *owner);
> +void qrtr_driver_unregister(struct qrtr_driver *drv);
> +
> +#define module_qrtr_driver(__qrtr_driver) \
> +       module_driver(__qrtr_driver, qrtr_driver_register, \
> +                       qrtr_driver_unregister)
> +
> +#endif /* __QCOM_QRTR_H__ */

...

> +       int ret = 0;

What is this assignment for? (The below is left for the context)

>         if (cb->type == QRTR_TYPE_NEW_SERVER) {
>                 /* Remote node endpoint can bridge other distant nodes */
>                 qrtr_node_assign(node, le32_to_cpu(pkt->server.node));
> +
> +               /* Create a QRTR device */
> +               ret = ep->add_device(ep, le32_to_cpu(pkt->server.node),
> +                                              le32_to_cpu(pkt->server.port),
> +                                              le32_to_cpu(pkt->server.service),
> +                                              le32_to_cpu(pkt->server.instance));
> +               if (ret)
> +                       goto err;
> +       } else if (cb->type == QRTR_TYPE_DEL_SERVER) {
> +               /* Remove QRTR device corresponding to service */
> +               ret = ep->del_device(ep, le32_to_cpu(pkt->server.port));
> +               if (ret)
> +                       goto err;
>         }

...

> +       return ret ? ret : -EINVAL;

It's also possible

       return ret ?: -EINVAL;

>  }

...

> +++ b/net/qrtr/smd.c
> @@ -7,6 +7,7 @@
>  #include <linux/module.h>
>  #include <linux/skbuff.h>
>  #include <linux/rpmsg.h>
> +#include <linux/soc/qcom/qrtr.h>

Can we keep this more ordered?

Also include export.h when the code uses one of the EXPORT_*() macros.

...

> +static int qcom_smd_qrtr_device_match(struct device *dev, const struct device_driver *drv)
> +{
> +       struct qrtr_device *qdev = to_qrtr_device(dev);
> +       struct qrtr_driver *qdrv = to_qrtr_driver(drv);
> +       const struct qrtr_device_id *id = qdrv->id_table;
> +
> +       if (!id)
> +               return 0;
> +
> +       while (id->service != 0) {

' != 0' is redundant

> +               if (id->service == qdev->service && id->instance == qdev->instance)
> +                       return 1;
> +               id++;
> +       }
> +
> +       return 0;
> +}

...

> +static int qcom_smd_qrtr_match_device_by_port(struct device *dev, const void *data)
> +{
> +       struct qrtr_device *qdev = to_qrtr_device(dev);
> +       unsigned const int *port = data;

Why not

       unsigned int port = *((const unsigned int *)data);

> +       return qdev->port == *port;
> +}

...

> +static void qcom_smd_qrtr_add_device_worker(struct work_struct *work)
> +{
> +       struct qrtr_new_server *new_server = container_of(work, struct qrtr_new_server, work);
> +       struct qrtr_smd_dev *qsdev = new_server->parent;
> +       struct qrtr_device *qdev;
> +       int ret;
> +
> +       qdev = kzalloc(sizeof(*qdev), GFP_KERNEL);
> +       if (!qdev)
> +               return;
> +
> +       *qdev = (struct qrtr_device) {
> +               .node = new_server->node,
> +               .port = new_server->port,
> +               .service = new_server->service,
> +               .instance = new_server->instance

Leave trailing comma.

> +       };

> +       devm_kfree(qsdev->dev, new_server);

?!?! No, just no. Please, fix the object lifetimes and use proper
allocators (not managed).

> +       dev_set_name(&qdev->dev, "%d-%d", qdev->node, qdev->port);

No error check?

> +       qdev->dev.bus = &qrtr_bus;
> +       qdev->dev.parent = qsdev->dev;
> +       qdev->dev.release = qcom_smd_qrtr_dev_release;
> +
> +       ret = device_register(&qdev->dev);
> +       if (ret) {
> +               dev_err(qsdev->dev, "Failed to register QRTR device: %pe\n", ERR_PTR(ret));
> +               put_device(&qdev->dev);
> +       }
> +}

...

> +static int qcom_smd_qrtr_add_device(struct qrtr_endpoint *parent, unsigned int node,
> +                                   unsigned int port, u16 service, u16 instance)
> +{
> +       struct qrtr_smd_dev *qsdev = container_of(parent, struct qrtr_smd_dev, ep);
> +       struct qrtr_new_server *new_server;
> +
> +       new_server = devm_kzalloc(qsdev->dev, sizeof(*new_server), GFP_KERNEL);

Why is the managed API in use?!

> +       if (!new_server)
> +               return -ENOMEM;
> +
> +       *new_server = (struct qrtr_new_server) {
> +               .parent = qsdev,
> +               .node = node,
> +               .port = port,
> +               .service = service,
> +               .instance = instance

Leave trailing comma.

> +       };
> +
> +       INIT_WORK(&new_server->work, qcom_smd_qrtr_add_device_worker);
> +       schedule_work(&new_server->work);
> +
> +       return 0;
> +}
> +
> +static int qcom_smd_qrtr_del_device(struct qrtr_endpoint *parent, unsigned int port)
> +{
> +       struct qrtr_smd_dev *qsdev = container_of(parent, struct qrtr_smd_dev, ep);
> +       struct qrtr_del_server *del_server;
> +
> +       del_server = devm_kzalloc(qsdev->dev, sizeof(*del_server), GFP_KERNEL);

Ditto.

> +       if (!del_server)
> +               return -ENOMEM;
> +
> +       del_server->parent = qsdev;
> +       del_server->port = port;
> +
> +       INIT_WORK(&del_server->work, qcom_smd_qrtr_del_device_worker);
> +       schedule_work(&del_server->work);
> +
> +       return 0;
> +}

...

> +static int qcom_smd_qrtr_device_unregister(struct device *dev, void *data)
> +{
> +       device_unregister(dev);
> +
> +       return 0;

Why? Can't this function be void?

> +}

...

>  {
>         struct qrtr_smd_dev *qsdev = dev_get_drvdata(&rpdev->dev);
>
> +       device_for_each_child(qsdev->dev, NULL, qcom_smd_qrtr_device_unregister);

Perhaps _reversed() ?

>         qrtr_endpoint_unregister(&qsdev->ep);
>
>         dev_set_drvdata(&rpdev->dev, NULL);

>  };


> +static void __exit qcom_smd_qrtr_exit(void)
> +{
> +       unregister_rpmsg_driver(&qcom_smd_qrtr_driver);
> +       bus_unregister(&qrtr_bus);
> +}
> +
> +subsys_initcall(qcom_smd_qrtr_init);
> +module_exit(qcom_smd_qrtr_exit);

Move these two closer to the mentioned callbacks.

--
With Best Regards,
Andy Shevchenko

On Thursday, July 10th, 2025 at 9:53 AM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, Jul 10, 2025 at 11:06 AM Yassine Oudjana via B4 Relay
> devnull+y.oudjana.protonmail.com@kernel.org wrote:
> 
> > Implement a QRTR bus to allow for creating drivers for individual QRTR
> > services. With this in place, devices are dynamically registered for QRTR
> > services as they become available, and drivers for these devices are
> > matched using service and instance IDs.
> 
> 
> ...
> 
> > +struct qrtr_device_id {
> > + __u16 service;
> > + __u16 instance;
> > + kernel_ulong_t driver_data; /* Data private to the driver */
> 
> 
> Can we not repeat mistakes from the past and use const void * from day 1 please?

I just looked at what most other *_device_id structs had and did the same. I guess
they were left like that for legacy reasons? Might be good to add a comment next to
the kernel_ulong_t definition on why not to use it for future contributors.

> 
> > +};
> > +
> > /* dmi */
> 
> 
> Wouldn't it be better to keep sections ordered alphabetically so 'q'
> will go at least after 'd'?

It didn't look ordered so I didn't pay much attention to ordering but
sure, will reorder.

> 
> ...
> 
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#ifndef QCOM_QRTR_H
> > +#define QCOM_QRTR_H
> > +
> > +#include <linux/mod_devicetable.h>
> 
> 
> Not enough. Please, follow IWYU principle and include / forward
> declare all this header uses.

So you're saying forward declare struct qrtr_device_id instead of including
mod_devicetable.h?

> 
> > +struct qrtr_device {
> > + struct device dev;
> > + unsigned int node;
> > + unsigned int port;
> > + u16 service;
> > + u16 instance;
> > +};
> > +
> > +#define to_qrtr_device(d) container_of(d, struct qrtr_device, dev)
> > +
> > +struct qrtr_driver {
> > + int (*probe)(struct qrtr_device *qdev);
> > + void (*remove)(struct qrtr_device *qdev);
> > + const struct qrtr_device_id *id_table;
> > + struct device_driver driver;
> > +};
> > +
> > +#define to_qrtr_driver(d) container_of(d, struct qrtr_driver, driver)
> > +
> > +#define qrtr_driver_register(drv) __qrtr_driver_register(drv, THIS_MODULE)
> > +
> > +int __qrtr_driver_register(struct qrtr_driver *drv, struct module *owner);
> > +void qrtr_driver_unregister(struct qrtr_driver drv);
> > +
> > +#define module_qrtr_driver(__qrtr_driver) \
> > + module_driver(__qrtr_driver, qrtr_driver_register, \
> > + qrtr_driver_unregister)
> > +
> > +#endif / QCOM_QRTR_H */
> 
> 
> ...
> 
> > + int ret = 0;
> 
> 
> What is this assignment for? (The below is left for the context)

Yup looks unnecessary, will remove.

> 
> > if (cb->type == QRTR_TYPE_NEW_SERVER) {
> > /* Remote node endpoint can bridge other distant nodes /
> > qrtr_node_assign(node, le32_to_cpu(pkt->server.node));
> > +
> > + / Create a QRTR device /
> > + ret = ep->add_device(ep, le32_to_cpu(pkt->server.node),
> > + le32_to_cpu(pkt->server.port),
> > + le32_to_cpu(pkt->server.service),
> > + le32_to_cpu(pkt->server.instance));
> > + if (ret)
> > + goto err;
> > + } else if (cb->type == QRTR_TYPE_DEL_SERVER) {
> > + / Remove QRTR device corresponding to service */
> > + ret = ep->del_device(ep, le32_to_cpu(pkt->server.port));
> > + if (ret)
> > + goto err;
> > }
> 
> 
> ...
> 
> > + return ret ? ret : -EINVAL;
> 
> 
> It's also possible
> 
> return ret ?: -EINVAL;

Ack

> 
> > }
> 
> 
> ...
> 
> > +++ b/net/qrtr/smd.c
> > @@ -7,6 +7,7 @@
> > #include <linux/module.h>
> > #include <linux/skbuff.h>
> > #include <linux/rpmsg.h>
> > +#include <linux/soc/qcom/qrtr.h>
> 
> 
> Can we keep this more ordered?
> 
> Also include export.h when the code uses one of the EXPORT_*() macros.

Sure thing

> 
> ...
> 
> > +static int qcom_smd_qrtr_device_match(struct device *dev, const struct device_driver *drv)
> > +{
> > + struct qrtr_device *qdev = to_qrtr_device(dev);
> > + struct qrtr_driver *qdrv = to_qrtr_driver(drv);
> > + const struct qrtr_device_id *id = qdrv->id_table;
> > +
> > + if (!id)
> > + return 0;
> > +
> > + while (id->service != 0) {
> 
> 
> ' != 0' is redundant

Ack

> 
> > + if (id->service == qdev->service && id->instance == qdev->instance)
> > + return 1;
> > + id++;
> > + }
> > +
> > + return 0;
> > +}
> 
> 
> ...
> 
> > +static int qcom_smd_qrtr_match_device_by_port(struct device *dev, const void *data)
> > +{
> > + struct qrtr_device *qdev = to_qrtr_device(dev);
> > + unsigned const int *port = data;
> 
> 
> Why not
> 
> unsigned int port = *((const unsigned int *)data);

What does this achieve? Isn't it fine to implicitly cast void *?

> 
> > + return qdev->port == *port;
> > +}
> 
> 
> ...
> 
> > +static void qcom_smd_qrtr_add_device_worker(struct work_struct *work)
> > +{
> > + struct qrtr_new_server *new_server = container_of(work, struct qrtr_new_server, work);
> > + struct qrtr_smd_dev *qsdev = new_server->parent;
> > + struct qrtr_device *qdev;
> > + int ret;
> > +
> > + qdev = kzalloc(sizeof(*qdev), GFP_KERNEL);
> > + if (!qdev)
> > + return;
> > +
> > + *qdev = (struct qrtr_device) {
> > + .node = new_server->node,
> > + .port = new_server->port,
> > + .service = new_server->service,
> > + .instance = new_server->instance
> 
> 
> Leave trailing comma.

Ok

> 
> > + };
> 
> > + devm_kfree(qsdev->dev, new_server);
> 
> 
> ?!?! No, just no. Please, fix the object lifetimes and use proper
> allocators (not managed).

Missed this redundant managed kfree. See below about use of managed API

> 
> > + dev_set_name(&qdev->dev, "%d-%d", qdev->node, qdev->port);
> 
> 
> No error check?

Oops. Will add.

> 
> > + qdev->dev.bus = &qrtr_bus;
> > + qdev->dev.parent = qsdev->dev;
> > + qdev->dev.release = qcom_smd_qrtr_dev_release;
> > +
> > + ret = device_register(&qdev->dev);
> > + if (ret) {
> > + dev_err(qsdev->dev, "Failed to register QRTR device: %pe\n", ERR_PTR(ret));
> > + put_device(&qdev->dev);
> > + }
> > +}
> 
> 
> ...
> 
> > +static int qcom_smd_qrtr_add_device(struct qrtr_endpoint *parent, unsigned int node,
> > + unsigned int port, u16 service, u16 instance)
> > +{
> > + struct qrtr_smd_dev *qsdev = container_of(parent, struct qrtr_smd_dev, ep);
> > + struct qrtr_new_server *new_server;
> > +
> > + new_server = devm_kzalloc(qsdev->dev, sizeof(*new_server), GFP_KERNEL);
> 
> 
> Why is the managed API in use?!

When should I use or not use the managed API? I thought I was supposed to
use it whenever possible.

> 
> > + if (!new_server)
> > + return -ENOMEM;
> > +
> > + *new_server = (struct qrtr_new_server) {
> > + .parent = qsdev,
> > + .node = node,
> > + .port = port,
> > + .service = service,
> > + .instance = instance
> 
> 
> Leave trailing comma.

Sure

> 
> > + };
> > +
> > + INIT_WORK(&new_server->work, qcom_smd_qrtr_add_device_worker);
> > + schedule_work(&new_server->work);
> > +
> > + return 0;
> > +}
> > +
> > +static int qcom_smd_qrtr_del_device(struct qrtr_endpoint *parent, unsigned int port)
> > +{
> > + struct qrtr_smd_dev *qsdev = container_of(parent, struct qrtr_smd_dev, ep);
> > + struct qrtr_del_server *del_server;
> > +
> > + del_server = devm_kzalloc(qsdev->dev, sizeof(*del_server), GFP_KERNEL);
> 
> 
> Ditto.
> 
> > + if (!del_server)
> > + return -ENOMEM;
> > +
> > + del_server->parent = qsdev;
> > + del_server->port = port;
> > +
> > + INIT_WORK(&del_server->work, qcom_smd_qrtr_del_device_worker);
> > + schedule_work(&del_server->work);
> > +
> > + return 0;
> > +}
> 
> 
> ...
> 
> > +static int qcom_smd_qrtr_device_unregister(struct device *dev, void *data)
> > +{
> > + device_unregister(dev);
> > +
> > + return 0;
> 
> 
> Why? Can't this function be void?

Did it this way after seeing device_iter_t having int return type.

> 
> > +}
> 
> 
> ...
> 
> > {
> > struct qrtr_smd_dev *qsdev = dev_get_drvdata(&rpdev->dev);
> > 
> > + device_for_each_child(qsdev->dev, NULL, qcom_smd_qrtr_device_unregister);
> 
> 
> Perhaps _reversed() ?

What difference does the order make?

> 
> > qrtr_endpoint_unregister(&qsdev->ep);
> > 
> > dev_set_drvdata(&rpdev->dev, NULL);
> 
> > };
> 
> 
> 
> > +static void __exit qcom_smd_qrtr_exit(void)
> > +{
> > + unregister_rpmsg_driver(&qcom_smd_qrtr_driver);
> > + bus_unregister(&qrtr_bus);
> > +}
> > +
> > +subsys_initcall(qcom_smd_qrtr_init);
> > +module_exit(qcom_smd_qrtr_exit);
> 
> 
> Move these two closer to the mentioned callbacks.

Ack