Message-ID: <fae5pl3su5mul7qnaealqvsgq37m6hlj2ckabrbjj55dqrixxw@q2dbbuq7pikw>
Date: Mon, 10 Nov 2025 11:56:11 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Bjorn Andersson <andersson@...nel.org>
Cc: Sumit Kumar <sumit.kumar@....qualcomm.com>, 
	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 Sun, Nov 09, 2025 at 10:00:26PM -0600, Bjorn Andersson wrote:
> On Fri, Nov 07, 2025 at 05:58:18PM +0530, Manivannan Sadhasivam wrote:
> > 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.
> > 
> 
> The fact that the protocol is defined in the specification doesn't imply
> that it's intended to be used by the typical user.
> 
> Also, the specification defines the LOOPBACK channel, it doesn't define
> an interface where the user can request that a certain number of packets
> of random data is sent and if those come back we can learn about that
> from a "results" file. Downstream has a completely different
> implementation of the same specification.
> 
> I could imagine that at some point one would want extend this test
> interface by altering the behavior of the packet generator, inject
> timestamps etc - to measure raw throughput, latency, jitter etc. Good
> reasons for not turning this into an ABI.
> 

I missed one important point while replying. This channel has an existing
interface between the host and endpoint i.e., the MHI based WLAN and modems out
there in the field already support an *interface* on top of this channel. We
cannot control the interface unless we change the endpoint firmware (which is
not feasible). And all the future extensions could only be possible if we could
modify the endpoint fw.

So for this specific reason, going by the sysfs ABI made sense to me. But to
admit, this point was never mentioned in the cover letter or in the patch
description and need to be fixed.

I also asked the Qcom PCIe team to verify that this interface, just works with
all kind of MHI devices if the devices support the LOOPBACK channel.

> 
> Thinking more about the use case, I also presume most MHI devices has a
> LOOPBACK channel, so every user is going to have this .ko auto-loaded,
> just so that they can poke sysfs to send some random data... So perhaps
> we should omit MODULE_DEVICE_TABLE()?
> 

I don't want to omit the MODULE_DEVICE_TABLE() since that will require users to
load the driver manually to use it. If users do not want this interface, then
they can always decide not to enable the driver while building the kernel :)

- Mani

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

Powered by blists - more mailing lists