Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SOF_HDA: need to clean-up exit sequences #299

Closed
plbossart opened this issue Nov 17, 2018 · 3 comments
Closed

SOF_HDA: need to clean-up exit sequences #299

plbossart opened this issue Nov 17, 2018 · 3 comments
Assignees
Labels
enhancement New feature or request P1 Blocker bugs or important features

Comments

@plbossart
Copy link
Member

plbossart commented Nov 17, 2018

we are missing a lot of hdac library calls, such as snd_hdac_ext_bus_device_remove(bus), both in the .remove calls as well as in error handling.

The goal is to release ALL the resources allocated, no exceptions. This will be mandatory for upstream.

@mengdonglin mengdonglin added the enhancement New feature or request label Nov 22, 2018
@jocelyn-li jocelyn-li assigned bardliao and unassigned keyonjie Nov 26, 2018
@mengdonglin mengdonglin added bug Something isn't working P1 Blocker bugs or important features and removed bug Something isn't working labels Nov 26, 2018
@bardliao
Copy link
Collaborator

@plbossart It seems we didn't use hdac stream control APIs. I think that's why we don't use snd_hdac_ext_stop_streams(). And hda_dsp_stream_free() do things similar to snd_hdac_stream_free_all() + snd_hdac_bus_free_stream_pages()

snd_hdac_bus_free_stream_pages()
                bus->io_ops->dma_free_pages(bus, &s->bdl);
                bus->io_ops->dma_free_pages(bus, &bus->rb);
                bus->io_ops->dma_free_pages(bus, &bus->posbuf);
snd_hdac_stream_free_all()
                snd_hdac_ext_stream_decouple(bus, stream, false);
                list_del(&s->list);
                kfree(stream);
hda_dsp_stream_free()
                snd_dma_free_pages(&bus->posbuf);
                snd_dma_free_pages(&bus->rb);
                snd_dma_free_pages(&s->bdl);
		list_del(&s->list);
		devm_kfree(sdev->dev, stream);

The only thing we missed here is snd_hdac_ext_stream_decouple(). But I don't know if we need to do it or it is done in somewhere else.

void snd_hdac_ext_stream_decouple(struct hdac_bus *bus,
                                struct hdac_ext_stream *stream, bool decouple)
{
        struct hdac_stream *hstream = &stream->hstream;
        u32 val;
        int mask = AZX_PPCTL_PROCEN(hstream->index);

        spin_lock_irq(&bus->reg_lock);
        val = readw(bus->ppcap + AZX_REG_PP_PPCTL) & mask;

        if (decouple && !val)
                snd_hdac_updatel(bus->ppcap, AZX_REG_PP_PPCTL, mask, mask);
        else if (!decouple && val)
                snd_hdac_updatel(bus->ppcap, AZX_REG_PP_PPCTL, mask, 0);

        stream->decoupled = decouple;
        spin_unlock_irq(&bus->reg_lock);
}

snd_hdac_ext_bus_exit() and snd_hdac_link_free_all() are add in #311

@plbossart
Copy link
Member Author

snd_hdac_ext_stream_decouple() is used in hda-dai.c

snd_hdac_ext_stream_decouple(bus, stream, false);

I have absolutely no idea why it's only called on TRIGGER_SUSPEND, makes no sense to me.

@bardliao
Copy link
Collaborator

bardliao commented Dec 3, 2018

@plbossart I just checked snd_hdac_ext_stream_decouple(). I believe that we need to call snd_hdac_ext_stream_decouple(false) in hda_dsp_stream_free(). I didn't get SNDRV_PCM_TRIGGER_SUSPEND cmd even when the system is suspended. And hdac_ext_link_stream_assign() will call snd_hdac_ext_stream_decouple(true) when system boots up.
I will send a PR for it tomorrow.

plbossart pushed a commit that referenced this issue Nov 29, 2022
In register_synth_event(), if set_synth_event_print_fmt() failed, then
both trace_remove_event_call() and unregister_trace_event() will be
called, which means the trace_event_call will call
__unregister_trace_event() twice. As the result, the second unregister
will causes the wild-memory-access.

register_synth_event
    set_synth_event_print_fmt failed
    trace_remove_event_call
        event_remove
            if call->event.funcs then
            __unregister_trace_event (first call)
    unregister_trace_event
        __unregister_trace_event (second call)

Fix the bug by avoiding to call the second __unregister_trace_event() by
checking if the first one is called.

general protection fault, probably for non-canonical address
	0xfbd59c0000000024: 0000 [#1] SMP KASAN PTI
KASAN: maybe wild-memory-access in range
[0xdead000000000120-0xdead000000000127]
CPU: 0 PID: 3807 Comm: modprobe Not tainted
6.1.0-rc1-00186-g76f33a7eedb4 #299
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
rel-1.15.0-0-g2dd4b9b3f840-prebuilt.qemu.org 04/01/2014
RIP: 0010:unregister_trace_event+0x6e/0x280
Code: 00 fc ff df 4c 89 ea 48 c1 ea 03 80 3c 02 00 0f 85 0e 02 00 00 48
b8 00 00 00 00 00 fc ff df 4c 8b 63 08 4c 89 e2 48 c1 ea 03 <80> 3c 02
00 0f 85 e2 01 00 00 49 89 2c 24 48 85 ed 74 28 e8 7a 9b
RSP: 0018:ffff88810413f370 EFLAGS: 00010a06
RAX: dffffc0000000000 RBX: ffff888105d050b0 RCX: 0000000000000000
RDX: 1bd5a00000000024 RSI: ffff888119e276e0 RDI: ffffffff835a8b20
RBP: dead000000000100 R08: 0000000000000000 R09: fffffbfff0913481
R10: ffffffff8489a407 R11: fffffbfff0913480 R12: dead000000000122
R13: ffff888105d050b8 R14: 0000000000000000 R15: ffff888105d05028
FS:  00007f7823e8d540(0000) GS:ffff888119e00000(0000)
knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f7823e7ebec CR3: 000000010a058002 CR4: 0000000000330ef0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 <TASK>
 __create_synth_event+0x1e37/0x1eb0
 create_or_delete_synth_event+0x110/0x250
 synth_event_run_command+0x2f/0x110
 test_gen_synth_cmd+0x170/0x2eb [synth_event_gen_test]
 synth_event_gen_test_init+0x76/0x9bc [synth_event_gen_test]
 do_one_initcall+0xdb/0x480
 do_init_module+0x1cf/0x680
 load_module+0x6a50/0x70a0
 __do_sys_finit_module+0x12f/0x1c0
 do_syscall_64+0x3f/0x90
 entry_SYSCALL_64_after_hwframe+0x63/0xcd

Link: https://lkml.kernel.org/r/20221117012346.22647-3-shangxiaojing@huawei.com

Fixes: 4b14793 ("tracing: Add support for 'synthetic' events")
Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com>
Cc: stable@vger.kernel.org
Cc: <mhiramat@kernel.org>
Cc: <zanussi@kernel.org>
Cc: <fengguang.wu@intel.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request P1 Blocker bugs or important features
Projects
None yet
Development

No branches or pull requests

4 participants