Re: [PATCH 2/2] soundwire: bus: Allow SoundWire peripherals to register IRQ handlers

[Pierre-Louis Bossart]

On 1/23/23 10:08, Richard Fitzgerald wrote:
> On 23/01/2023 15:50, Pierre-Louis Bossart wrote:
>>
>>
>> On 1/23/23 08:53, Charles Keepax wrote:
>>> On Fri, Jan 20, 2023 at 10:20:50AM -0600, Pierre-Louis Bossart wrote:
>>>> On 1/20/23 03:59, Charles Keepax wrote:
>>>>> On Thu, Jan 19, 2023 at 11:12:04AM -0600, Pierre-Louis Bossart wrote:
>>>>>> There should be an explanation and something checking that both
>>>>>> are not
>>>>>> used concurrently.
>>>>>
>>>>> I will try to expand the explanation a litte, but I dont see any
>>>>> reason to block calling both handlers, no ill effects would come
>>>>> for a driver having both and it is useful if any soundwire
>>>>> specific steps are needed that arn't on other control buses.
>>>>
>>>> I think it's problematic if the peripheral tries to wake-up the manager
>>>> from clock-stop with both an in-band wake (i.e. drive the data line
>>>> high) and a separate GPIO-based interrupt. It's asking for trouble
>>>> IMHO.
>>>> We spent hours in the MIPI team to make sure there were no races
>>>> between
>>>> the manager-initiated restarts and peripheral-initiated restarts,
>>>> adding
>>>> a 3rd mechanism in the mix gives me a migraine already.
>>>
>>> Apologies but I am struggling see why this has any bearing on
>>> the case of a device that does both an in-band and out-of-band
>>> wake. The code we are adding in this patch will only be called in the
>>> in-band case. handle_nested_irq doesn't do any hardware magic or
>>> schedule any threads, it just calls a function that was provided
>>> when the client called request_threaded_irq. The only guarantee
>>> of atomicity you have on the interrupt_callback is sdw_dev_lock
>>> and that is being held across both calls after the patch.
>>>
>>> Could you be a little more specific on what you mean by this
>>> represents a 3rd mechanism, to me this isn't a new mechanism just
>>> an extra callback? Say for example this patch added an
>>> interrupt_callback_early to sdw_slave_ops that is called just
>>> before interrupt_callback.
>>
>> Well, the main concern is exiting the clock-stop. That is handled by the
>> manager and could be done
>> a) as the result of the framework deciding that something needs to be
>> done (typically as a result of user/applications starting a stream)
>> b) by the device with an in-band wake in case of e.g. jack detection or
>> acoustic events detected
>> c) same as b) but with a separate out-of-band interrupt.
>>
>> I'd like to make sure b) and c) are mutually-exclusive options, and that
>> the device will not throw BOTH an in-band wake and an external interrupt.
> 
> Why would it be a problem if the device did (b) and (c)?
> (c) is completely invisible to the SoundWire core and not something
> that it has to handle. The handler for an out-of-band interrupt must
> call pm_runtime_get_sync() or pm_runtime_resume_and_get() and that
> would wake its own driver and the host controller.

The Intel hardware has a power optimization for the clock-stop, which
leads to different paths to wake the system. The SoundWire IP can deal
with the data line staying high, but in the optimized mode the wakes are
signaled as DSP interrupts at a higher level. That's why we added this
intel_link_process_wakeen_event() function called from
hda_dsp_interrupt_thread().

So yes on paper everything would work nicely, but that's asking for
trouble with races left and right. In other words, unless you have a
very good reason for using two wake-up mechanisms, pick a single one.

(a) and (c) are very similar in that all the exit is handled by
pm_runtime so I am not worried too much. I do worry about paths that
were never tested and never planned for.