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

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

On 9/9/25 18:52, Charles Keepax wrote:
> On Tue, Sep 09, 2025 at 03:14:00PM +0200, Pierre-Louis Bossart wrote:
>> 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:
>>>> 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.
>>>
>>> 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?
> 
> I will have a look see if I can find that driver and see what is
> going on there. But HID seems like a good example (or at least the
> event part of HID) of why the wait isn't in the UMP code to me.
> 
> The only notification that comes from the device is the the
> UMP buffer is being returned to the host, which indicates a HID
> event has happened. But there is no time out on HID events, it
> could be months between HID events. Like one could set a timeout
> and at each timeout just verify the buffer is still set to the
> device but that seems very cautious. Or one could check the buffer
> is set as expected on boot, but I would really expect the SDCA
> reset/boot logic to ensure that is good.
> 
> If it is a UMP transfer where the host expects the device to do
> something and return that may need a timeout but as that is a
> subset of UMP transfers it feels like it makes more sense to
> implement the timeouts in the places that need them.

You have a point that in the case where the host waits for an event, then the notion of timeout is irrelevant.

What I was referring to was the case where the host writes a message and waits for a notification that the message has been handled. 
This could be a write to a buffer or a read from a buffer. That's standard IPC stuff, and all existing cases of such message-passing IPC do have a timeout built-in. The communication is serial by nature since there is a single signaling mechanism, so the host has to wait before sending another message.

I am not sure it's a great idea to leave the timeout handling at a higher level, because you will have to deal with other sources of information. Once a message has been sent with UMP, there will be additional signaling that the action requested by the host was completed on the peripheral side. In the case of FDL, once the firmware buffers have been transferred, possibly in different chunks, then the peripheral might have to perform an internal reboot/reset. That takes time and it's a different level of timeout.

My take is that the UMP should implement basic timeout capabilities for just 'buffer passed/ownership changed' transition. Things will go wrong and you want information on where they went wrong. Implementing timeouts at a higher level makes it harder to debug/root cause.