Date:   Wed, 31 May 2023 09:04:10 -0600
From:   Jeffrey Hugo <quic_jhugo@...cinc.com>
To:     Greg KH <gregkh@...uxfoundation.org>,
        Manivannan Sadhasivam <mani@...nel.org>
CC:     <kuba@...nel.org>, <andersson@...nel.org>, <daniel@...ll.ch>,
        <mhi@...ts.linux.dev>, <linux-arm-msm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@...cinc.com>
Subject: Re: [PATCH] bus: mhi: host: Add userspace character interface

On 5/31/2023 8:35 AM, Greg KH wrote:
> On Wed, May 31, 2023 at 07:58:03PM +0530, Manivannan Sadhasivam wrote:
>> + Jakub (who NACKed the previous submission of UCI driver)
>> Link to previous submission: https://lore.kernel.org/all/1606533966-22821-1-git-send-email-hemantk@codeaurora.org/
>>
>> On Mon, May 22, 2023 at 01:04:59PM -0600, Jeffrey Hugo wrote:
>>> From: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@...cinc.com>
>>>
>>> I2C, USB, and PCIe are examples of buses which have a mechanism to give
>>> userspace direct access to a device on those buses. The MHI userspace
>>> character interface (uci) is the MHI bus analogue.
>>>
>>> The MHI bus devices are MHI channels which ferry blocks of data from one
>>> end to the other. With this simple purpose, we can define a simple
>>> interface to userspace - a character device that supports open/close/read/
>>> write/poll operations. Since bus devices can only have a single consumer
>>> we encode a whitelist of MHI channels to be exported to userspace so as
>>> to avoid conflicts.
>>>
>>> We also make this mechanism open to any device that implements MHI.
>>> Today this includes WLAN (Wi-Fi), WWAN (4G/5G cellular), and ML/AI
>>> devices. More devices are expected in the future.
>>>
>>> In addition to implementing the framework for uci, we include an initial
>>> usecase - the QAIC Sahara device.
>>>
>>> Sahara is a file transfer protocol that is commonly used for two purposes
>>> when interacting with a device - transferring firmware to the device and
>>> transferring crashdumps from the device. The Sahara protocol puts the
>>> receiver of the data in control of the transfer. A firmware transfer
>>> operation would have the device requesting the specific firmware images
>>> that the device wants, and the host satisfying those requests.
>>>
>>> In most cases, including for AIC100, Sahara is used as part of a two stage
>>> loading process. The device will boot a very limited bootloader that does
>>> the base minimum initialization and jump to the next stage. A simple, one-
>>> shot protocol like BHI is used to send the next stage bootloader to the
>>> device. This second stage bootloader contains more functionality and
>>> implements the Sahara protocol. The second stage determines from various
>>> inputs what set of runtime firmware is required to boot the device into an
>>> operational status, and requests those pieces from the host.  With those
>>> images transferred over, the device can funnly initialize.
>>>
>>> Each AIC100 instance (currently, up to 16) in a system will create a
>>> MHI device for QAIC_SAHARA. MHI_uci will consume each of these and create
>>> a unique chardev which will be found as
>>> /dev/<mhi instance>_QAIC_SAHARA
>>> For example - /dev/mhi0_QAIC_SAHARA
>>>
>>> An open userspace application that can consume these devices for firmware
>>> transfers is located at https://github.com/andersson/qdl
>>>
>>> Signed-off-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@...cinc.com>
>>> [jhugo: Rename to uci, plumb to mhi, rewrite commit text]
>>> Signed-off-by: Jeffrey Hugo <quic_jhugo@...cinc.com>
>>
>> The previous attempt on adding UCI driver was NACKed by Jakub. For merging this
>> patch, I need an ACK from Jakub.
> 
> Given that this fails the kernel robot tests, why would anyone ack it
> as-is?

I think Mani I looking for some "guidance" on the "architecture", and 
frankly so am I.  An official Ack from Jakub might not be quite the 
right thing at this stage, but at-least Jakub could come in and say he 
isn't planning on NACKing this right off the bat, in particular because 
this functionality can be used by WWAN devices which seems to be what 
caused the mess the last time around.

We've gone full circle here.  This functionality was proposed as part of 
the bus.  Jakub came in an NACKed that, which resulted in the WWAN 
subsystem and the guidance that this functionally belongs with the 
devices.  I tried to put it with the AIC100/QAIC device based on that, 
and that got NACKed by Daniel (GPU) saying that this belongs with the 
bus.  You (Greg) seemed to agree with Daniel on that.

Fixing kernel robot tests is one thing (I haven't seen any reports on 
this iteration), but if there is no agreement on where this lives, isn't 
it DOA?

In summary, if you don't like this, please give some clear guidance. 
Greg, you've told me in the past that you don't discuss "architecture" 
without seeing the code.  Here is some code.  I don't claim it is 
perfect (you mentioned the QAIC version had some issues you were going 
to help with), but I would like to see some input.

-Jeff

Powered by blists - more mailing lists