Message-ID: <zkhtvquyhgvdqcft6s3jmfjh76hg62mrwn4wj4qqoecrxprq4y@w5zvgp5vbbn2>
Date: Fri, 7 Nov 2025 17:58:18 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Bjorn Andersson <andersson@...nel.org>, 
	Sumit Kumar <sumit.kumar@....qualcomm.com>
Cc: 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 1/3] bus: mhi: host: Add loopback driver with sysfs
 interface

On Wed, Nov 05, 2025 at 04:17:41PM -0600, Bjorn Andersson wrote:
> On Tue, Nov 04, 2025 at 11:09:05AM +0530, Sumit Kumar wrote:
> > Add loopback driver for MHI host controllers that provides sysfs based
>   ^--- Here would be e good place to explain why we want this driver. Per
> https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
> start your commit message with a description of the problem you're
> solving.
> 
> > testing interface for data path validation. The driver supports the
> > "LOOPBACK" channel and offers configurable test parameters.
> > 
> > Sysfs interface provides:
> > - size: Configure TRE size
> > - num_tre: Set number of TREs for chained transfers
> > - start: Initiate loopback test
> > - status: Read test results
> 
> The words "loopback" and "testing" gives clear indications that this
> should live in debugfs and not sysfs.
> 

Though the wording gives an impression like that, this interface is for a MHI
channel that is defined in the MHI spec, so it is perfectly fine to have it
exposed as a sysfs interface to the users. This channel is intented to be used
for MHI protocol testing.

> Also, sysfs attribute should be documented in Documentation/ABI/testing/
> 

Yes, this is mandatory.

> > 
> > 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/host/Kconfig        |   7 +
> >  drivers/bus/mhi/host/Makefile       |   1 +
> >  drivers/bus/mhi/host/mhi_loopback.c | 347 ++++++++++++++++++++++++++++++++++++

Create a separate sub-directory 'clients' and move this driver there. Also,
rename it to just 'loopback', but keep the module name as 'mhi_loopback'.

> >  3 files changed, 355 insertions(+)
> > 
> > diff --git a/drivers/bus/mhi/host/Kconfig b/drivers/bus/mhi/host/Kconfig
> > index da5cd0c9fc620ab595e742c422f1a22a2a84c7b9..08a39ecb47f585bf39721c101ed5e2ff44bdd5f8 100644
> > --- a/drivers/bus/mhi/host/Kconfig
> > +++ b/drivers/bus/mhi/host/Kconfig
> > @@ -29,3 +29,10 @@ config MHI_BUS_PCI_GENERIC
> >  	  This driver provides MHI PCI controller driver for devices such as
> >  	  Qualcomm SDX55 based PCIe modems.
> >  
> > +config MHI_BUS_LOOPBACK
> > +	tristate "MHI loopback driver"

'MHI LOOPBACK client driver'

Use 'LOOPBACK' everywhere.

> > +	depends on MHI_BUS
> > +	help
> > +	  MHI loopback driver for data path testing. This driver
> > +	  provides a mechanism to test MHI data transfer functionality
> > +	  by implementing an echo service between host and endpoint.

Again, this just sounds like this driver exposes a random interface for testing.
You need to properly describe that this driver binds to the MHI LOOPBACK channel
and offers the testing interface to the users.

> > diff --git a/drivers/bus/mhi/host/Makefile b/drivers/bus/mhi/host/Makefile
> > index 859c2f38451c669b3d3014c374b2b957c99a1cfe..e5d6dccf5a976eaeb827c47924ad0614c9958f8b 100644
> > --- a/drivers/bus/mhi/host/Makefile
> > +++ b/drivers/bus/mhi/host/Makefile
> > @@ -4,3 +4,4 @@ mhi-$(CONFIG_MHI_BUS_DEBUG) += debugfs.o
> >  
> >  obj-$(CONFIG_MHI_BUS_PCI_GENERIC) += mhi_pci_generic.o
> >  mhi_pci_generic-y += pci_generic.o
> > +obj-$(CONFIG_MHI_BUS_LOOPBACK) += mhi_loopback.o
> > diff --git a/drivers/bus/mhi/host/mhi_loopback.c b/drivers/bus/mhi/host/mhi_loopback.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..980ace675718a79c97d9b2968ccef04c992a6c20
> > --- /dev/null
> > +++ b/drivers/bus/mhi/host/mhi_loopback.c
> > @@ -0,0 +1,347 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> > + */
> > +
> > +#include <linux/mhi.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/completion.h>
> > +#include <linux/string.h>
> > +#include <linux/random.h>
> > +#include <linux/kernel.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/types.h>
> > +#include <linux/errno.h>
> > +#include <linux/mutex.h>
> > +#include <linux/atomic.h>
> > +#include <linux/cleanup.h>
> > +#include <linux/sizes.h>
> 
> Keep them sorted, and make sure you need all of them.
> 
> > +
> > +#define MHI_LOOPBACK_DEFAULT_TRE_SIZE   32
> > +#define MHI_LOOPBACK_DEFAULT_NUM_TRE    1
> > +#define MHI_LOOPBACK_TIMEOUT_MS         5000
> > +#define MHI_LOOPBACK_MAX_TRE_SIZE       SZ_64K
> > +
> > +struct mhi_loopback {
> > +	struct mhi_device *mdev;
> > +	struct mutex lb_mutex;
> > +	struct completion comp;
> > +	atomic_t num_completions_received;
> > +	char result[32];
> > +	u32 num_tre;
> > +	u32 size;
> > +	bool loopback_in_progress;
> > +};
> > +
> > +static ssize_t size_show(struct device *dev,
> > +			 struct device_attribute *attr, char *buf)
> > +{
> > +	struct mhi_loopback *mhi_lb = dev_get_drvdata(dev);

s/mhi_lb/loopback

> > +
> > +	return sysfs_emit(buf, "%u\n", mhi_lb->size);
> > +}
> > +
> > +static ssize_t size_store(struct device *dev,
> > +			  struct device_attribute *attr,
> > +			  const char *buf, size_t count)
> > +{
> > +	struct mhi_loopback *mhi_lb = dev_get_drvdata(dev);
> > +	u32 val;
> > +
> > +	if (kstrtou32(buf, 0, &val)) {
> > +		dev_err(dev, "Invalid size value\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (val == 0 || val > MHI_LOOPBACK_MAX_TRE_SIZE) {
> > +		dev_err(dev, "Size must be between 1 and %u bytes\n",
> > +			MHI_LOOPBACK_MAX_TRE_SIZE);

How the user is supposed to know the size limit if it is just hardcoded in the
driver? You need to expose it as a RO attribute.

Also, 'size' here is the TRE size, not the size of the buffer. So name it as
such.

> > +		return -EINVAL;
> > +	}
> > +
> > +	guard(mutex)(&mhi_lb->lb_mutex);
> > +	if (mhi_lb->loopback_in_progress)
> 
> The only time loopback_in_progress is true is between the beginning and
> end of start_store(), and that entire function is under guard(lb_mutex),
> just as here and in num_tre_store().
> 
> So at all times loopback_in_progress is true, any other context will
> block on getting the mutex, and then it will be reset to false before
> the mutex is let go.
> 
> In other words, loopback_in_progress is unnecessary.
> 
> > +		return -EBUSY;
> > +
> > +	mhi_lb->size = val;
> > +	return count;
> > +}
> > +static DEVICE_ATTR_RW(size);
> > +
> > +static ssize_t num_tre_show(struct device *dev,
> > +			    struct device_attribute *attr, char *buf)
> > +{
> > +	struct mhi_loopback *mhi_lb = dev_get_drvdata(dev);
> > +
> > +	return sysfs_emit(buf, "%u\n", mhi_lb->num_tre);
> > +}
> > +
> > +static ssize_t num_tre_store(struct device *dev,
> > +			     struct device_attribute *attr,
> > +			     const char *buf, size_t count)
> > +{
> > +	struct mhi_loopback *mhi_lb = dev_get_drvdata(dev);
> > +	u32 val;
> > +	int el_num;
> > +
> > +	if (kstrtou32(buf, 0, &val)) {
> > +		dev_err(dev, "Invalid num_tre value\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (val == 0) {
> > +		dev_err(dev, "Number of TREs cannot be zero\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	guard(mutex)(&mhi_lb->lb_mutex);
> > +	if (mhi_lb->loopback_in_progress)
> > +		return -EBUSY;
> > +
> > +	el_num = mhi_get_free_desc_count(mhi_lb->mdev, DMA_TO_DEVICE);
> 
> Aren't the descs per-channel in MHI? Given that you have a mutex around
> start_store() is this actually a dynamic value?
> 
> > +	if (val > el_num) {
> > +		dev_err(dev, "num_tre (%u) exceeds ring capacity (%d)\n", val, el_num);
> > +		return -EINVAL;
> > +	}
> > +
> > +	mhi_lb->num_tre = val;
> > +	return count;
> > +}
> > +static DEVICE_ATTR_RW(num_tre);
> > +
> > +static ssize_t start_store(struct device *dev,
> > +			   struct device_attribute *attr,
> > +			   const char *buf, size_t count)
> > +{
> > +	struct mhi_loopback *mhi_lb = dev_get_drvdata(dev);
> > +	void *send_buf __free(kfree) = NULL;
> > +	void *recv_buf __free(kfree) = NULL;
> > +	u32 total_size, tre_count, tre_size;
> > +	int ret, i;
> > +
> > +	guard(mutex)(&mhi_lb->lb_mutex);
> > +
> > +	if (mhi_lb->loopback_in_progress)
> > +		return -EBUSY;
> > +
> > +	atomic_set(&mhi_lb->num_completions_received, 0);
> > +	mhi_lb->loopback_in_progress = true;
> > +
> > +	tre_size = mhi_lb->size;
> > +	tre_count = mhi_lb->num_tre;
> > +
> > +	strscpy(mhi_lb->result, "Loopback started", sizeof(mhi_lb->result));
> 

Do not print these progress reports in the 'status', just print the end result.
It can be a blocking read.

> All assignments to result are static const strings being strscpy'ed into
> the buffer, if you made result a const char * instead, you could just
> assign the string.
> 
> > +
> > +	total_size = tre_count * tre_size;
> > +
> > +	recv_buf = kzalloc(total_size, GFP_KERNEL);
> > +	if (!recv_buf) {
> > +		strscpy(mhi_lb->result, "Memory allocation failed", sizeof(mhi_lb->result));

When kzalloc() fails, it will itself print the error log.

> > +		mhi_lb->loopback_in_progress = false;
> > +		return -ENOMEM;
> 
> You're setting loopback_in_progress to false and returning in 7
> different places in this function. There seems to be some room for
> improvement here.
> 
> That said, as I said above, I don't think your code can ever find
> loopback_in_progress to be true...
> 
> > +	}
> > +
> > +	send_buf = kzalloc(total_size, GFP_KERNEL);
> > +	if (!send_buf) {
> > +		strscpy(mhi_lb->result, "Memory allocation failed", sizeof(mhi_lb->result));
> > +		mhi_lb->loopback_in_progress = false;
> > +		return -ENOMEM;
> > +	}
> > +
> > +	for (i = 0; i < tre_count; i++) {
> > +		ret = mhi_queue_buf(mhi_lb->mdev, DMA_FROM_DEVICE, recv_buf + (i * tre_size),
> > +				    tre_size, MHI_EOT);
> > +		if (ret) {
> > +			dev_err(dev, "Unable to queue read TRE %d: %d\n", i, ret);
> > +			strscpy(mhi_lb->result, "Queue tre failed", sizeof(mhi_lb->result));
> > +			mhi_lb->loopback_in_progress = false;
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	get_random_bytes(send_buf, total_size);
> > +
> > +	reinit_completion(&mhi_lb->comp);
> > +
> > +	for (i = 0; i < tre_count - 1; i++) {
> > +		ret = mhi_queue_buf(mhi_lb->mdev, DMA_TO_DEVICE, send_buf + (i * tre_size),
> > +				    tre_size, MHI_CHAIN);
> > +		if (ret) {
> > +			dev_err(dev, "Unable to queue send TRE %d (chained): %d\n", i, ret);
> > +			strscpy(mhi_lb->result, "Queue send failed", sizeof(mhi_lb->result));
> > +			mhi_lb->loopback_in_progress = false;
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	ret = mhi_queue_buf(mhi_lb->mdev, DMA_TO_DEVICE, send_buf + (i * tre_size),
> > +			    tre_size, MHI_EOT);
> > +	if (ret) {
> > +		dev_err(dev, "Unable to queue final TRE: %d\n", ret);
> > +		strscpy(mhi_lb->result, "Queue final tre failed", sizeof(mhi_lb->result));
> > +		mhi_lb->loopback_in_progress = false;
> > +		return ret;
> > +	}
> > +
> > +	if (!wait_for_completion_timeout(&mhi_lb->comp,
> > +					 msecs_to_jiffies(MHI_LOOPBACK_TIMEOUT_MS))) {
> > +		strscpy(mhi_lb->result, "Loopback timeout", sizeof(mhi_lb->result));
> > +		dev_err(dev, "Loopback test timed out\n");
> > +		mhi_lb->loopback_in_progress = false;
> > +		return -ETIMEDOUT;
> > +	}
> > +
> > +	ret = memcmp(send_buf, recv_buf, total_size);
> > +	if (!ret) {
> > +		strscpy(mhi_lb->result, "Loopback successful", sizeof(mhi_lb->result));
> > +		dev_info(dev, "Loopback test passed\n");
> 
> Why both print the test status and log it to the result? Less is more...
> 

Yes, I do find it quite annoying to see both getting passed on. Use dev_info()
to print the detailed error logs and just return the 'pass' or 'fail' status to
the user via the buffer.

> > +	} else {
> > +		strscpy(mhi_lb->result, "Loopback data mismatch", sizeof(mhi_lb->result));
> > +		dev_err(dev, "Loopback test failed\n");
> > +		ret = -EIO;
> > +	}
> > +
> > +	mhi_lb->loopback_in_progress = false;
> > +	return ret;
> > +}
> > +
> > +static DEVICE_ATTR_WO(start);
> > +
> > +static ssize_t status_show(struct device *dev,
> > +			   struct device_attribute *attr, char *buf)
> > +{
> > +	struct mhi_loopback *mhi_lb = dev_get_drvdata(dev);
> > +
> > +	return sysfs_emit(buf, "%s\n", mhi_lb->result);

Don't you need to have some lock or sync here?

> > +}
> > +static DEVICE_ATTR_RO(status);
> > +
> > +static void mhi_loopback_dl_callback(struct mhi_device *mhi_dev,
> > +				     struct mhi_result *mhi_res)
> > +{
> > +	struct mhi_loopback *mhi_lb = dev_get_drvdata(&mhi_dev->dev);
> > +
> > +	if (!mhi_res->transaction_status) {
> > +		if (atomic_inc_return(&mhi_lb->num_completions_received) >= mhi_lb->num_tre) {
> > +			atomic_set(&mhi_lb->num_completions_received, 0);
> > +			complete(&mhi_lb->comp);
> > +		}
> > +	} else {
> > +		dev_err(&mhi_dev->dev, "DL callback error: status %d\n",
> > +			mhi_res->transaction_status);
> > +		atomic_set(&mhi_lb->num_completions_received, 0);
> > +		complete(&mhi_lb->comp);
> > +	}
> > +}
> > +
> > +static void mhi_loopback_ul_callback(struct mhi_device *mhi_dev,
> > +				     struct mhi_result *mhi_res)
> > +{
> > +}
> > +
> > +static int mhi_loopback_probe(struct mhi_device *mhi_dev,
> > +			      const struct mhi_device_id *id)
> > +{
> > +	struct mhi_loopback *mhi_lb;
> > +	int rc;
> > +
> > +	mhi_lb = devm_kzalloc(&mhi_dev->dev, sizeof(*mhi_lb), GFP_KERNEL);
> > +	if (!mhi_lb)
> > +		return -ENOMEM;
> > +
> > +	mhi_lb->mdev = mhi_dev;
> > +
> > +	dev_set_drvdata(&mhi_dev->dev, mhi_lb);
> > +
> > +	mhi_lb->size = MHI_LOOPBACK_DEFAULT_TRE_SIZE;
> > +	mhi_lb->num_tre = MHI_LOOPBACK_DEFAULT_NUM_TRE;
> > +	mhi_lb->loopback_in_progress = false;
> 
> kzalloc() already did that for you.
> 
> > +
> > +	mutex_init(&mhi_lb->lb_mutex);
> > +	strscpy(mhi_lb->result, "Loopback not started", sizeof(mhi_lb->result));
> > +
> > +	rc = sysfs_create_file(&mhi_dev->dev.kobj, &dev_attr_size.attr);
> > +	if (rc) {
> > +		dev_err(&mhi_dev->dev, "failed to create size sysfs file\n");
> > +		goto out;
> > +	}
> > +
> > +	rc = sysfs_create_file(&mhi_dev->dev.kobj, &dev_attr_num_tre.attr);
> > +	if (rc) {
> > +		dev_err(&mhi_dev->dev, "failed to create num_tre sysfs file\n");
> > +		goto del_size_sysfs;
> 
> This is ugly, devm_device_add_group() seems more appropriate. Then
> again, I don't think this belongs in sysfs in the first place.
> 
> Regards,
> Bjorn
> 
> > +	}
> > +
> > +	rc = sysfs_create_file(&mhi_dev->dev.kobj, &dev_attr_start.attr);
> > +	if (rc) {
> > +		dev_err(&mhi_dev->dev, "failed to create start sysfs file\n");
> > +		goto del_num_tre_sysfs;
> > +	}
> > +
> > +	rc = sysfs_create_file(&mhi_dev->dev.kobj, &dev_attr_status.attr);
> > +	if (rc) {
> > +		dev_err(&mhi_dev->dev, "failed to create status sysfs file\n");
> > +		goto del_start_sysfs;
> > +	}
> > +
> > +	rc = mhi_prepare_for_transfer(mhi_lb->mdev);
> > +	if (rc) {
> > +		dev_err(&mhi_dev->dev, "failed to prepare for transfers\n");
> > +		goto del_status_sysfs;
> > +	}
> > +
> > +	init_completion(&mhi_lb->comp);
> > +
> > +	return 0;
> > +
> > +del_status_sysfs:
> > +	sysfs_remove_file(&mhi_dev->dev.kobj, &dev_attr_status.attr);
> > +del_start_sysfs:
> > +	sysfs_remove_file(&mhi_dev->dev.kobj, &dev_attr_start.attr);
> > +del_num_tre_sysfs:
> > +	sysfs_remove_file(&mhi_dev->dev.kobj, &dev_attr_num_tre.attr);
> > +del_size_sysfs:
> > +	sysfs_remove_file(&mhi_dev->dev.kobj, &dev_attr_size.attr);
> > +out:
> > +	return rc;
> > +}
> > +
> > +static void mhi_loopback_remove(struct mhi_device *mhi_dev)
> > +{
> > +	struct mhi_loopback *mhi_lb = dev_get_drvdata(&mhi_dev->dev);
> > +
> > +	if (mhi_lb)

Does this check make sense?

> > +		complete(&mhi_lb->comp);
> > +
> > +	sysfs_remove_file(&mhi_dev->dev.kobj, &dev_attr_status.attr);
> > +	sysfs_remove_file(&mhi_dev->dev.kobj, &dev_attr_start.attr);
> > +	sysfs_remove_file(&mhi_dev->dev.kobj, &dev_attr_num_tre.attr);
> > +	sysfs_remove_file(&mhi_dev->dev.kobj, &dev_attr_size.attr);

Move to attribute group as Bjorn suggested.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists