Message-ID: <3r5biv2frrk4bjtdqulmpruieaocrktrfxwwq5f3xossjikyef@hoxb4fgitjqs>
Date: Mon, 10 Nov 2025 08:28:33 -0600
From: Bjorn Andersson <andersson@...nel.org>
To: Manivannan Sadhasivam <mani@...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 Mon, Nov 10, 2025 at 11:56:11AM +0530, Manivannan Sadhasivam wrote:
> 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.

But, that defines what happens "on the wire", right?

The MHI specification doesn't state that you have to expose this to the
user in the form of taking a "size" and a "num_tre" number and then
expose a "start" knob which performs a loopback test, of which result
you can then find in a "status" file?

> We
> cannot control the interface unless we change the endpoint firmware (which is
> not feasible).

You're already changing the user-facing interface, because downstream
this is implemented using UCI...and I bet it's not sysfs files in
Windows...

> And all the future extensions could only be possible if we could
> modify the endpoint fw.
> 

I'm not talking about extensions to the protocol, or how the endpoint is
supposed to react.

I'm saying that it should be perfectly fine to have a test that just
fills the out-pipe with packets containing say the current timestamp,
and when they return I compare the timestamp in the payload with the
clock to see how long time the particular message took to loop back.
Same LOOPBACK protocol, different user-interface.

> 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 think you should include a comment in the driver as well, stating that
the user-facing interface is ABI and the MHI LOOPBACK protocol defines
that the payload has to be random etc. If that is the case...

> 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.

Fair enough.

> If users do not want this interface, then
> they can always decide not to enable the driver while building the kernel :)
> 

I expect even you will use this interface approximately 0% of the times
you boot the kernel. _Users_ will never use it.

So, I guess per your suggestion, we leave this disabled everywhere
(defconfig, distros etc), and then those few _developers_ who want it
can enable it?

Regards,
Bjorn

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

Powered by blists - more mailing lists