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

[Takashi Iwai]

On Mon, 09 Aug 2021 16:26:51 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> 
> >>> 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?

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.

> 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?


Takashi