Message-ID: <FDNgvJ7ZZjLaxvtcVqsbv9grSzkKe_xTylp1JWzltRetWlLycNFXOfTLz87WwRGC342HKQlohGRjy9WL3eBD3jB8F6Iuswabuq-eYkV18x4=@protonmail.com>
Date: Thu, 17 Jul 2025 13:21:35 +0000
From: Yassine Oudjana <y.oudjana@...tonmail.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: Manivannan Sadhasivam <mani@...nel.org>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, Bjorn Andersson <andersson@...nel.org>, Konrad Dybcio <konradybcio@...nel.org>, Masahiro Yamada <masahiroy@...nel.org>, Nathan Chancellor <nathan@...nel.org>, Nicolas Schier <nicolas.schier@...ux.dev>, Jonathan Cameron <jic23@...nel.org>, David Lechner <dlechner@...libre.com>, Nuno Sá <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>, Luca Weiss <luca@...aweiss.eu>, linux-arm-msm@...r.kernel.org, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, linux-kbuild@...r.kernel.org, linux-iio@...r.kernel.org
Subject: Re: [PATCH v2 2/4] net: qrtr: Turn QRTR into a bus

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

> On Thu, Jul 10, 2025 at 11:06 AM Yassine Oudjana via B4 Relay
> devnull+y.oudjana.protonmail.com@...nel.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

Powered by blists - more mailing lists