Hi Alexey,
On Wed, Oct 9, 2024 at 4:39 PM Alexey Klimov <alexey.klimov@xxxxxxxxxx> wrote:
>
> During the migration of Soundwire runtime stream allocation from
> the Qualcomm Soundwire controller to SoC's soundcard drivers the sdm845
> soundcard was forgotten.
>
> At this point any playback attempt or audio daemon startup, for instance
> on sdm845-db845c (Qualcomm RB3 board), will result in stream pointer
> NULL dereference:
>
> Unable to handle kernel NULL pointer dereference at virtual
> address 0000000000000020
> Mem abort info:
> ESR = 0x0000000096000004
> EC = 0x25: DABT (current EL), IL = 32 bits
> SET = 0, FnV = 0
> EA = 0, S1PTW = 0
> FSC = 0x04: level 0 translation fault
> Data abort info:
> ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
> CM = 0, WnR = 0, TnD = 0, TagAccess = 0
> GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> user pgtable: 4k pages, 48-bit VAs, pgdp=0000000101ecf000
> [0000000000000020] pgd=0000000000000000, p4d=0000000000000000
> Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
> Modules linked in: ...
> CPU: 5 UID: 0 PID: 1198 Comm: aplay
> Not tainted 6.12.0-rc2-qcomlt-arm64-00059-g9d78f315a362-dirty #18
> Hardware name: Thundercomm Dragonboard 845c (DT)
> pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : sdw_stream_add_slave+0x44/0x380 [soundwire_bus]
> lr : sdw_stream_add_slave+0x44/0x380 [soundwire_bus]
> sp : ffff80008a2035c0
> x29: ffff80008a2035c0 x28: ffff80008a203978 x27: 0000000000000000
> x26: 00000000000000c0 x25: 0000000000000000 x24: ffff1676025f4800
> x23: ffff167600ff1cb8 x22: ffff167600ff1c98 x21: 0000000000000003
> x20: ffff167607316000 x19: ffff167604e64e80 x18: 0000000000000000
> x17: 0000000000000000 x16: ffffcec265074160 x15: 0000000000000000
> x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000
> x11: 0000000000000000 x10: 0000000000000000 x9 : 0000000000000000
> x8 : 0000000000000000 x7 : 0000000000000000 x6 : ffff167600ff1cec
> x5 : ffffcec22cfa2010 x4 : 0000000000000000 x3 : 0000000000000003
> x2 : ffff167613f836c0 x1 : 0000000000000000 x0 : ffff16761feb60b8
> Call trace:
> sdw_stream_add_slave+0x44/0x380 [soundwire_bus]
> wsa881x_hw_params+0x68/0x80 [snd_soc_wsa881x]
> snd_soc_dai_hw_params+0x3c/0xa4
> __soc_pcm_hw_params+0x230/0x660
> dpcm_be_dai_hw_params+0x1d0/0x3f8
> dpcm_fe_dai_hw_params+0x98/0x268
> snd_pcm_hw_params+0x124/0x460
> snd_pcm_common_ioctl+0x998/0x16e8
> snd_pcm_ioctl+0x34/0x58
> __arm64_sys_ioctl+0xac/0xf8
> invoke_syscall+0x48/0x104
> el0_svc_common.constprop.0+0x40/0xe0
> do_el0_svc+0x1c/0x28
> el0_svc+0x34/0xe0
> el0t_64_sync_handler+0x120/0x12c
> el0t_64_sync+0x190/0x194
> Code: aa0403fb f9418400 9100e000 9400102f (f8420f22)
> ---[ end trace 0000000000000000 ]---
>
> 0000000000006108 <sdw_stream_add_slave>:
> 6108: d503233f paciasp
> 610c: a9b97bfd stp x29, x30, [sp, #-112]!
> 6110: 910003fd mov x29, sp
> 6114: a90153f3 stp x19, x20, [sp, #16]
> 6118: a9025bf5 stp x21, x22, [sp, #32]
> 611c: aa0103f6 mov x22, x1
> 6120: 2a0303f5 mov w21, w3
> 6124: a90363f7 stp x23, x24, [sp, #48]
> 6128: aa0003f8 mov x24, x0
> 612c: aa0203f7 mov x23, x2
> 6130: a9046bf9 stp x25, x26, [sp, #64]
> 6134: aa0403f9 mov x25, x4 <-- x4 copied to x25
> 6138: a90573fb stp x27, x28, [sp, #80]
> 613c: aa0403fb mov x27, x4
> 6140: f9418400 ldr x0, [x0, #776]
> 6144: 9100e000 add x0, x0, #0x38
> 6148: 94000000 bl 0 <mutex_lock>
> 614c: f8420f22 ldr x2, [x25, #32]! <-- offset 0x44
> ^^^
> This is 0x6108 + offset 0x44 from the beginning of sdw_stream_add_slave()
> where data abort happens.
> wsa881x_hw_params() is called with stream = NULL and passes it further
> in register x4 (5th argument) to sdw_stream_add_slave() without any checks.
> Value from x4 is copied to x25 and finally it aborts on trying to load
> a value from address in x25 plus offset 32 (in dec) which corresponds
> to master_list member in struct sdw_stream_runtime:
>
> struct sdw_stream_runtime {
> const char * name; /* 0 8 */
> struct sdw_stream_params params; /* 8 12 */
> enum sdw_stream_state state; /* 20 4 */
> enum sdw_stream_type type; /* 24 4 */
> /* XXX 4 bytes hole, try to pack */
> here-> struct list_head master_list; /* 32 16 */
> int m_rt_count; /* 48 4 */
> /* size: 56, cachelines: 1, members: 6 */
> /* sum members: 48, holes: 1, sum holes: 4 */
> /* padding: 4 */
> /* last cacheline: 56 bytes */
>
> Fix this by adding required calls to qcom_snd_sdw_startup() and
> sdw_release_stream() to startup and shutdown routines which restores
> the previous correct behaviour when ->set_stream() method is called to
> set a valid stream runtime pointer on playback startup.
>
> Reproduced and then fix was tested on db845c RB3 board.
>
> Reported-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 15c7fab0e047 ("ASoC: qcom: Move Soundwire runtime stream alloc to soundcards")
> Cc: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> Cc: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> Cc: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> Signed-off-by: Alexey Klimov <alexey.klimov@xxxxxxxxxx>
> ---
> sound/soc/qcom/sdm845.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/sound/soc/qcom/sdm845.c b/sound/soc/qcom/sdm845.c
> index 75701546b6ea..a479d7e5b7fb 100644
> --- a/sound/soc/qcom/sdm845.c
> +++ b/sound/soc/qcom/sdm845.c
> @@ -15,6 +15,7 @@
> #include <uapi/linux/input-event-codes.h>
> #include "common.h"
> #include "qdsp6/q6afe.h"
> +#include "sdw.h"
> #include "../codecs/rt5663.h"
>
> #define DRIVER_NAME "sdm845"
> @@ -416,7 +417,7 @@ static int sdm845_snd_startup(struct snd_pcm_substream *substream)
> pr_err("%s: invalid dai id 0x%x\n", __func__, cpu_dai->id);
> break;
> }
> - return 0;
> + return qcom_snd_sdw_startup(substream);
> }
>
> static void sdm845_snd_shutdown(struct snd_pcm_substream *substream)
> @@ -425,6 +426,7 @@ static void sdm845_snd_shutdown(struct snd_pcm_substream *substream)
> struct snd_soc_card *card = rtd->card;
> struct sdm845_snd_data *data = snd_soc_card_get_drvdata(card);
> struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0);
> + struct sdw_stream_runtime *sruntime = data->sruntime[cpu_dai->id];
>
> switch (cpu_dai->id) {
> case PRIMARY_MI2S_RX:
> @@ -463,6 +465,9 @@ static void sdm845_snd_shutdown(struct snd_pcm_substream *substream)
> pr_err("%s: invalid dai id 0x%x\n", __func__, cpu_dai->id);
> break;
> }
> +
> + data->sruntime[cpu_dai->id] = NULL;
> + sdw_release_stream(sruntime);
> }
>
> static int sdm845_snd_prepare(struct snd_pcm_substream *substream)
> --
> 2.45.2
>
>
Thank you so much for tracking this down! Was experiencing the same
thing on my Lenovo Yoga C630, and testing with this patch, I no longer
see the null pointer and also have working audio.
Tested-by: Steev Klimaszewski <steev@xxxxxxxx> # Lenovo Yoga C630
