remove hda checking from hda_dsp_remove and add it to hda_dsp_trace_init #521
Conversation
sdev->pdata->hw_pdata is assigned in hda_dsp_probe() so sdev->pdata->hw_pdata won't be null in hda_dsp_remove(). Signed-off-by: Bard liao <[email protected]>
sound/soc/sof/intel/hda-trace.c
Outdated
There was a problem hiding this comment.
I don't get why this is needed? the trace_init can only happen if the probe is successful, no?
There was a problem hiding this comment.
@plbossart Yes. But if we set .trace_init = hda_dsp_trace_init, in a specific struct snd_sof_dsp_ops with a .probe other than hda_dsp_probe, it will cause problem. The situation should not happen, but it may happen by mistake.
There was a problem hiding this comment.
@bardliao can we not argue the same for the first commit?
There was a problem hiding this comment.
@ranj063 Sorry, I didn't get your question. I didn't argue with anything. I was trying to explain the second patch. But I don't insist on it. It's fine for me if the second patch is not applied. :)
There was a problem hiding this comment.
@bardliao sorry please dont take it personally. All i was trying to say was that the situation that you described with .probe being somthing other than hda_dsp_probe() could lead to the same problem in hda_dsp_remove() just like in hda_dsp_trace_init().
There was a problem hiding this comment.
@ranj063 Oh yes, indeed, there is also the same risk in the first patch. However, in my opinion, the .probe and .remove ops are a pair. That's why I didn't think set .probe = hda_dsp_probe() but not .remove = hda_dsp_remove() will happen in real world. But I agree with you, we should use the same criterion for both patches. Lets revert the second patch.
The temp buffer size variable for trace_find_next_entry() was incorrectly being updated when the size did not change. The temp buffer size should only be updated when it is reallocated. This is mostly an issue when used with ftrace_dump(). That's because ftrace_dump() can not allocate a new buffer, and instead uses a temporary buffer with a fix size. But the variable that keeps track of that size is incorrectly updated with each call, and it could fall into the path that would try to reallocate the buffer and produce a warning. ------------[ cut here ]------------ WARNING: CPU: 1 PID: 1601 at kernel/trace/trace.c:3548 trace_find_next_entry+0xd0/0xe0 Modules linked in [..] CPU: 1 PID: 1601 Comm: bash Not tainted 5.9.0-rc5-test+ thesofproject#521 Hardware name: Hewlett-Packard HP Compaq Pro 6300 SFF/339A, BIOS K01 v03.03 07/14/2016 RIP: 0010:trace_find_next_entry+0xd0/0xe0 Code: 40 21 00 00 4c 89 e1 31 d2 4c 89 ee 48 89 df e8 c6 9e ff ff 89 ab 54 21 00 00 5b 5d 41 5c 41 5d c3 48 63 d5 eb bf 31 c0 eb f0 <0f> 0b 48 63 d5 eb b4 66 0f 1f 84 00 00 00 00 00 53 48 8d 8f 60 21 RSP: 0018:ffff95a4f2e8bd70 EFLAGS: 00010046 RAX: ffffffff96679fc0 RBX: ffffffff97910de0 RCX: ffffffff96679fc0 RDX: ffff95a4f2e8bd98 RSI: ffff95a4ee321098 RDI: ffffffff97913000 RBP: 0000000000000018 R08: 0000000000000000 R09: 0000000000000000 R10: 0000000000000001 R11: 0000000000000046 R12: ffff95a4f2e8bd98 R13: 0000000000000000 R14: ffff95a4ee321098 R15: 00000000009aa301 FS: 00007f8565484740(0000) GS:ffff95a55aa40000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 000055876bd43d90 CR3: 00000000b76e6003 CR4: 00000000001706e0 Call Trace: trace_print_lat_context+0x58/0x2d0 ? cpumask_next+0x16/0x20 print_trace_line+0x1a4/0x4f0 ftrace_dump.cold+0xad/0x12c __handle_sysrq.cold+0x51/0x126 write_sysrq_trigger+0x3f/0x4a proc_reg_write+0x53/0x80 vfs_write+0xca/0x210 ksys_write+0x70/0xf0 do_syscall_64+0x33/0x40 entry_SYSCALL_64_after_hwframe+0x44/0xa9 RIP: 0033:0x7f8565579487 Code: 64 89 02 48 c7 c0 ff ff ff ff eb bb 0f 1f 80 00 00 00 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 48 89 54 24 18 48 89 74 24 RSP: 002b:00007ffd40707948 EFLAGS: 00000246 ORIG_RAX: 0000000000000001 RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 00007f8565579487 RDX: 0000000000000002 RSI: 000055876bd74de0 RDI: 0000000000000001 RBP: 000055876bd74de0 R08: 000000000000000a R09: 0000000000000001 R10: 000055876bdec280 R11: 0000000000000246 R12: 0000000000000002 R13: 00007f856564a500 R14: 0000000000000002 R15: 00007f856564a700 irq event stamp: 109958 ---[ end trace 7aab5b7e51484b00 ]--- Not only fix the updating of the temp buffer, but also do not free the temp buffer before a new buffer is allocated (there's no reason to not continue to use the current temp buffer if an allocation fails). Cc: [email protected] Fixes: 8e99cf9 ("tracing: Do not allocate buffer in trace_find_next_entry() in atomic") Reported-by: Anna-Maria Behnsen <[email protected]> Signed-off-by: Steven Rostedt (VMware) <[email protected]>
3 participants
sdev->pdata->hw_pdata is assigned in hda_dsp_probe() so sdev->pdata->hw_pdata won't be null in hda_dsp_remove(). But hda could be null if hda_dsp_trace_init() is using in a wrong place.
Fix #492