Re: [PATCH 1/3] net: qrtr: Turn QRTR into a bus

Jonathan Cameron
2025-04-06T16:01:23Z

On Sun, 06 Apr 2025 14:07:43 +0000
Yassine Oudjana <y.oudjana@protonmail.com> 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.
>
> In smd.c, replace all current occurences of qdev with qsdev in order to
> distinguish between the newly added QRTR device which represents a QRTR
> service with the existing QRTR SMD device which represents the endpoint
> through which services are provided.
>
> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
Hi Yassine

Just took a quick look through.

It might make more sense to do this with an auxiliary_bus rather
than defining a new bus.

I'd also split out the renames as a precursor patch.

Various other comments inline.

Jonathan

> diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
> index 00c51cf693f3..e11682fd7960 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;
How do we get here with non error value given we couldn't before?

> }
> EXPORT_SYMBOL_GPL(qrtr_endpoint_post);
>

> diff --git a/net/qrtr/smd.c b/net/qrtr/smd.c
> index c91bf030fbc7..fd5ad6a8d1c3 100644
> --- a/net/qrtr/smd.c
> +++ b/net/qrtr/smd.c
> @@ -7,6 +7,7 @@

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

> +void qrtr_driver_unregister(struct qrtr_driver *drv)
> +{
> + driver_unregister(&drv->driver);
> +}
> +EXPORT_SYMBOL_GPL(qrtr_driver_unregister);

Given this is a 'new thing' maybe namespace it from the start?
EXPORT_SYMBOL_NS_GPL();

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

return qdev->port == *port;

> +
> + 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;
> +
Maybe
*qdev = (struct qrtr_device *) {
};
and free new_server after all of these are filled in.

> + qdev->node = new_server->node;
> + qdev->port = new_server->port;
> + qdev->service = new_server->service;
> + qdev->instance = new_server->instance;
> +
> + devm_kfree(qsdev->dev, new_server);

As below.

> +
> + 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;
> + qdev->dev.driver = NULL;

it's kzalloc'd so no need to set this.

> +
> + 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);
> +
> + devm_kfree(qsdev->dev, del_server);
If we are always going to free what was alocated in qcom_smd_qrtr_del_device()
why use devm at all?
> +
> + if (dev)
> + device_unregister(dev);
If this doesn't match anything I'm guessing it's a bug? So maybe an error message?

> +}
> +
> +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(struct qrtr_new_server), GFP_KERNEL);

As below. sizeof(*new_server)

> + if (!new_server)
> + return -ENOMEM;
> +
*new_server = (struct qtr_new_server) {
.parent = qsdev,
.ndoe = node,
...
};

perhaps a tiny bit easier to read?

> + new_server->parent = qsdev;
> + new_server->node = node;
> + new_server->port = port;
> + new_server->service = service;
> + new_server->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(struct qrtr_del_server), GFP_KERNEL);

sizeof(*del_server)
preferred as then no one has to check types match.

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

One option that may simplify this is to do the device_unregister() handling
a devm_action_or_reset() handler that is using the parent device as it's dev
but unregistering the children. That way the unregister is called in the
reverse order of setup and you only register a handler for those devices
registered (rather walking children). I did this in the CXL pmu driver
for instance.

> +
> + return 0;
> +}
> +

> @@ -82,9 +276,11 @@ static int qcom_smd_qrtr_probe(struct rpmsg_device *rpdev)
>
> static void qcom_smd_qrtr_remove(struct rpmsg_device *rpdev)
> {
> - struct qrtr_smd_dev *qdev = dev_get_drvdata(&rpdev->dev);
> + struct qrtr_smd_dev *qsdev = dev_get_drvdata(&rpdev->dev);

May be worth doing the rename in a precursor patch to simplify a little what is
in this one.

> +
> + device_for_each_child(qsdev->dev, NULL, qcom_smd_qrtr_device_unregister);
>
> - qrtr_endpoint_unregister(&qdev->ep);
> + qrtr_endpoint_unregister(&qsdev->ep);
>
> dev_set_drvdata(&rpdev->dev, NULL);
> }
> @@ -104,7 +300,27 @@ 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)
> + ret = register_rpmsg_driver(&qcom_smd_qrtr_driver);
This style tends to extend badly. Go with more conventional errors
out of line style.

if (ret)
return ret;

ret = register_rpmsg_driver(&qcom_smd_qrtr_driver);
if (ret) {
bus_unregister(&qtr_bus);
return ret;
}

return 0;

> + else
> + bus_unregister(&qrtr_bus);
> +
> + return ret;
> +}
> +
> +static void __exit qcom_smd_qrtr_exit(void)
> +{
> + bus_unregister(&qrtr_bus);

Order should be the reverse of what happened in probe so swap these round.

> + unregister_rpmsg_driver(&qcom_smd_qrtr_driver);
> +}
> +
> +subsys_initcall(qcom_smd_qrtr_init);
> +module_exit(qcom_smd_qrtr_exit);
>
> MODULE_ALIAS("rpmsg:IPCRTR");
> MODULE_DESCRIPTION("Qualcomm IPC-Router SMD interface driver");
>