[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250406170111.7a11437a@jic23-huawei>
Date: Sun, 6 Apr 2025 17:01:11 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Yassine Oudjana <y.oudjana@...tonmail.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Bjorn Andersson
<andersson@...nel.org>, Konrad Dybcio <konradybcio@...nel.org>, Manivannan
Sadhasivam <manivannan.sadhasivam@...aro.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>, Masahiro Yamada <masahiroy@...nel.org>, Nathan
Chancellor <nathan@...nel.org>, Nicolas Schier <nicolas.schier@...ux.dev>,
Alexander Sverdlin <alexander.sverdlin@...il.com>, Sean Nyekjaer
<sean@...nix.com>, Javier Carrasco <javier.carrasco.cruz@...il.com>, Matti
Vaittinen <mazziesaccount@...il.com>, Antoniu Miclaus
<antoniu.miclaus@...log.com>, Ramona Gradinariu
<ramona.gradinariu@...log.com>, "Yo-Jung (Leo) Lin" <0xff07@...il.com>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, Neil Armstrong
<neil.armstrong@...aro.org>, Barnabás Czémán
<barnabas.czeman@...nlining.org>, Danila Tikhonov <danila@...xyga.com>,
Antoni Pokusinski <apokusinski01@...il.com>, Vasileios Amoiridis
<vassilisamir@...il.com>, Petar Stoykov <pd.pstoykov@...il.com>, shuaijie
wang <wangshuaijie@...nic.com>, Yasin Lee <yasin.lee.x@...il.com>,
"Borislav Petkov (AMD)" <bp@...en8.de>, Dave Hansen
<dave.hansen@...ux.intel.com>, Tony Luck <tony.luck@...el.com>, Pawan Gupta
<pawan.kumar.gupta@...ux.intel.com>, Ingo Molnar <mingo@...nel.org>,
Yassine Oudjana <yassine.oudjana@...il.com>, linux-kernel@...r.kernel.org,
linux-iio@...r.kernel.org, linux-arm-msm@...r.kernel.org,
netdev@...r.kernel.org, linux-kbuild@...r.kernel.org
Subject: Re: [PATCH 1/3] net: qrtr: Turn QRTR into a bus
On Sun, 06 Apr 2025 14:07:43 +0000
Yassine Oudjana <y.oudjana@...tonmail.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@...tonmail.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");
>
Powered by blists - more mailing lists