| 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: <myzlzn6jv5ghpdj2k4gbcqh6bogi6w67yu6nj4haib4asgkapi@ddy4ebdpb6xt>
Date: Wed, 5 Nov 2025 16:31:49 -0600
From: Bjorn Andersson <andersson@...nel.org>
To: Sumit Kumar <sumit.kumar@....qualcomm.com>
Cc: Manivannan Sadhasivam <mani@...nel.org>,
Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>, Akhil Vinod <akhil.vinod@....qualcomm.com>,
Subramanian Ananthanarayanan <subramanian.ananthanarayanan@....qualcomm.com>, linux-kernel@...r.kernel.org, mhi@...ts.linux.dev,
linux-arm-msm@...r.kernel.org, quic_vpernami@...cinc.com
Subject: Re: [PATCH v2 3/3] bus: mhi: ep: Add loopback driver for data path
testing
On Tue, Nov 04, 2025 at 11:09:07AM +0530, Sumit Kumar wrote:
> Add loopback driver for MHI endpoint devices. The driver receives
Start by establishing why we want this.
> data on the uplink channel and echoes it back on the downlink
> channel using a workqueue for asynchronous processing.
>
> The driver is useful for testing MHI endpoint data path functionality
> and debugging communication issues.
>
> Co-developed-by: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
> Signed-off-by: Sumit Kumar <sumit.kumar@....qualcomm.com>
> ---
> drivers/bus/mhi/ep/Kconfig | 8 +++
> drivers/bus/mhi/ep/Makefile | 1 +
> drivers/bus/mhi/ep/mhi_ep_loopback.c | 134 +++++++++++++++++++++++++++++++++++
> 3 files changed, 143 insertions(+)
>
> diff --git a/drivers/bus/mhi/ep/Kconfig b/drivers/bus/mhi/ep/Kconfig
> index 90ab3b040672e0f04181d4802e3062afcc7cf782..ce7b63c2da82a6ca49528517687f4910552c35bb 100644
> --- a/drivers/bus/mhi/ep/Kconfig
> +++ b/drivers/bus/mhi/ep/Kconfig
> @@ -8,3 +8,11 @@ config MHI_BUS_EP
>
> MHI_BUS_EP implements the MHI protocol for the endpoint devices,
> such as SDX55 modem connected to the host machine over PCIe.
> +
> +config MHI_BUS_EP_LOOPBACK
> + tristate "MHI Endpoint loopback driver"
> + depends on MHI_BUS_EP
> + help
> + MHI endpoint loopback driver for data path testing.
> + This driver receives data on the uplink channel and echoes
> + it back on the downlink channel for testing purposes.
> diff --git a/drivers/bus/mhi/ep/Makefile b/drivers/bus/mhi/ep/Makefile
> index aad85f180b707fb997fcb541837eda9bbbb67437..02e4700e8dc3f860d40290476b0a852286683f8f 100644
> --- a/drivers/bus/mhi/ep/Makefile
> +++ b/drivers/bus/mhi/ep/Makefile
> @@ -1,2 +1,3 @@
> obj-$(CONFIG_MHI_BUS_EP) += mhi_ep.o
> mhi_ep-y := main.o mmio.o ring.o sm.o
> +obj-$(CONFIG_MHI_BUS_EP_LOOPBACK) += mhi_ep_loopback.o
> diff --git a/drivers/bus/mhi/ep/mhi_ep_loopback.c b/drivers/bus/mhi/ep/mhi_ep_loopback.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..ba6154dd9b785f051043c10a980ab340012ba986
> --- /dev/null
> +++ b/drivers/bus/mhi/ep/mhi_ep_loopback.c
> @@ -0,0 +1,134 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + */
> +
> +#include <linux/mhi_ep.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +
> +struct mhi_ep_loopback {
> + struct workqueue_struct *loopback_wq;
> + struct mhi_ep_device *mdev;
> +};
> +
> +struct mhi_ep_loopback_work {
> + struct mhi_ep_device *mdev;
> + struct work_struct work;
> + void *buf;
> + size_t len;
> +};
> +
> +static void mhi_ep_loopback_work_handler(struct work_struct *work)
> +{
> + struct mhi_ep_loopback_work *mhi_ep_lb_work = container_of(work,
> + struct mhi_ep_loopback_work, work);
> + int ret;
> +
> + ret = mhi_ep_queue_buf(mhi_ep_lb_work->mdev, mhi_ep_lb_work->buf,
> + mhi_ep_lb_work->len);
If you didn't use the "pile of abbreviations" naming scheme for your
local variables, you wouldn't have to line break this.
> + if (ret) {
> + dev_err(&mhi_ep_lb_work->mdev->dev, "Failed to send the packet\n");
> + kfree(mhi_ep_lb_work->buf);
> + }
> +
> + kfree(mhi_ep_lb_work);
> +}
> +
> +static void mhi_ep_loopback_ul_callback(struct mhi_ep_device *mhi_dev,
> + struct mhi_result *mhi_res)
> +{
> + struct mhi_ep_loopback *mhi_ep_lb = dev_get_drvdata(&mhi_dev->dev);
> + struct mhi_ep_loopback_work *mhi_ep_lb_work;
> + void *buf;
> +
> + if (!(mhi_res->transaction_status)) {
Unnecessary parenthesis.
> + buf = kmalloc(mhi_res->bytes_xferd, GFP_KERNEL);
> + if (!buf) {
> + dev_err(&mhi_dev->dev, "Failed to allocate buffer\n");
No error prints on alloc failures, the kernel has already printed for
you.
> + return;
> + }
> +
> + memcpy(buf, mhi_res->buf_addr, mhi_res->bytes_xferd);
kmalloc + memcpy == kmemdup()
> +
> + mhi_ep_lb_work = kmalloc(sizeof(*mhi_ep_lb_work), GFP_KERNEL);
> + if (!mhi_ep_lb_work) {
> + dev_err(&mhi_dev->dev, "Unable to allocate the work structure\n");
Ditto.
> + kfree(buf);
> + return;
> + }
> +
> + INIT_WORK(&mhi_ep_lb_work->work, mhi_ep_loopback_work_handler);
> + mhi_ep_lb_work->mdev = mhi_dev;
> + mhi_ep_lb_work->buf = buf;
> + mhi_ep_lb_work->len = mhi_res->bytes_xferd;
> +
> + queue_work(mhi_ep_lb->loopback_wq, &mhi_ep_lb_work->work);
> + }
> +}
> +
> +static void mhi_ep_loopback_dl_callback(struct mhi_ep_device *mhi_dev,
> + struct mhi_result *mhi_res)
> +{
> + void *buf;
> +
> + if (mhi_res->transaction_status)
> + return;
> +
> + buf = mhi_res->buf_addr;
> + if (buf)
> + kfree(buf);
No need to check for NULL, or have a local variable.
kfree(mhi_res->buf_addr);
> +}
> +
> +static int mhi_ep_loopback_probe(struct mhi_ep_device *mhi_dev, const struct mhi_device_id *id)
> +{
> + struct mhi_ep_loopback *mhi_ep_lb;
> +
> + mhi_ep_lb = devm_kzalloc(&mhi_dev->dev, sizeof(struct mhi_ep_loopback), GFP_KERNEL);
> + if (!mhi_ep_lb)
> + return -ENOMEM;
> +
> + mhi_ep_lb->loopback_wq = alloc_ordered_workqueue("mhi_loopback", WQ_MEM_RECLAIM);
> + if (!mhi_ep_lb->loopback_wq) {
> + dev_err(&mhi_dev->dev, "Failed to create workqueue.\n");
> + return -ENOMEM;
> + }
> +
> + mhi_ep_lb->mdev = mhi_dev;
> + dev_set_drvdata(&mhi_dev->dev, mhi_ep_lb);
> +
> + return 0;
> +}
> +
> +static void mhi_ep_loopback_remove(struct mhi_ep_device *mhi_dev)
> +{
> + struct mhi_ep_loopback *mhi_ep_lb = dev_get_drvdata(&mhi_dev->dev);
> +
> + destroy_workqueue(mhi_ep_lb->loopback_wq);
> + dev_set_drvdata(&mhi_dev->dev, NULL);
Shouldn't be necessary to clear drvdata here.
> +}
> +
> +static const struct mhi_device_id mhi_ep_loopback_id_table[] = {
> + { .chan = "LOOPBACK"},
> + {}
> +};
> +MODULE_DEVICE_TABLE(mhi, mhi_ep_loopback_id_table);
> +
> +static struct mhi_ep_driver mhi_ep_loopback_driver = {
> + .probe = mhi_ep_loopback_probe,
> + .remove = mhi_ep_loopback_remove,
> + .dl_xfer_cb = mhi_ep_loopback_dl_callback,
> + .ul_xfer_cb = mhi_ep_loopback_ul_callback,
> + .id_table = mhi_ep_loopback_id_table,
> + .driver = {
> + .name = "mhi_ep_loopback",
> + .owner = THIS_MODULE,
module_mhi_ep_driver() assigns owner for you...
Regards,
Bjorn
> + },
> +};
> +
> +module_mhi_ep_driver(mhi_ep_loopback_driver);
> +
> +MODULE_AUTHOR("Krishna chaitanya chundru <krishna.chundru@....qualcomm.com>");
> +MODULE_AUTHOR("Sumit Kumar <sumit.kumar@....qualcomm.com>");
> +MODULE_DESCRIPTION("MHI Endpoint Loopback driver");
> +MODULE_LICENSE("GPL");
>
> --
> 2.34.1
>
>
Powered by blists - more mailing lists