| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHp75Ved4cnpmiUzidoJqRbdnz=L-0F_KdyWifOOrZHUUf2KQA@mail.gmail.com>
Date: Thu, 10 Jul 2025 11:53:04 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: y.oudjana@...tonmail.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 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?
> +};
> +
> /* 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
Powered by blists - more mailing lists