Skip to content

Use single irq for IPC, stream, and sdw interrupts - #1525

Closed
bardliao wants to merge 59 commits into
thesofproject:integration/soundwire-debug-fixesfrom
bardliao:sdw-one-irq
Closed

bardliao wants to merge 59 commits into
thesofproject:integration/soundwire-debug-fixesfrom
bardliao:sdw-one-irq

Conversation

@bardliao

Copy link
Copy Markdown
Collaborator

The existing code uses three handlers for a shared edge-based MSI interrupts.
In corner cases, interrupts are lost, leading to IPC timeouts. Those
timeouts do not appear in legacy mode.

This PR merges the all handlers into a single one.

The first commit is the same as #1483

plbossart and others added 30 commits November 18, 2019 14:36
When a Slave device becomes synchronized with the bus, it may report
its presence in PING frames, as well as optionally asserting an
in-band PREQ signal.

The bus driver will detect a new Device0, start the enumeration
process and assign it a non-zero device number. The SoundWire
enumeration provides an arbitration to deal with multiple Slaves
reporting ATTACHED at the same time. The bus driver will also invoke
the driver .probe() callback associated with this device. The probe()
depends on the Linux device core, which handles the match operations
and may result in modules being loaded.

Once the non-zero device number is programmed, the Slave will report
its new status in PING frames and the Master hardware will typically
report this status change with an interrupt. At this point, the
.update_status() callback of the codec driver will be invoked (usually
from an interrupt thread or workqueue scheduled from the interrupt
thread).

The first race condition which can happen is between the .probe(),
which allocates the resources, and .update_status() where
initializations are typically handled. The .probe() is only called
once during the initial boot, while .update_status() will be called
for every bus hardware reset and if the Slave device loses
synchronization (an unlikely event but with non-zero probability).

The time difference between the end of the enumeration process and a
change of status reported by the hardware may be as small as one
SoundWire PING frame. The scheduling of the interrupt thread, which
invokes .update_status() is not deterministic, but can be small enough
to create a race condition. With a 48 kHz frame rate and ideal
scheduling cases, the .probe() may be pre-empted within double-digit
microseconds.

Since there is no guarantee that the .probe() completes by the time
.update_status() is invoked as a result of an interrupt, it's not
unusual for the .update_status() to rely on data structures that have
not been allocated yet, leading to kernel oopses.

This patch adds a probe_complete utility, which is used in the
sdw_update_slave_status() routine. The codec driver does not need to
do anything and can safely assume all resources are allocated in its
update_status() callback.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
When the Master starts the bus (be it during the initial boot or
system resume), it usually performs a HardReset to make sure
electrical levels are correct, then enables the control channel.

While the PM framework guarantees that the Slave devices will only
become 'active' once the Master completes the bus initialization,
there is still a risk of a race condition: the Slave enumeration is
handled in a separate interrupt thread triggered by hardware status
changes, so the Slave device may not be ready to accept commands when
the Slave driver tries to access the registers and restore settings in
its resume or pm_runtime_resume callbacks. In those cases, any
read/write commands from/to the Slave device will result in a timeout.

This patch adds an enumeration_complete structure. When the bus is
goes through a HardReset sequence and restarted, the Slave will be
marked as UNATTACHED, which will result in a call to
init_completion().

When the Slave reports its presence during PING frames as a non-zero
Device, the Master hardware will issue an interrupt and the bus driver
will invoke complete(). The order between init_completion()/complete()
is predictable since this is a Master-initiated transition.

The Slave driver may use wait_for_completion() in its resume callback.
When regmap is used, the Slave driver will typically set its regmap in
cache-only mode on suspend, then on resume block on
wait_for_completion(&enumeration_complete) to guarantee it is safe to
start read/write transactions. It may then exit the cache-only mode
and use a regmap_sync to restore settings. All these steps are
optional, their use completely depends on the Slave device
capabilities and how the Slave driver is implemented.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Slave drivers may have different ways of handling their settings, with
or without regmap.

During the integration of codec drivers, done in partnership between
Intel and Realtek, it became desirable to implement a predictable
order between low-level initializations performed in .update_status()
(invoked by an interrupt thread) and the settings restored in the
resume steps (invoked by the PM core).

This patch builds on the previous solution to wait for the Slave
device to be fully enumerated. The complete() in this case is signaled
not before the .update_status() is called, but after .update_status()
returns. Without this patch, the settings were not properly restored,
leading to timing-dependent 'no sound after resume' or 'no headset
detected after resume' bug reports.

Depending on how initialization is handled, a Slave device driver may
wait for enumeration_complete, or for initialization_complete, both
are valid synchronization points. They are initialized at the same
time, they only differ on when complete() is invoked.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
…nces

The Slave device initialization can be split in 4 different cases:

1. Master-initiated hardware reset, system suspend-resume and
pm_runtime based on clock-stop mode1. To avoid timeouts and a bad
audio experience, the Slave device resume operations need to wait for
the Slave device to be re-enumerated and its settings restored.

2. Exit from clock-stop mode0. In this case, the Slave device is
required to remain enumerated and its context preserved while the
clock is stopped, so no re-initialization or wait_for_completion() is
necessary.

3. Slave-initiated pm_runtime D3 transition. With the parent child
relationship, it is possible that a Slave device becomes 'suspended'
while its parent is still 'active' with the bus clock still
toggling. In this case, during the pm_runtime resume operation, there
is no need to wait for any settings to be restored.

4. Slave reset (sync loss or implementation-defined). In that case the
bus remains operational and the Slave device will be re-initialized
when it becomes ATTACHED again.

In previous patches, we suggested the use of wait_for_completion() to
deal with the case #1, but case #2 and #3 do not need any wait.

To account for those differences, this patch adds an unattach_request
field. The field is explicitly set by the Master for the case #1, and
if non-zero the Slave device shall wait on resume. In all other cases,
the Slave resume operations can proceed without wait.

The only request tracked so far is Master HardReset, but the request
is declared as a bit mask for future extensions (if needed). The
definition for this value is added in bus.h and does not need to be
exposed in sdw.h

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The current interfaces between ASoC and SoundWire are limited by the
platform_device infrastructure to an init() and exit() (mapped to the
platform driver.probe and .remove)

To help with the platform detection, machine driver selection and
management of power dependencies between DSP and SoundWire IP, the
ASoC side requires:

a) an ACPI scan helper, to report if any devices are exposed in the
DSDT tables, and if any links are disabled by the BIOS.

b) a probe helper that allocates the resources without actually
starting the bus.

c) a startup helper which does start the bus when all power
dependencies are settled.

d) an exit helper to free all resources

e) an interrupt_enable/disable helper, typically invoked after the
startup helper but also used in suspend routines.

This patch moves all required interfaces to sdw_intel.h, mainly to
allow SoundWire and ASoC parts to be merged separately once the header
files are shared between trees.

To avoid compilation issues, the conflicts in intel_init.c are blindly
removed. This would in theory prevent the code from working, but since
there are no users of the Intel Soundwire driver this has no
impact. Functionality will be restored when the removal of platform
devices is complete.

Support for SoundWire + SOF builds will only be provided once all the
required pieces are upstream.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
…erations

The SoundWire DAIs for Intel platform are created in
drivers/soundwire/intel.c, while the communication with the Intel DSP
is all controlled in soc/sof/intel

When the DAI status changes, a callback is used to bridge the gap
between the two subsystems.

The naming of the existing 'config_stream' callback does not map well
with any of ALSA/ASoC concepts. This patch renames it as
'params_stream' to be more self-explanatory.

A new 'free_stream' callback is added in case any resources allocated
in the 'params_stream' stage need to be released. In the SOF
implementation, this is used in the hw_free case to release the DMA
channels over IPC.

These two callbacks now rely on structures which expose the link_id
and alh_stream_id (required by the firmware IPC), instead of a list of
parameters. The 'void *' definitions are changed to use explicit
types, as suggested on alsa-devel during earlier reviews.

Signed-off-by: Rander Wang <rander.wang@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Add clearer references to sdw_slave_driver for internal macros

No change for sdw_driver and module_sdw_driver to avoid compatibility
issues with existing codec devices

No functionality change.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Since we want to introduce master devices, rename macro so that we
have consistency between slave and master device access, following the
Grey Bus example.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Align with previous renames and shorten macro

No functionality change

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Before we add master driver support, make sure there is no ambiguity
and no occurrences of sdw_drv_ functions.

No functionality change.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
There are too many fields called 'res' so add prefix to make it easier
to track what the structures are.

Pure rename, no functionality change

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Currently the bus does not have any explicit support for master
devices.

First add explicit support for sdw_slave_type and error checks if this type
is not set.

In follow-up patches we can add support for the sdw_md_type (md==Master
Device), following the Grey Bus example.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Currently the code deals with uevents at the bus level, but we only care
for Slave events

Suggested-by: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Since we want an explicit support for the SoundWire Master device, add
the definitions, following the Grey Bus example.

Unlike for the Slave device, we do not set a variable when dealing
with the master uevent.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Use sdw_master_device and driver instead of platform devices

To quote GregKH:

"Don't mess with a platform device unless you really have no other
possible choice. And even then, don't do it and try to do something
else. Platform devices are really abused, don't perpetuate it "

In addition, rather than a plain-vanilla init/exit, this patch
provides 3 steps in the initialization (ACPI scan, probe, startup)
which make it easier to verify support and allocate required resources
as early as possible, and conversely help make the startup
lighter-weight with only hardware register setup.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Setting an device driver is necessary for ASoC to register DAI
components.

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The existing code does not expose a prepare operation, which is very
much needed to deal with underflow and resume operations.

Signed-off-by: Rander Wang <rander.wang@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The existing code does not expose a trigger callback, which is very
much required for streaming.

The SoundWire stream is enabled and disabled in trigger function.

Signed-off-by: Rander Wang <rander.wang@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The sdw stream is allocated and stored in dai to share the sdw runtime
information.

Signed-off-by: Rander Wang <rander.wang@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Make sure all calls to the SoundWire stream API are done and involve
callback

Signed-off-by: Rander Wang <rander.wang@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
This function is required to enable all interrupts across all links.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The existing code uses one pair of interrupt handler/thread per link
but at the hardware level the interrupt is shared. This works fine for
legacy PCI interrupts, but leads to timeouts in MSI (Message-Signaled
Interrupt) mode, likely due to edges being lost.

This patch unifies interrupt handling for all links with a single
threaded IRQ handler. The handler is simplified to the bare minimum of
detecting a SoundWire interrupt, and the thread takes care of dealing
with interrupt sources. This partition follows the model used for the
SOF IPC on HDaudio platforms, where similar timeout issues were
noticed and doing all the interrupt handling/clearing in the thread
improved reliability/stability.

Validation results with 4 links active in parallel show a night-and-day
improvement with no timeouts noticed even during stress tests. Latency
and quality of service are not affected by the change - mostly because
events on a SoundWire link are throttled by the bus frame rate
(typically 8..48kHz).

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The driver probe takes care of basic initialization and is invoked
when a Slave becomes attached, after a match between the Slave DevID
registers and ACPI/DT entries.

The update_status callback is invoked when a Slave state changes,
e.g. when it is assigned a non-zero Device Number and it reports with
an ATTACHED/ALERT state.

The state change detection is usually hardware-based and based on the
SoundWire frame rate (e.g. double-digit microseconds) while the probe
is a pure software operation, which may involve a kernel module
load. In corner cases, it's possible that the state changes before the
probe completes.

This patch suggests the use of wait_for_completion to avoid races on
startup, so that the update_status callback does not rely on invalid
pointers/data structures.

Signed-off-by: Rander Wang <rander.wang@intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Add support for pm_runtime with the appropriate error checks for
sdw_write/read functions, e.g. when pm_runtime is not supported.

Also expose internal functions without pm_runtime support, which are
required to perform any sort of suspend/resume operation, as well as
any enumeration tasks.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
While handling the Device0, we can safely use sdw_write_no_pm.

This move will also helps us track that all other usages of
sdw_write() happen when the Slave is already enumerated.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
These routines are required for power management

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Implement suspend/resume capabilities (not runtime_pm for now)
The resume part is essentially a full-blown re-enumeration.

When S0ix is supported, we will select clock stop mode when the ACPI
target state is S0, and tear down the link for S3.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Add basic hooks in DAI .startup and .shutdown callbacks.

The SoundWire IP should be powered between those two calls. The power
dependencies between SoundWire and DSP are handled with the
parent/child relationship, before the SoundWire master device becomes
active the parent device will become active and power-up the shared
rails.

For now the strategy is to rely on complete enumeration when the
device becomes active, so the code is a copy/paste of the sequence for
system suspend/resume. In future patches, the strategy will optionally
be to rely on clock stop if the enumeration time is prohibitive or
when the devices connected to a link can signal a wake.

A module parameter is added to make integration of new Slave devices
easier, to e.g. keep the device active or prevent clock-stop.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The system resume does the entire bus re-initialization and brings it
to full-power. If the device was pm_runtime suspended, there is no
need to run the pm_runtime resume sequence after the system runtime.

Follow the documentation from runtime_pm.rst, and conditionally
disable, set_active and re-enable the device on system resume.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
When resuming, we need to re-enumerate and restart from UNATTACHED.

This will help implement a more robust state machine avoiding race
conditions on resume.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
plbossart and others added 22 commits November 18, 2019 14:36
Only keep pr_err to flag critical configuration errors that will
typically only happen during system integration.

For errors on prepare/deprepare/enable/disable, the caller can do a
much better job with more information on the DAI and device that
caused the issue.

Suggested-by: Cezary Rojewski <cezary.rojewski@intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The state machine and notes don't accurately explain or allow
transitions from STREAM_DEPREPARED and STREAM_DISABLED.

Add more explanations and allow for more transitions as a result of a
trigger_stop(), trigger_suspend() and prepare(), depending on the
ALSA/ASoC layer behavior defined by the INFO_RESUME and INFO_PAUSE
flags.

Also add basic checks to help debug inconsistent states and illegal
state machine transitions.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
We don't need to prepare the stream again if the stream is already
prepared.

sdw_prepare_stream() could be called multiple times without calling
sdw_deprepare_stream(). We call sdw_prepare_stream() in the prepare
dai ops and sdw_deprepare_stream() in the hw_free dai ops. If an xrun
happens, sdw_prepare_stream() will be called but
sdw_deprepare_stream() will not, which results in an imbalance and an
invalid total bandwidth.

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
…transition

After a system suspend, the ALSA/ASoC core will invoke the .prepare()
callback and a TRIGGER_START when INFO_RESUME is not supported.

Likewise, when an underflow occurs, the .prepare callback will be invoked.

In both cases, the stream can be in DISABLED mode, and will transition
into the PREPARED mode. We however don't want the bus bandwidth to be
recomputed.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
…suming

The .prepare() callback is invoked for normal streaming, underflows or
during the system resume transition. In the latter case, the context
for the ALH PDIs is lost, and the DSP is not initialized properly
either, but the bus parameters don't need to be recomputed.

Conversely, when doing a regular .prepare() during an underflow, the
ALH/SHIM registers shall not be changed as the hardware cannot be
reprogrammed after the DMA started (hardware spec requirement).

This patch adds storage of PDI and hw_params in the DAI dma context,
and the difference between the types of .prepare() usages is handled
via a simple boolean, updated when suspending, and tested for in the
.prepare() case.

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Add quirk and pm_runtime idle scheduling to let the Master suspend if
no Slaves become attached. This can happen when a link is not marked
as disabled and has devices exposed in the DSDT, if the power is
controlled by sideband means or the link includes a pluggable
connector.

Signed-off-by: Rander Wang <rander.wang@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
The laptop doesn't use DMIC, but use SOF.

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
When a Slave reports multiple status in the sticky bits, find the
latest configuration from the mirror of the PING frame status and
update the status directly.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
This algorithm computes bus parameters like clock frequency, frame
shape and port transport parameters based on active stream(s) running
on the bus.

Developers can also implement their own .compute_params() callback for
specific resource management algorithm, and set if before calling
sdw_add_bus_master()

Credits: this patch is based on an earlier internal contribution by
Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah. All hard-coded
values were removed from the initial contribution to use BIOS
information instead.

FIXME: remove checkpatch report
WARNING: Reusing the krealloc arg is almost always a bug
+			group->rates = krealloc(group->rates,

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Some Realtek codecs don't support grp_ctrl_valid. Set it to false
to prevent DPN_BlockCtrl2 reg write failed.

Signed-off-by: Bard Liao <bardliao@gmail.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
If the programming of the dev_number fails due to an IO error, a new
device_number will be assigned, resulting in a leak.

Make sure we only assign a device_number once per Slave device.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The current value for this field is too low and results in some
samples getting dropped and playback being faster than normal.

Increase the value to a safer value.

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Somehow Intel folks were confused, the property is 2x what the mclk
frequency actually is, as checked with the actual bus frequency.

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
useful for debug, but can be verbose so only enable if strictly needed.

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
This patch uses rtd instead of pcm at snd_soc_pcm_component_new/free()
parameter.
This is prepare for dai_link remove bug fix on topology.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
soc-topology adds extra dai_link by using snd_soc_add_dai_link(),
and removes it by snd_soc_romove_dai_link().

This snd_soc_add/remove_dai_link() and/or its related
functions are unbalanced before, and now, these are balance-uped.
But, it finds the random operation issue, and it is reported by
Pierre-Louis.

When card was released, topology will call snd_soc_remove_dai_link()
via (A).

	static void soc_cleanup_card_resources(struct snd_soc_card *card)
	{
		struct snd_soc_dai_link *link, *_link;

		/* This should be called before snd_card_free() */
	(A)	soc_remove_link_components(card);

		/* free the ALSA card at first; this syncs with pending operations */
		if (card->snd_card) {
	(B)		snd_card_free(card->snd_card);
			card->snd_card = NULL;
		}

		/* remove and free each DAI */
	(X)	soc_remove_link_dais(card);

		for_each_card_links_safe(card, link, _link)
	(C)		snd_soc_remove_dai_link(card, link);

		...
	}

At (A), topology calls snd_soc_remove_dai_link().
Then topology rtd, and its related all data are freed.

Next, (B) is called, and then, pcm->private_free = soc_pcm_private_free()
is called.

	static void soc_pcm_private_free(struct snd_pcm *pcm)
	{
		struct snd_soc_pcm_runtime *rtd = pcm->private_data;

		/* need to sync the delayed work before releasing resources */
		flush_delayed_work(&rtd->delayed_work);
		snd_soc_pcm_component_free(rtd);
	}

Here, it gets rtd via pcm->private_data.
But, topology related rtd are already freed at (A).
Normal sound card has no damage, becase it frees rtd at (C).

These are finalizing rtd related data.
Thus, these should be called when rtd was freed, not sound card
was freed. It is very natural and understandable.

In other words, pcm->private_free = soc_pcm_private_free()
is no longer needed.

Extra issue is that there is zero chance to call
soc_remove_dai() for topology related dai at (X).
Because (A) removes rtd connection from card too, and,
(X) is based on card connected rtd.

This means, (X) need to be called before (C) (= for normal sound)
and (A) (= for topology).

Now, I want to focus this patch which is the reason why
snd_card_free() = (B) is located there.

	commit 4efda5f
	("ASoC: Fix use-after-free at card unregistration")

Original snd_card_free() was called last of this function.
But moved to top to avoid use-after-free issue.
The issue was happen at soc_pcm_free() which was pcm->private_free,
today it is updated/renamed to soc_pcm_private_free().

In other words, (B) need to be called before (C) (= for normal sound)
and (A) (= for topology), because it needs (not yet freed) rtd.
But, (A) need to be called before (B),
because it needs card->snd_card pointer.

If we call flush_delayed_work() and snd_soc_pcm_component_free()
(= same as soc_pcm_private_free()) when rtd was freed (= (C), (A)),
there is no reason to call snd_card_free() at top of this function.
It can be called end of this function, again.

But, in such case, it will likely break unbind again, as Takashi-san
reported. When unbind is performed in a busy state, the code may
release still-in-use resources.
At least we need to call snd_card_disconnect_sync() at the first place.

The final code will be...

	static void soc_cleanup_card_resources(struct snd_soc_card *card)
	{
		struct snd_soc_dai_link *link, *_link;

		if (card->snd_card)
	(Z)		snd_card_disconnect_sync(card->snd_card);

	(X)	soc_remove_link_dais(card);
	(A)	soc_remove_link_components(card);

		for_each_card_links_safe(card, link, _link)
	(C)		snd_soc_remove_dai_link(card, link);

		...
		if (card->snd_card) {
	(B)		snd_card_free(card->snd_card);
			card->snd_card = NULL;
		}
	}

To avoid release still-in-use resources,
call snd_card_disconnect_sync() at (Z).

(X) is needed for both non-topology and topology.

    topology removes rtd via (A), and
non topology removes rtd via (C).

snd_card_free() is no longer related to use-after-free issue.
Thus, locating (B) is no problem.

Fixes: df95a16 ("ASoC: soc-core: fix RIP warning on card removal")
Fixes: bc7a909 ("ASoC: soc-core: add soc_unbind_dai_link()")
Reported-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Tested-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
…andlers

The existing code uses two handlers for a shared edge-based MSI interrupts.
In corner cases, interrupts are lost, leading to IPC timeouts. Those
timeouts do not appear in legacy mode.

This patch merges the two handlers into a single one. Follow-up patches
will extend this work to SoundWire interrupts which are also shared with
IPC and stream interrupts.

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Now we have a single irq for SOF interrupts. We can further merge
sof and sdw irqs.

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
@bardliao
bardliao requested a review from lgirdwood as a code owner November 21, 2019 06:38
@plbossart
plbossart force-pushed the integration/soundwire-debug-fixes branch from f867015 to 288abcc Compare November 21, 2019 23:05
@plbossart

Copy link
Copy Markdown
Member

I fixed all the conflicts, cleaned-up the code and added it in integration/soundwire-debug-fixes.

@bardliao please double-check if the changes are ok.

@plbossart plbossart closed this Nov 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants