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

From: Jeffrey Hugo <jhugo@codeaurora.org>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
	arnd@arndb.de, smohanad@codeaurora.org, kvalo@codeaurora.org,
	bjorn.andersson@linaro.org, hemantk@codeaurora.org,
	linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 02/16] bus: mhi: core: Add support for registering MHI controllers
Date: Sun, 26 Jan 2020 14:00:05 -0700	[thread overview]
Message-ID: <21a513d8-225b-9543-1b61-bcb1e77a1b1e@codeaurora.org> (raw)
In-Reply-To: <20200125132615.GA3516435@kroah.com>

On 1/25/2020 6:26 AM, Greg KH wrote:
> On Fri, Jan 24, 2020 at 11:12:57AM -0700, Jeffrey Hugo wrote:
>> On 1/24/2020 10:47 AM, Greg KH wrote:
>>> On Fri, Jan 24, 2020 at 07:24:43AM -0700, Jeffrey Hugo wrote:
>>>>>> +/**
>>>>>> + * struct mhi_result - Completed buffer information
>>>>>> + * @buf_addr: Address of data buffer
>>>>>> + * @dir: Channel direction
>>>>>> + * @bytes_xfer: # of bytes transferred
>>>>>> + * @transaction_status: Status of last transaction
>>>>>> + */
>>>>>> +struct mhi_result {
>>>>>> +	void *buf_addr;
>>>>>
>>>>> Why void *?
>>>>
>>>> Because its not possible to resolve this more clearly.  The client provides
>>>> the buffer and knows what the structure is.  The bus does not. Its just an
>>>> opaque pointer (hence void *) to the bus, and the client needs to decode it.
>>>> This is the struct that is handed to the client to allow them to decode the
>>>> activity (either a received buf, or a confirmation that a transmitted buf
>>>> has been consumed).
>>>
>>> Then shouldn't this be a "u8 *" instead as you are saying how many bytes
>>> are here?
>>
>> I'm sorry, I don't see the benefit of that.  Can you elaborate on why you
>> think that u8 * is a better type?
>>
>> Sure, its an arbitrary byte stream from the perspective of the bus, but to
>> the client, 99% of the time its going to have some structure.
> 
> So which side is in control here, the "bus" or the "client"?  For the
> bus to care, it's a bytestream and should be represented as such (like
> you have) with a number of bytes in the "packet". >
> If you already know the structure types, just make a union of all of the
> valid ones and be done with it.  In other words, try to avoid using void
> * as much as is ever possible please.

The client is in control.  Perhaps if you think of this like a NIC - the 
NIC is a dumb pipe that you shove bytes into and get bytes out of.  The 
NIC doesn't know or care what the bytes are, only that it performs its 
responsibilities of successfully moving those bytes through the pipe. 
The bytes could be a TCP packet, UDP packet, raw IP packet, or something 
entirely different.  The NIC doesn't need to know, nor care.

MHI is a little one sided because its designed so that the Host is in 
control for the most part.

In the transmit path, the client on the Host gives the bus a stream of 
bytes.  The DMA-able address of that stream of bytes is put into the bus 
structures, and the doorbell is rung.  Then the device pulls in the data.

In the receive path, the client on the host gives the bus a receive 
buffer.  The DMA-able address of that buffer is put into the bus 
structures.  When the device wants to send data to the host, it picks up 
the buffer address, copies the data into it, and then flags an event to 
the Host.

This structure we are discussing is used in the callback from the bus to 
the client to either signal that the TX buffer has been consumed by the 
device and is now back under the control of the client, or that the 
device has consumed a queued RX buffer, and now the buffer is back under 
the control of the client and can be read to determine what data the 
device sent.

In both cases, its impossible for the bus to know the structure or 
content of the data.  All the bus knows or cares about is the location 
and size of the buffer.  Its entirely up to the control of the client. 
The client could be the network stack, in which case the data is 
probably an IP packet.  The client could be entirely something else 
where the client protocol running over MHI is entirely unique to that 
client.

Since MHI supports arbitrary clients, its impossible to come up with 
some kind of union that describes every possible client's structure 
definitions from now until the end of time.

void * is the only type that makes realistic sense.

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