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

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

>>> 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?
> 
> That's why I wrote "ideally".  We all know that the world is no
> perfect...  So hardening in that way would be possible, but it has to
> be done carefully if we really go for it, and I'm not asking you to do
> that now.
> 
>> 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'?
> 
> My previous comment wasn't specifically about your patch itself but
> rather arguing a generic problem.  We have no notion or matching
> mechanism of the atomicity of DPCM BE.

I think the only problem is actually on the SoundWire dailinks.

For SSP/DMIC we don't do anything for BE dailinks, there's no IPC or
waits, only some settings/masks. I don't see any need to set the
.nonatomic field in those cases.

But for SoundWire, we do use the 'stream' functions from the BE ops
callbacks - sdw_prepare_stream, sdw_trigger_stream - which will do a
bank switch operation. That's certainly not an atomic operation, there's
a clear wait_for_completion().

That seems like a miss indeed, I'll add a patch to set the .nonatomic
field for these links.

But for this patch proper, does anyone have an objection? I am still not
clear on what is permissible at the DAI level.

>> 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.
> 
> Hm, how the trigger and other PCM callbacks for BE get called in that
> mode?


IIRC everything was handled with DAPM, changing pin states would enable
data transfers. Not 100% sure and that's not really relevant anyways,
you did have a point that the SoundWire BEs are not correctly configured.