Re: [PATCH] soundwire: intel: trap TRIGGER_SUSPEND in .trigger callback

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

>>> For Intel machine drivers, all BE dailinks use
>>> .no_pcm = 1 (explicit setting)
>>> .nonatomic = 0 (implicit).
>>
>> that was my question, how is it implicit?
>> Should be explicitly set, right?

implicit behavior with C, if you don't set a field its value is zero...

>>> All FE dailinks use
>>> .no_pcm = 0 (implicit)
>>> .nonatomic = 1 (explicit setting)
>>>
>>>>> So the question is: is there any issue with sending an IPC in a DAI
>>>>> trigger callback?
>>>>
>>>> Sorry looks like we diverged, orignal question was can we do heavy tasks
>>>> in trigger, the answer is no, unless one uses nonatomic flag which was
>>>> added so that people can do that work with DSPs like sending IPCs..
>>>> Maybe we should add heavy slimbus/soundwire handling to it too...?
>>>
>>> I don't think the answer is as clear as you describe it Vinod.
>>>
>>> The .nonatomic field is at the BE dailink level.
>>>
>>> Unless I am missing something, I don't see anything that lets me set a
>>> .nonatomic property at the *DAI* level.
>>
>> I would say that was a miss in original design, it should have been set
>> at dai level or at least allowed to propagate from dai level setting.
>>
>> Now we are allowed to set it at dai_link but it is governed by dai
>> behaviour (DSP based DAI etc...)
> 
> Actually, there was one big piece I overlooked.  The whole DPCM BE
> operation is *always* tied with FE's.  That is, the nonatomic flag is
> completely ignored for BE, but just follows what FE sets up.
> 
> And that's the very confusing point when reviewing the code.  You
> cannot know whether it's written for non-atomic context or not.  This
> means that it's also error-prone; the code that assumes the operation
> in a certain mode might mismatch with the bound FE.
> 
> So, ideally, both FE and BE should set the proper nonatomic flags, and
> have a consistency check with WARN_ON() at the run time.

Sorry Takashi, I am not following. Are you asking me to add a .nonatomic
flag in all the exiting BEs along with a WARN_ON?

I can do this, but that's a sure way to trigger massive amounts of
user-reported "regression in kernel 5.1x". Is this really what you want?

Also I don't understand how this would help with the specific problem
raised in this patch: can we yes/no do something 'heavy' in a *DAI*
callback? What is the definition of 'heavy'?

And last, I am not sure it's always the case that a BE follows the FE
configuration. We've had cases of BE->BE loopbacks where the host
doesn't see or configured the data.