linux-kernel - Re: [PATCH] soundwire: stream: Use polling for DP Prepare completion

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1d340be7-2f21-73a5-bcc3-91372976dfb9@linux.intel.com>
Date:   Thu, 17 Jun 2021 11:53:57 -0500
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Richard Fitzgerald <rf@...nsource.cirrus.com>, vkoul@...nel.org,
        yung-chuan.liao@...ux.intel.com, sanyog.r.kale@...el.com
Cc:     alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
        patches@...nsource.cirrus.com
Subject: Re: [PATCH] soundwire: stream: Use polling for DP Prepare completion

Thanks Richard for the patch, very interesting.

> sdw_prep_deprep_slave_ports() cannot use the port interrupt to signal
> CP_SM completion because it is called while holding the bus lock, which
> blocks the alert handler from running.
> 
> Change to polling the PREPARESTATUS register and remove the
> infrastructure that was implemented for the previous interrupt waiting.

I am afraid the entire PORT_READY support is completely untested at the moment: all the existing codecs use the simpler state machine, e.g. 		

dpn[i].simple_ch_prep_sm = true;

So my main question is: how did you test this change? Is this on a platform already supported upstream? yes/no answer is fine, no details necessary.

I am also not fully clear on the locking issue. 

Is the problem that sdw_handle_slave_status() uses a mutex_lock(&bus->bus_lock), which is already held in sdw_prepare_stream

int sdw_prepare_stream(struct sdw_stream_runtime *stream)
{
[...]
	sdw_acquire_bus_lock(stream);

[...]
	ret = _sdw_prepare_stream(stream, update_params);

	sdw_release_bus_lock(stream);
	return ret;
}

so dealing with an ALERT status while doing pre-deprep would cause a self-inflicted deadlock? 

If yes, that's a more general problem that would need to be fixed. this lock is taken in ALL stream operations, not just prepare/deprepare.

If e.g. a jack detection was signaled during any stream reconfiguration, we would also not be able to deal with the information, would we?

the purpose of the lock in sdw_handle_slave_status() was to check if devices attach or fall-off the bus before handling their status. The command/control protocol is always operational so nothing prevents the interrupt from being handled.

There's also something odd about directly polling on the status bits. The status bits will be used to signal the ALERT condition which is transmitted to the host during PING frames. This solution would result in the host noticing an interrupt: host controllers typically have a sticky status where all changes in PING frames are detected and used as a trigger for interrupt processing. In this case the interrupt would still be handled, but the sdw_handle_slave_status() would be deferred until the stream prepare is complete, and at that point the interrupt processing would not see any sources. It's not illegal but it's a bit odd to cause empty interrupts to be handled. 

It might be a better solution to fix conflicts between stream reconfiguration and interrupts. I don't have a turn-key proposal but the suggested patch feels like a work-around.
maybe using mutex_is_locked()?

If we can't figure something out, then at least the PORT_READY mask should not be set, i.e. this code would need to be removed:

int sdw_configure_dpn_intr(struct sdw_slave *slave,
			   int port, bool enable, int mask)
{
...
	/* Set/Clear port ready interrupt mask */
	if (enable) {
		val |= mask;
		val |= SDW_DPN_INT_PORT_READY;
	} else {
		val &= ~(mask);
		val &= ~SDW_DPN_INT_PORT_READY;
	}

	ret = sdw_update(slave, addr, (mask | SDW_DPN_INT_PORT_READY), val);
}

> A new configuration field 'ch_prep_poll_us' is added to struct
> sdw_dpn_prop so that the peripheral driver may select a polling interval.
> If this is left at zero a default interval of 500 usec is used.

we already have a 'ch_prep_timeout' that's defined, do you need to redefine a new field? why not just read once at the end of that timeout? It's a prepare operation, there's no requirement to be fast, is there?

Those properties were supposed to be populated in platform firmware btw, we should try and limit what properties need to be set in a codec driver.

> +		prep_timeout_us = dpn_prop->ch_prep_timeout * USEC_PER_MSEC;

I also just noticed that we don't have a default for ch_prep_timeout, that's a bug.

> +		if (dpn_prop->ch_prep_poll_us)
> +			prep_poll_us = dpn_prop->ch_prep_poll_us;
> +		else
> +			prep_poll_us = SDW_DEFAULT_DP_PREP_POLL_US;

Thanks
-Pierre