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