Re: [PATCH v2 02/16] bus: mhi: core: Add support for registering MHI controllers

From: Jeffrey Hugo
Date: Fri Feb 14 2020 - 11:41:43 EST

On 2/13/2020 8:53 AM, Greg KH wrote:
On Thu, Feb 13, 2020 at 09:18:09PM +0530, Manivannan Sadhasivam wrote:
Hi Greg,

On Thu, Feb 13, 2020 at 07:34:18AM -0800, Greg KH wrote:
On Thu, Feb 13, 2020 at 08:50:13PM +0530, Manivannan Sadhasivam wrote:
On Tue, Feb 11, 2020 at 11:20:55AM -0800, Greg KH wrote:
On Wed, Feb 12, 2020 at 12:11:30AM +0530, Manivannan Sadhasivam wrote:
Hi Greg,

On Thu, Feb 06, 2020 at 05:57:55PM +0100, Greg KH wrote:
On Fri, Jan 31, 2020 at 07:19:55PM +0530, Manivannan Sadhasivam wrote:
--- /dev/null
+++ b/drivers/bus/mhi/core/init.c
@@ -0,0 +1,407 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018-2020, The Linux Foundation. All rights reserved.
+ *
+ */
+
+#define dev_fmt(fmt) "MHI: " fmt

This should not be needed, right? The bus/device name should give you
all you need here from what I can tell. So why is this needed?


The log will have only the device name as like PCI-E. But that won't specify
where the error is coming from. Having "MHI" prefix helps the users to
quickly identify that the error is coming from MHI stack.

If the driver binds properly to the device, the name of the driver will
be there in the message, so I suggest using that please.

No need for this prefix...


So the driver name will be in the log but that won't help identifying where
the log is coming from. This is more important for MHI since it reuses the
`struct device` of the transport device like PCI-E. For instance, below is
the log without MHI prefix:

[ 47.355582] ath11k_pci 0000:01:00.0: Requested to power on
[ 47.355724] ath11k_pci 0000:01:00.0: Power on setup success

As you can see, this gives the assumption that the log is coming from the
ath11k_pci driver. But the reality is, it is coming from MHI bus.

Then you should NOT be trying to "reuse" a struct device.

With the prefix added, we will get below:

[ 47.355582] ath11k_pci 0000:01:00.0: MHI: Requested to power on
[ 47.355724] ath11k_pci 0000:01:00.0: MHI: Power on setup success

IMO, the prefix will give users a clear idea of logs and that will be very
useful for debugging.

Hope this clarifies.

Don't try to reuse struct devices, if you are a bus, have your own
devices as that's the correct way to do things.


I assumed that the buses relying on a different physical interface for the
actual communication can reuse the `struct device`. I can see that the MOXTET
bus driver already doing it. It reuses the `struct device` of SPI.

How can you reuse anything?

And this assumption has deep rooted in MHI bus design.

Maybe I do not understand what this is at all, but a device can only be
on one "bus" at a time. How is that being broken here?

Maybe I can bring some clarity to the discussion. I hope I do not muddy things.

My interest in this series is I am developing a kernel driver for a product which uses MHI. Unfortunately I am currently under embargo, otherwise I would post an RFC of my kernel driver which uses this series. Let me try to dance around the embargo issue to hopefully illustrate how I aim to use this series.

Note that I expect my usecase is not the "defining" usecase. I fully expect other usecases to exist. IE what I hope to explain is not to be taken as the "one true way" to use MHI.

At the physical layer, my product is attached to a PCIE bus. Thus my driver registers as a PCI driver for a device containing a specific VID/PID. My product uses MHI as the "control path". When I want to give commands to my product, they occur via the MHI protocol. The "data path" occurs outside of MHI, and thus there is direct access to that hardware via PCIE.

In my usecase, my driver acts as both MHI Controller, and also MHI Client Driver. Based on the PCIE device probe, my driver registers a MHI Controller. Its going to be important for me that I have some means of associating the MHI Controller instance to the PCIE device instance (struct device) so that I can then correlate MHI devices to the PCIE device (more on that in a bit). Right now, since the MHI controller essentially reuses the struct device from the PCIE device (provided by my driver), I can do that.

My MHI Controller exposes a number of channels, but for this discussion, the relevant one is the "control" channel.

My kernel driver also registers a MHI Client Driver which binds to this control channel. Since its possible to have multiple instances of my product, I need to make a connection between the specific control channel device that my MHI Client Driver has probed on, and the PCIE device this control channel runs on so that I can store this connection in my struct that is in the driver data of the struct device of the pci device. With that connection made, I can then correlate activity on the control MHI channel for a particular device with the services that my driver provides.

So, what we have, in my case, is a layering of buses, and its important to be able to identify the relationships (ie this particular MHI bus is associated with this particular PCI device on the PCI bus).

As I said, currently since the MHI bus is reusing the PCI struct device, its possible for my driver to make these connections as necessary. If the MHI bus needs to have its own struct device instance, then how do you propose those connections be determined? The MHI bus device is a child of the PCI device (thus from the MHI bus device, we dereference its parent)?

--
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.