2022-04-21 22:40:59

by Neil Armstrong

[permalink] [raw]
Subject: [PATCH 1/2] Revert "ASoC: meson: axg-tdm-interface: manage formatters in trigger"

This reverts commit bf5e4887eeddb48480568466536aa08ec7f179a5 because
the following and required commit e138233e56e9829e65b6293887063a1a3ccb2d68
causes the following system crash when using audio:
BUG: sleeping function called from invalid context at kernel/locking/mutex.c:282

Reported-by: Dmitry Shmidt <[email protected]>
Signed-off-by: Neil Armstrong <[email protected]>
---
sound/soc/meson/axg-tdm-interface.c | 26 +++++---------------------
1 file changed, 5 insertions(+), 21 deletions(-)

diff --git a/sound/soc/meson/axg-tdm-interface.c b/sound/soc/meson/axg-tdm-interface.c
index 0c31934a9630..e076ced30025 100644
--- a/sound/soc/meson/axg-tdm-interface.c
+++ b/sound/soc/meson/axg-tdm-interface.c
@@ -351,29 +351,13 @@ static int axg_tdm_iface_hw_free(struct snd_pcm_substream *substream,
return 0;
}

-static int axg_tdm_iface_trigger(struct snd_pcm_substream *substream,
- int cmd,
+static int axg_tdm_iface_prepare(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)
{
- struct axg_tdm_stream *ts =
- snd_soc_dai_get_dma_data(dai, substream);
-
- switch (cmd) {
- case SNDRV_PCM_TRIGGER_START:
- case SNDRV_PCM_TRIGGER_RESUME:
- case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
- axg_tdm_stream_start(ts);
- break;
- case SNDRV_PCM_TRIGGER_SUSPEND:
- case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
- case SNDRV_PCM_TRIGGER_STOP:
- axg_tdm_stream_stop(ts);
- break;
- default:
- return -EINVAL;
- }
+ struct axg_tdm_stream *ts = snd_soc_dai_get_dma_data(dai, substream);

- return 0;
+ /* Force all attached formatters to update */
+ return axg_tdm_stream_reset(ts);
}

static int axg_tdm_iface_remove_dai(struct snd_soc_dai *dai)
@@ -413,8 +397,8 @@ static const struct snd_soc_dai_ops axg_tdm_iface_ops = {
.set_fmt = axg_tdm_iface_set_fmt,
.startup = axg_tdm_iface_startup,
.hw_params = axg_tdm_iface_hw_params,
+ .prepare = axg_tdm_iface_prepare,
.hw_free = axg_tdm_iface_hw_free,
- .trigger = axg_tdm_iface_trigger,
};

/* TDM Backend DAIs */
--
2.25.1


2022-04-22 18:55:32

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH 1/2] Revert "ASoC: meson: axg-tdm-interface: manage formatters in trigger"

On Thu, Apr 21, 2022 at 05:57:24PM +0200, Neil Armstrong wrote:
> This reverts commit bf5e4887eeddb48480568466536aa08ec7f179a5 because
> the following and required commit e138233e56e9829e65b6293887063a1a3ccb2d68
> causes the following system crash when using audio:

Please submit patches using subject lines reflecting the style for the
subsystem, this makes it easier for people to identify relevant patches.
Look at what existing commits in the area you're changing are doing and
make sure your subject lines visually resemble what they're doing.
There's no need to resubmit to fix this alone.


Attachments:
(No filename) (607.00 B)
signature.asc (499.00 B)
Download all attachments

2022-04-22 19:06:56

by Jerome Brunet

[permalink] [raw]
Subject: Re: [PATCH 1/2] Revert "ASoC: meson: axg-tdm-interface: manage formatters in trigger"


On Thu 21 Apr 2022 at 17:57, Neil Armstrong <[email protected]> wrote:

> This reverts commit bf5e4887eeddb48480568466536aa08ec7f179a5 because
> the following and required commit e138233e56e9829e65b6293887063a1a3ccb2d68
> causes the following system crash when using audio:
> BUG: sleeping function called from invalid context at kernel/locking/mutex.c:282
>
> Reported-by: Dmitry Shmidt <[email protected]>
> Signed-off-by: Neil Armstrong <[email protected]>

For both:
Acked-by: Jerome Brunet <[email protected]>

The main reason for the this was to be able to configure the start order
between the DPCM Backend and Frontend. Only the trigger() callback has
that capability for now.

This HW require the BE to start before FE, otherwise channels get randomly
shifted in the output stream if there is more than 2 slots on the link,
mainly on the capture path.

This HW require mutexes to handle the TDM formatters (because it uses
the CCF API). This why I moved to non-atomic to use trigger(),
forgetting that doing so would make period_elapsed() take a mutex from
the IRQ ... :/

To properly fix this, I'll need to extend ASoC so the prepare() callback
BE/FE call order can also be configured.

2022-04-22 20:30:24

by Neil Armstrong

[permalink] [raw]
Subject: Re: [PATCH 1/2] Revert "ASoC: meson: axg-tdm-interface: manage formatters in trigger"

Hi Mark,

On 21/04/2022 19:20, Mark Brown wrote:
> On Thu, Apr 21, 2022 at 05:57:24PM +0200, Neil Armstrong wrote:
>> This reverts commit bf5e4887eeddb48480568466536aa08ec7f179a5 because
>> the following and required commit e138233e56e9829e65b6293887063a1a3ccb2d68
>
> One other thing - these should be Fixes: tags, that helps tooling figure
> out things like backports.
>
> Also:
>
> Please include human readable descriptions of things like commits and
> issues being discussed in e-mail in your mails, this makes them much
> easier for humans to read especially when they have no internet access.
> I do frequently catch up on my mail on flights or while otherwise
> travelling so this is even more pressing for me than just being about
> making things a bit easier to read.

Thanks, I'll think of this for the next time.

Neil

2022-04-22 20:33:03

by Neil Armstrong

[permalink] [raw]
Subject: [PATCH 2/2] Revert "ASoC: meson: axg-card: make links nonatomic"

This commit e138233e56e9829e65b6293887063a1a3ccb2d68 causes the
following system crash when using audio on G12A/G12B & SM1 systems:
BUG: sleeping function called from invalid context at kernel/locking/mutex.c:282
in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 0, name: swapper/0
preempt_count: 10001, expected: 0
RCU nest depth: 0, expected: 0
Preemption disabled at:
schedule_preempt_disabled+0x20/0x2c
CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 5.17.0-rc6-03747-gd403c3588f77-dirty #957
Hardware name: SEI Robotics SEI610 (DT)
Call trace:
dump_backtrace+0xd8/0xf4
show_stack+0x18/0x30
dump_stack_lvl+0x70/0x8c
dump_stack+0x18/0x38
__might_resched+0x154/0x164
__might_sleep+0x48/0x78
mutex_lock+0x24/0x60
_snd_pcm_stream_lock_irqsave+0x20/0x3c
snd_pcm_period_elapsed+0x24/0xa4
axg_fifo_pcm_irq_block+0x64/0xdc
__handle_irq_event_percpu+0x104/0x264
handle_irq_event+0x48/0xb4
...
start_kernel+0x3f0/0x484
__primary_switched+0xc0/0xc8

Revert this commit until the crash is fixed.

Reported-by: Dmitry Shmidt <[email protected]>
Signed-off-by: Neil Armstrong <[email protected]>
---
sound/soc/meson/axg-card.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/sound/soc/meson/axg-card.c b/sound/soc/meson/axg-card.c
index cbbaa55d92a6..2b77010c2c5c 100644
--- a/sound/soc/meson/axg-card.c
+++ b/sound/soc/meson/axg-card.c
@@ -320,7 +320,6 @@ static int axg_card_add_link(struct snd_soc_card *card, struct device_node *np,

dai_link->cpus = cpu;
dai_link->num_cpus = 1;
- dai_link->nonatomic = true;

ret = meson_card_parse_dai(card, np, &dai_link->cpus->of_node,
&dai_link->cpus->dai_name);
--
2.25.1

2022-04-22 22:04:33

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH 1/2] Revert "ASoC: meson: axg-tdm-interface: manage formatters in trigger"

On Thu, Apr 21, 2022 at 05:57:24PM +0200, Neil Armstrong wrote:
> This reverts commit bf5e4887eeddb48480568466536aa08ec7f179a5 because
> the following and required commit e138233e56e9829e65b6293887063a1a3ccb2d68

One other thing - these should be Fixes: tags, that helps tooling figure
out things like backports.

Also:

Please include human readable descriptions of things like commits and
issues being discussed in e-mail in your mails, this makes them much
easier for humans to read especially when they have no internet access.
I do frequently catch up on my mail on flights or while otherwise
travelling so this is even more pressing for me than just being about
making things a bit easier to read.


Attachments:
(No filename) (719.00 B)
signature.asc (499.00 B)
Download all attachments

2022-04-22 22:38:19

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH 1/2] Revert "ASoC: meson: axg-tdm-interface: manage formatters in trigger"

On Thu, 21 Apr 2022 17:57:24 +0200, Neil Armstrong wrote:
> This reverts commit bf5e4887eeddb48480568466536aa08ec7f179a5 because
> the following and required commit e138233e56e9829e65b6293887063a1a3ccb2d68
> causes the following system crash when using audio:
> BUG: sleeping function called from invalid context at kernel/locking/mutex.c:282
>
>

Applied to

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/2] Revert "ASoC: meson: axg-tdm-interface: manage formatters in trigger"
commit: c26830b6c5c534d273ce007eb33d5a2d2ad4e969
[2/2] Revert "ASoC: meson: axg-card: make links nonatomic"
commit: 0c9b152c72e53016e96593bdbb8cffe2176694b9

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark