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

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

>>>>>> The trigger callback is handled in the stream lock atomically,
>>>>>> and are you sure that you want to operate a possibly heavy task there?
>>>>>
>>>>> It's a good objection that we didn't think of.
>>>>
>>>> Doesn't Intel use non atomic trigger to send IPCs which anyway
>>>> involve code which can sleep..?
>>>
>>> sof_sdw.c doesn't seem setting it?
>>
>> Yes I think init_dai_link() should set it. Maybe Pierre/Bard would know why.
> 
> init_dai_link() is to assign dai link elements only. No IPC is needed.

The 'nonatomic' concept is only used for an FE dailink which expose a
PCM device:

soc-pcm.c:	pcm->nonatomic = rtd->dai_link->nonatomic;

Setting a BE dailink as 'nonatomic' would not accomplish much since BEs
use the 'no_pcm' option.

So the question is: is there any issue with sending an IPC in a DAI
trigger callback? This is not very different from sending a command on a
bus btw, I see a similar example for SLIMbus devices:

wcd9335.c:      case SNDRV_PCM_TRIGGER_SUSPEND:
wcd9335.c-      case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
wcd9335.c-              slim_stream_unprepare(dai_data->sruntime);
wcd9335.c-              slim_stream_disable(dai_data->sruntime);

int slim_stream_unprepare(struct slim_stream_runtime *stream)
{
	int i;

	for (i = 0; i < stream->num_ports; i++)
		slim_disconnect_port(stream, &stream->ports[i]);

static int slim_disconnect_port(struct slim_stream_runtime *stream,
				struct slim_port *port)
{
	struct slim_device *sdev = stream->dev;
	u8 wbuf[1];
	struct slim_val_inf msg = {0, 1, NULL, wbuf, NULL};
	u8 mc = SLIM_MSG_MC_DISCONNECT_PORT;
	DEFINE_SLIM_LDEST_TXN(txn, mc, 5, stream->dev->laddr, &msg);

	wbuf[0] = port->id;
	port->ch.state = SLIM_CH_STATE_DISCONNECTED;
	port->state = SLIM_PORT_DISCONNECTED;

	return slim_do_transfer(sdev->ctrl, &txn);
}

Such commands may take time...