[alsa-devel] [PATCH] ASoC: Intel: sst: Delay machine device creation until after initialization

[Stephan Gerhold]

On Sun, Dec 16, 2018 at 07:49:56PM +0100, Stephan Gerhold wrote:
> Right now, the machine devices are created early, before the
> SST context is initialized. This means that SST might not be
> fully initialized if sst_acpi_probe() fails later on (e.g. after
> sst_platform_get_resources() if the IRQ does not exist).
> 
> However, at least sst-mfld-platform assumes that sst_register_dsp()
> was called to set the (global) "sst" device pointer, which happens
> only later in sst_acpi_probe() when sst_context_init() is called.
> This may cause a NULL pointer dereference later when the ALSA device
> is first opened:
> 
>   BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
>   PGD 0 P4D 0
>   Oops: 0000 [#1] PREEMPT SMP PTI
>   CPU: 0 PID: 790 Comm: pulseaudio Not tainted 4.20.0-rc6-mainline-00161-g6531e115b7ab #1
>   Hardware name: ASUSTeK COMPUTER INC. ME176C/ME176C, BIOS 5.6.5 09/16/2015
>   RIP: 0010:sst_handle_vb_timer+0x61/0x1b0 [snd_soc_sst_atom_hifi2_platform]
>   Code: 44 24 04 e9 84 00 00 00 31 c9 c7 04 24 ff ff ff ff 66 89 4c 24 06 84 db 0f 84 90 00 00 00 48 8b 05 c4 23 01 00 be 01 00 00 00 <48> 8b 78 08 48 8b 40 10 48 8b 40 48 e8 2e 5e d7 f0 89 c3 85 c0 78
>   RSP: 0018:ffff9d968099fa30 EFLAGS: 00010202
>   RAX: 0000000000000000 RBX: 0000000000000001 RCX: 0000000000000000
>   RDX: 0000000080000001 RSI: 0000000000000001 RDI: 00000000ffffffff
>   RBP: ffff968d33384618 R08: 0000000000000001 R09: 00000000000002e3
>   R10: ffff968d333a0800 R11: 0000000000000000 R12: ffff968d34bc7c00
>   R13: ffff968d333a3eb0 R14: 0000000000000001 R15: ffff968d333a08c0
>   FS:  00007f63a7e7b200(0000) GS:ffff968d37600000(0000) knlGS:0000000000000000
>   CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>   CR2: 0000000000000008 CR3: 00000000297b8000 CR4: 00000000001006f0
>   Call Trace:
>    sst_enable_ssp+0x24/0x40 [snd_soc_sst_atom_hifi2_platform]
>    soc_pcm_open+0xeb/0x960 [snd_soc_core]
>    ? __debugfs_create_file+0xcd/0x120
>    dpcm_be_dai_startup+0x183/0x3c0 [snd_soc_core]
>    dpcm_fe_dai_open+0x10c/0xab0 [snd_soc_core]
>    snd_pcm_open_substream+0x7f/0x140 [snd_pcm]
>    snd_pcm_open+0xe6/0x220 [snd_pcm]
>    ? wake_up_q+0x70/0x70
>    snd_pcm_playback_open+0x3d/0x70 [snd_pcm]
>    chrdev_open+0xa3/0x1b0
>    ? cdev_put.part.0+0x20/0x20
>    do_dentry_open+0x12f/0x350
>    path_openat+0x2d1/0x14e0
>    ? inotify_handle_event+0x17b/0x1e0
>    do_filp_open+0x93/0x100
>    ? snd_card_file_remove+0x14b/0x170 [snd]
>    ? __check_object_size+0x102/0x189
>    ? _raw_spin_unlock+0x12/0x30
>    do_sys_open+0x186/0x210
>    do_syscall_64+0x55/0x160
>    entry_SYSCALL_64_after_hwframe+0x44/0xa9
>   RIP: 0033:0x7f63a992f4c2
>   Code: 25 00 00 41 00 3d 00 00 41 00 74 4c 48 8d 05 85 70 0d 00 8b 00 85 c0 75 6d 89 f2 b8 01 01 00 00 48 89 fe bf 9c ff ff ff 0f 05 <48> 3d 00 f0 ff ff 0f 87 a2 00 00 00 48 8b 4c 24 28 64 48 33 0c 25
>   RSP: 002b:00007ffe71196b70 EFLAGS: 00000246 ORIG_RAX: 0000000000000101
>   RAX: ffffffffffffffda RBX: 0000000000080802 RCX: 00007f63a992f4c2
>   RDX: 0000000000080802 RSI: 00007ffe71196d20 RDI: 00000000ffffff9c
>   RBP: 00000000ffffffff R08: 0000000000000000 R09: 0000000000000000
>   R10: 0000000000000000 R11: 0000000000000246 R12: 00007ffe71196c00
>   R13: 0000000000000004 R14: 00007ffe71196d20 R15: 0000558466055a80
>   CR2: 0000000000000008
>   ---[ end trace 34534a02650ee26c ]---
> 
> This can be avoided if the machine device creation is delayed
> in sst_acpi_probe() until after sst_context_init(), when
> sst_register_dsp() is guaranteed to have already been called.
> 
> Signed-off-by: Stephan Gerhold <stephan at gerhold.net>
> ---
> An other option to fix this would be to add proper NULL checks
> in the probe method of sst-mfld-platform and/or sst_enable_ssp().
> Maybe this should be done additionally, but at least in my opinion
> there is not much point in registering the machine devices if they
> end up being broken anyway.
> 
>  sound/soc/intel/atom/sst/sst_acpi.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/sound/soc/intel/atom/sst/sst_acpi.c b/sound/soc/intel/atom/sst/sst_acpi.c
> index ac542535b9d5..493d32923815 100644
> --- a/sound/soc/intel/atom/sst/sst_acpi.c
> +++ b/sound/soc/intel/atom/sst/sst_acpi.c
> @@ -345,6 +345,18 @@ static int sst_acpi_probe(struct platform_device *pdev)
>  	mach->mach_params.acpi_ipc_irq_index =
>  		pdata->res_info->acpi_ipc_irq_index;
>  
> +	/* Fill sst platform data */
> +	ctx->pdata = pdata;
> +	strcpy(ctx->firmware_name, mach->fw_filename);
> +
> +	ret = sst_platform_get_resources(ctx);
> +	if (ret)
> +		return ret;
> +
> +	ret = sst_context_init(ctx);
> +	if (ret < 0)
> +		return ret;
> +
>  	plat_dev = platform_device_register_data(dev, pdata->platform, -1,
>  						NULL, 0);
>  	if (IS_ERR(plat_dev)) {
> @@ -365,18 +377,6 @@ static int sst_acpi_probe(struct platform_device *pdev)
>  		return PTR_ERR(mdev);
>  	}
>  
> -	/* Fill sst platform data */
> -	ctx->pdata = pdata;
> -	strcpy(ctx->firmware_name, mach->fw_filename);
> -
> -	ret = sst_platform_get_resources(ctx);
> -	if (ret)
> -		return ret;
> -
> -	ret = sst_context_init(ctx);
> -	if (ret < 0)
> -		return ret;
> -
>  	sst_configure_runtime_pm(ctx);
>  	platform_set_drvdata(pdev, ctx);
>  	return ret;
> -- 
> 2.20.0
> 

Hi,

Mark's mail on the other thread ("ASoC: Intel: sst: Missing IRQ at index 
5 on BYT-T device") just reminded me that this patch is still open.

With "ASoC: Intel: sst: Fallback to BYT-CR if IRQ 5 is missing" the
initialization failure is solved for my device, and it does no longer
run into this BUG. However, the NULL pointer dereference is still 
possible if another device has no or an invalid IRQ listed, causing
SST initialization to fail.

This patch is one way to avoid it.

Let me know if I should re-send the patch (in case you cannot find it 
anymore). :)

Thanks,
Stephan