Re: [PATCH v4 3/3] bus: mhi: ep: clients: Add loopback driver for data path testing

[Sumit Kumar <sumit.kumar@oss.qualcomm.com>]

On 6/23/2026 2:47 PM, Manivannan Sadhasivam wrote:
> On Mon, Jun 22, 2026 at 10:39:17AM +0530, Sumit Kumar wrote:
>> When an MHI endpoint device runs Linux, there is no firmware to implement
>> the LOOPBACK channel echo that real modem firmware provides. Without an
>> endpoint-side driver, the host loopback test has no software echo partner
>> and cannot exercise the full end-to-end MHI data path.
>>
>> Add an endpoint-side loopback driver that binds to the LOOPBACK channel and
>> echoes received data back to the host. An ordered workqueue is used for
>> asynchronous processing to preserve packet ordering. Together with the
>> host-side loopback driver, this enables complete MHI data path validation
>> for Linux-based endpoint devices.
>>
>> Co-developed-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>> Signed-off-by: Sumit Kumar <sumit.kumar@oss.qualcomm.com>
>> ---
>>   drivers/bus/mhi/ep/Kconfig            |   2 +
>>   drivers/bus/mhi/ep/Makefile           |   1 +
>>   drivers/bus/mhi/ep/clients/Kconfig    |  16 +++++
>>   drivers/bus/mhi/ep/clients/Makefile   |   2 +
>>   drivers/bus/mhi/ep/clients/loopback.c | 128 ++++++++++++++++++++++++++++++++++
>>   5 files changed, 149 insertions(+)
>>
>> diff --git a/drivers/bus/mhi/ep/Kconfig b/drivers/bus/mhi/ep/Kconfig
>> index 90ab3b040672e0f04181d4802e3062afcc7cf782..9edb81b39890e093a51138465a4d7705767eafa5 100644
>> --- a/drivers/bus/mhi/ep/Kconfig
>> +++ b/drivers/bus/mhi/ep/Kconfig
>> @@ -8,3 +8,5 @@ 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.
>> +
>> +source "drivers/bus/mhi/ep/clients/Kconfig"
>> diff --git a/drivers/bus/mhi/ep/Makefile b/drivers/bus/mhi/ep/Makefile
>> index aad85f180b707fb997fcb541837eda9bbbb67437..ab36ef2a40ab8174e5ddae44a3e6ccb8eb31168d 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-y += clients/
>> diff --git a/drivers/bus/mhi/ep/clients/Kconfig b/drivers/bus/mhi/ep/clients/Kconfig
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..4cf27184058ca2be020885b6f57b4cc44b5054b6
>> --- /dev/null
>> +++ b/drivers/bus/mhi/ep/clients/Kconfig
>> @@ -0,0 +1,16 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +config MHI_BUS_EP_LOOPBACK
>> +	tristate "MHI Endpoint LOOPBACK client driver"
>> +	depends on MHI_BUS_EP
>> +	help
>> +	  MHI Endpoint LOOPBACK client driver that binds to the MHI LOOPBACK
>> +	  channel as defined in the MHI specification. The LOOPBACK channel is
>> +	  implemented by MHI-based endpoint devices (modems, WLAN) in the field,
>> +	  where the endpoint firmware echoes back whatever the host sends.
>> +
>> +	  This driver receives data on the uplink channel and echoes it back on
>> +	  the downlink channel for testing the MHI endpoint data path.
>> +
>> +	  To compile this driver as a module, choose M here. The module
>> +	  will be called mhi_ep_loopback.
>> diff --git a/drivers/bus/mhi/ep/clients/Makefile b/drivers/bus/mhi/ep/clients/Makefile
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..71dc91cc63b02592b177cf66db6090748c0653a6
>> --- /dev/null
>> +++ b/drivers/bus/mhi/ep/clients/Makefile
>> @@ -0,0 +1,2 @@
>> +obj-$(CONFIG_MHI_BUS_EP_LOOPBACK) += mhi_ep_loopback.o
>> +mhi_ep_loopback-y += loopback.o
>> diff --git a/drivers/bus/mhi/ep/clients/loopback.c b/drivers/bus/mhi/ep/clients/loopback.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..05db91be6ffc5afe5a2022962410c96a7ec19962
>> --- /dev/null
>> +++ b/drivers/bus/mhi/ep/clients/loopback.c
>> @@ -0,0 +1,128 @@
>> +// 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>
>> +#include <linux/string.h>
>> +
>> +struct mhi_ep_loopback {
>> +	struct workqueue_struct *loopback_wq;
> s/loopback_wq/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 (ret) {
>> +		dev_err(&mhi_ep_lb_work->mdev->dev, "Failed to send the packet\n");
> 'Failed to queue buffer'
>
>> +		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_ep_lb)
>> +		return;
> Hmm. This seems similar to patch 1 race condition. But we do not have a EP
> specific API to stop a channel and flush the wq like
> mhi_ep_unprepare_from_transfer(). But you should add one and call it from
> remove().
mhi_ep_driver_remove() already does this before calling mhi_drv->remove():
        mutex_lock(&mhi_chan->lock);
       mhi_chan->xfer_cb(mhi_dev, -ENOTCONN);
       mhi_chan->state = MHI_CH_STATE_DISABLED;
       mhi_chan->xfer_cb = NULL;
       mutex_unlock(&mhi_chan->lock);

So by the time mhi_ep_loopback_remove() is entered, both channels are
already stopped and xfer_cb is NULL. A mhi_ep_unprepare_from_transfer()
call from remove() would be redundant.

The race with mhi_ep_read_completion() is handled by setting
dev_set_drvdata(NULL) before destroy_workqueue(), combined with the NULL
guard in ul_callback.

Please correct me if i am missing something here.

-Sumit
>
>> +
>> +	if (!mhi_res->transaction_status) {
>> +		if (!mhi_res->bytes_xferd)
>> +			return;
>> +
>> +		buf = kmemdup(mhi_res->buf_addr, mhi_res->bytes_xferd, GFP_KERNEL);
>> +		if (!buf)
>> +			return;
> Error log?
>
>> +
>> +		mhi_ep_lb_work = kmalloc(sizeof(*mhi_ep_lb_work), GFP_KERNEL);
>> +		if (!mhi_ep_lb_work) {
>> +			kfree(buf);
> Same here.
>
>> +			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)
>> +{
>> +	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(*mhi_ep_lb), GFP_KERNEL);
>> +	if (!mhi_ep_lb)
>> +		return -ENOMEM;
>> +
>> +	mhi_ep_lb->loopback_wq = alloc_ordered_workqueue("mhi_ep_loopback", WQ_MEM_RECLAIM);
>> +	if (!mhi_ep_lb->loopback_wq) {
>> +		dev_err(&mhi_dev->dev, "Failed to create workqueue.\n");
> nit: Remove fullstop at the end of error message.
>
>> +		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);
> As mentioned above, this should be dropped if you can ensure that xfer_cb()
> won't be called.
>
> - Mani
>