Re: [PATCH 09/15] ASoC: SDCA: Add UMP buffer helper functions

[Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>]

On 9/8/25 15:15, Charles Keepax wrote:
> On Mon, Sep 08, 2025 at 02:02:09PM +0200, Pierre-Louis Bossart wrote:
>> On 9/5/25 16:31, Charles Keepax wrote:
>>> Add helper functions for handling Universal Message Passing (UMP)
>>> buffers on SDCA devices. These are generic mechanisms to pass blocks of
>>> binary data between the host and the device, in both directions. They
>>> are used for things like passing HID descriptors and the File Download
>>> process.
>>
>> Humm, these helpers are very host-centric and don't seem to cover the basic
>> UMP message passing, e.g. for host to device:
>>
>> 1. host puts information in a buffer.
> 
> by calling sdca_ump_write_message

ok
>> 2. host sets ownership to device and waits for ownership to be returned.
> 
> by calling sdca_ump_set_owner_device

sdca_ump_set_owner_device does the ownership change only, there's no wait.

>> 3. device does a bunch of things and returns the ownership to the host.
>> 4. host detects ownership change or times out with a failed transfer error message.
> 
> by receiving an IRQ on the owner control.

but that IRQ may never come, in which case the entire protocol would fail.
>> What am I missing?
> 
> You might need to elaborate a little more on what you think is
> missing. The four functions implement the four operations that
> power a UMP buffer, checking you have ownership, passing
> ownership back to the device, reading data, and writing data.
> 
> One could combine more of the functionality into a single helper,
> but then you start to hit problems where you need to do extra
> bits.
> 
> For example HID, very simple just: check you own the buffer,
> read the message, and pass the buffer back.

I remember one of the early implementations of HID support in a Realtek driver needed some sort of timeout to reset the communication in case the ownership change interrupt never came. It's reasonable to expect that things will go wrong at some point in the communication between host and device, isn't it?

> FDL, has extra steps: check you own the buffer, check the FDL
> status, then write the buffer, then write the FDL response,
> and only then pass the buffer back.

Right, my feedback was only related to the basic UMP support and what happens if the expected ownership isn't signaled. 
I do think we need some sort of software timeout to make sure we don't wait forever on an ownership change. I did see the use of wait_for_completion/complete for the FDL stuff but it should be used at a lower UMP level IMHO.