Skip to content

Merge/sound upstream 20210301 - #2772

Merged
plbossart merged 10000 commits into
thesofproject:topic/sof-devfrom
plbossart:merge/sound-upstream-20210301
Mar 4, 2021
Merged

plbossart merged 10000 commits into
thesofproject:topic/sof-devfrom
plbossart:merge/sound-upstream-20210301

Conversation

@plbossart

Copy link
Copy Markdown
Member

First attempt with v5.12-rc1.

I don't have a matching topic/sof-dev-rebase due to issues with the script used to add github reviews

Christoph Hellwig and others added 30 commits February 25, 2021 14:31
Use the proper get_kernel_nofault helper to access an unsafe kernel
pointer without faulting instead of playing with set_fs and get_user.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Acked-by: Nick Hu <nickhu@andestech.com>
Acked-by: Greentime Hu <green.hu@gmail.com>
Signed-off-by: Greentime Hu <green.hu@gmail.com>
Andestech(nds32) never had <asm/procinfo.h>.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Acked-by: Greentime Hu <green.hu@gmail.com>
Signed-off-by: Greentime Hu <green.hu@gmail.com>
Free the memory on failure.
Also no need to re-alloc memory on retry.

Signed-off-by: xinhui pan <xinhui.pan@amd.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20210219042547.44855-1-xinhui.pan@amd.com
Reviewed-by: Christian König <christian.koenig@amd.com>
CC: stable@vger.kernel.org # 5.11
Signed-off-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
The AFBC decoder used in the Rockchip VOP assumes the use of the
YUV-like colourspace transform (YTR). YTR is lossless for RGB(A)
buffers, which covers the RGBA8 and RGB565 formats supported in
vop_convert_afbc_format. Use of YTR is signaled with the
AFBC_FORMAT_MOD_YTR modifier, which prior to this commit was missing. As
such, a producer would have to generate buffers that do not use YTR,
which the VOP would erroneously decode as YTR, leading to severe visual
corruption.

The upstream AFBC support was developed against a captured frame, which
failed to exercise modifier support. Prior to bring-up of AFBC in Mesa
(in the Panfrost driver), no open userspace respected modifier
reporting. As such, this change is not expected to affect broken
userspaces.

Tested on RK3399 with Panfrost and Weston.

Fixes: 7707f72 ("drm/rockchip: Add support for afbc")
Cc: stable@vger.kernel.org
Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
Acked-by: Daniel Stone <daniels@collabora.com>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
Link: https://patchwork.freedesktop.org/patch/msgid/20200811202631.3603-1-alyssa.rosenzweig@collabora.com
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
The panel is able to work when dsi clock is non-continuous, thus
the system power consumption can be reduced using such feature.

Add MIPI_DSI_CLOCK_NON_CONTINUOUS to panel's mode_flags.

Also the flag actually becomes necessary after
commit c6d94e3 ("drm/bridge/synopsys: dsi: add support for non-continuous HS clock")
and without it the panel only emits stripes instead of output.

Fixes: c6d94e3 ("drm/bridge/synopsys: dsi: add support for non-continuous HS clock")
Cc: stable@vger.kernel.org # 5.10+
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Ezequiel Garcia <ezequiel@collabora.com>
Reviewed-by: Christopher Morgan <macromorgan@hotmail.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20210206135020.1991820-1-heiko@sntech.de
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
In case of error, the function devm_platform_ioremap_resource()
returns ERR_PTR() and never returns NULL. The NULL test in the
return value check should be replaced with IS_ERR().

Fixes: 1448f8a ("sound: Add n64 driver")
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
Reviewed-by: Lauri Kasanen <cand@gmx.com>
Link: https://lore.kernel.org/r/20210224013803.2146953-1-weiyongjun1@huawei.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
Fix the interpreation of nested_svm_vmexit()'s return value when
synthesizing a nested VM-Exit after intercepting an SVM instruction while
L2 was running.  The helper returns '0' on success, whereas a return
value of '0' in the exit handler path means "exit to userspace".  The
incorrect return value causes KVM to exit to userspace without filling
the run state, e.g. QEMU logs "KVM: unknown exit, hardware reason 0".

Fixes: 14c2bf8 ("KVM: SVM: Fix #GP handling for doubly-nested virtualization")
Signed-off-by: Sean Christopherson <seanjc@google.com>
Message-Id: <20210224005627.657028-1-seanjc@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Since commit f086f67 ("arm64: ptrace: add support for syscall
emulation"), if system call number -1 is called and the process is being
traced with PTRACE_SYSCALL, for example by strace, the seccomp check is
skipped and -ENOSYS is returned unconditionally (unless altered by the
tracer) rather than carrying out action specified in the seccomp filter.

The consequence of this is that it is not possible to reliably strace
a seccomp based implementation of a foreign system call interface in
which r7/x8 is permitted to be -1 on entry to a system call.

Also trace_sys_enter and audit_syscall_entry are skipped if a system
call is skipped.

Fix by removing the in_syscall(regs) check restoring the previous
behaviour which is like AArch32, x86 (which uses generic code) and
everything else.

Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Catalin Marinas<catalin.marinas@arm.com>
Cc: <stable@vger.kernel.org>
Fixes: f086f67 ("arm64: ptrace: add support for syscall emulation")
Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
Tested-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Timothy E Baldwin <T.E.Baldwin99@members.leeds.ac.uk>
Link: https://lore.kernel.org/r/90edd33b-6353-1228-791f-0336d94d5f8c@majoroak.me.uk
Signed-off-by: Will Deacon <will@kernel.org>
Currently the arm64 unwinder code returns -EINVAL whenever it can't find
the next stack frame, not distinguishing between cases where the stack has
been corrupted or is otherwise in a state it shouldn't be and cases
where we have reached the end of the stack. At the minute none of the
callers care what error code is returned but this will be important for
reliable stack trace which needs to be sure that the stack is intact.

Change to return -ENOENT in the case where we reach the bottom of the
stack. The error codes from this function are only used in kernel, this
particular code is chosen as we are indicating that we know there is no
frame there.

Signed-off-by: Mark Brown <broonie@kernel.org>
Acked-by: Mark Rutland <mark.rutland@arm.com>
Link: https://lore.kernel.org/r/20210224165037.24138-1-broonie@kernel.org
Signed-off-by: Will Deacon <will@kernel.org>
This reverts commit 88f171a.

I ran into a case where the ref resurrect now spins, so revert
this change for now until we can further investigate why it's
broken. The bug seems to indicate spinning on the lock itself,
likely there's some ABBA deadlock involved:

[<0>] __percpu_ref_switch_mode+0x45/0x180
[<0>] percpu_ref_resurrect+0x46/0x70
[<0>] io_refs_resurrect+0x25/0xa0
[<0>] __io_uring_register+0x135/0x10c0
[<0>] __x64_sys_io_uring_register+0xc2/0x1a0
[<0>] do_syscall_64+0x42/0x110
[<0>] entry_SYSCALL_64_after_hwframe+0x44/0xa9

Signed-off-by: Jens Axboe <axboe@kernel.dk>
Freed extents are marked busy from the point the freeing transaction
commits until the associated CIL context is checkpointed to the log.
This prevents reuse and overwrite of recently freed blocks before
the changes are committed to disk, which can lead to corruption
after a crash. The exception to this rule is that metadata
allocation is allowed to reuse busy extents because metadata changes
are also logged.

As of commit 97d3ac7 ("xfs: exact busy extent tracking"), XFS
has allowed modification or complete invalidation of outstanding
busy extents for metadata allocations. This implementation assumes
that use of the associated extent is imminent, which is not always
the case. For example, the trimmed extent might not satisfy the
minimum length of the allocation request, or the allocation
algorithm might be involved in a search for the optimal result based
on locality.

generic/019 reproduces a corruption caused by this scenario. First,
a metadata block (usually a bmbt or symlink block) is freed from an
inode. A subsequent bmbt split on an unrelated inode attempts a near
mode allocation request that invalidates the busy block during the
search, but does not ultimately allocate it. Due to the busy state
invalidation, the block is no longer considered busy to subsequent
allocation. A direct I/O write request immediately allocates the
block and writes to it. Finally, the filesystem crashes while in a
state where the initial metadata block free had not committed to the
on-disk log. After recovery, the original metadata block is in its
original location as expected, but has been corrupted by the
aforementioned dio.

This demonstrates that it is fundamentally unsafe to modify busy
extent state for extents that are not guaranteed to be allocated.
This applies to pretty much all of the code paths that currently
trim busy extents for one reason or another. Therefore to address
this problem, drop the reuse mechanism from the busy extent trim
path. This code already knows how to return partial non-busy ranges
of the targeted free extent and higher level code tracks the busy
state of the allocation attempt. If a block allocation fails where
one or more candidate extents is busy, we force the log and retry
the allocation.

Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Brian Foster reported a lockdep warning on xfs/167:

============================================
WARNING: possible recursive locking detected
5.11.0-rc4 #35 Tainted: G        W I
--------------------------------------------
fsstress/17733 is trying to acquire lock:
ffff8e0fd1d90650 (sb_internal){++++}-{0:0}, at: xfs_free_eofblocks+0x104/0x1d0 [xfs]

but task is already holding lock:
ffff8e0fd1d90650 (sb_internal){++++}-{0:0}, at: xfs_trans_alloc_inode+0x5f/0x160 [xfs]

stack backtrace:
CPU: 38 PID: 17733 Comm: fsstress Tainted: G        W I       5.11.0-rc4 #35
Hardware name: Dell Inc. PowerEdge R740/01KPX8, BIOS 1.6.11 11/20/2018
Call Trace:
 dump_stack+0x8b/0xb0
 __lock_acquire.cold+0x159/0x2ab
 lock_acquire+0x116/0x370
 xfs_trans_alloc+0x1ad/0x310 [xfs]
 xfs_free_eofblocks+0x104/0x1d0 [xfs]
 xfs_blockgc_scan_inode+0x24/0x60 [xfs]
 xfs_inode_walk_ag+0x202/0x4b0 [xfs]
 xfs_inode_walk+0x66/0xc0 [xfs]
 xfs_trans_alloc+0x160/0x310 [xfs]
 xfs_trans_alloc_inode+0x5f/0x160 [xfs]
 xfs_alloc_file_space+0x105/0x300 [xfs]
 xfs_file_fallocate+0x270/0x460 [xfs]
 vfs_fallocate+0x14d/0x3d0
 __x64_sys_fallocate+0x3e/0x70
 do_syscall_64+0x33/0x40
 entry_SYSCALL_64_after_hwframe+0x44/0xa9

The cause of this is the new code that spurs a scan to garbage collect
speculative preallocations if we fail to reserve enough blocks while
allocating a transaction.  While the warning itself is a fairly benign
lockdep complaint, it does expose a potential livelock if the rwsem
behavior ever changes with regards to nesting read locks when someone's
waiting for a write lock.

Fix this by freeing the transaction and jumping back to xfs_trans_alloc
like this patch in the V4 submission[1].

[1] https://lore.kernel.org/linux-xfs/161142798066.2171939.9311024588681972086.stgit@magnolia/

Fixes: a1a7d05 ("xfs: flush speculative space allocations when we run out of space")
Reported-by: Brian Foster <bfoster@redhat.com>
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Because the iomap code using PF_MEMALLOC_NOFS to detect transaction
recursion in XFS is just wrong. Remove it from the iomap code and
replace it with XFS specific internal checks using
current->journal_info instead.

[djwong: This change also realigns the lifetime of NOFS flag changes to
match the incore transaction, instead of the inconsistent scheme we have
now.]

Fixes: 9070733 ("xfs: abstract PF_FSTRANS to PF_MEMALLOC_NOFS")
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
…reshold

When I added the quirk for the "HP Pavilion x2 10-p0XX" I copied the
byt_rt5640_quirk_table[] entry for the HP Pavilion x2 10-k0XX / 10-n0XX
models since these use almost the same settings.

While doing this I accidentally also copied and kept the non-standard
OVCD_TH_1500UA setting used on those models. This too low threshold is
causing headsets to often be seen as headphones (without a headset-mic)
and when correctly identified it is causing ghost play/pause
button-presses to get detected.

Correct the HP Pavilion x2 10-p0XX quirk to use the default OVCD_TH_2000UA
setting, fixing these problems.

Fixes: fbdae7d ("ASoC: Intel: bytcr_rt5640: Fix HP Pavilion x2 Detachable quirks")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Link: https://lore.kernel.org/r/20210224105052.42116-1-hdegoede@redhat.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Before the io-wq thread change, we maintained a hash work map and lock
per-node per-ring. That wasn't ideal, as we really wanted it to be per
ring. But now that we have per-task workers, the hash map ends up being
just per-task. That'll work just fine for the normal case of having
one task use a ring, but if you share the ring between tasks, then it's
considerably worse than it was before.

Make the hash map per ctx instead, which provides full per-ctx buffered
write serialization on hashed writes.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
Just like the changes for io-wq, ensure that we re-fork the SQPOLL
thread if the owner execs. Mark the ctx sq thread as sqo_exec if
it dies, and the ring as needing a wakeup which will force the task
to enter the kernel. When it does, setup the new thread and proceed
as usual.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
syzbot reports the following hang:

INFO: task syz-executor.0:12538 can't die for more than 143 seconds.
task:syz-executor.0  state:D stack:28352 pid:12538 ppid:  8423 flags:0x00004004
Call Trace:
 context_switch kernel/sched/core.c:4324 [inline]
 __schedule+0x90c/0x21a0 kernel/sched/core.c:5075
 schedule+0xcf/0x270 kernel/sched/core.c:5154
 schedule_timeout+0x1db/0x250 kernel/time/timer.c:1868
 do_wait_for_common kernel/sched/completion.c:85 [inline]
 __wait_for_common kernel/sched/completion.c:106 [inline]
 wait_for_common kernel/sched/completion.c:117 [inline]
 wait_for_completion+0x168/0x270 kernel/sched/completion.c:138
 io_sq_thread_finish+0x96/0x580 fs/io_uring.c:7152
 io_sq_offload_create fs/io_uring.c:7929 [inline]
 io_uring_create fs/io_uring.c:9465 [inline]
 io_uring_setup+0x1fb2/0x2c20 fs/io_uring.c:9550
 do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
 entry_SYSCALL_64_after_hwframe+0x44/0xae

which is due to exiting after the SQPOLL thread has been created, but
hasn't been started yet. Ensure that we always complete the startup
side when waiting for it to exit.

Reported-by: syzbot+c927c937cba8ef66dd4a@syzkaller.appspotmail.com
Signed-off-by: Jens Axboe <axboe@kernel.dk>
exec will cancel any threads, including the ones that io-wq is using. This
isn't a problem, in fact we'd prefer it to be that way since it means we
know that any async work cancels naturally without having to handle it
proactively.

But it does mean that we need to setup a new manager, as the manager and
workers are gone. Handle this at queue time, and cancel work if we fail.
Since the manager can go away without us noticing, ensure that the manager
itself holds a reference to the 'wq' as well. Rename io_wq_destroy() to
io_wq_put() to reflect that.

In the future we can now simplify exec cancelation handling, for now just
leave it the same.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
This flag is now dead, remove it.

Fixes: 1cbd9c2 ("io-wq: don't create any IO workers upfront")
Signed-off-by: Jens Axboe <axboe@kernel.dk>
sja1105_unpack() takes a "const void *buf" as its first parameter, so
there is no need to cast away the "const" of the "buf" variable before
calling it.

Drop the cast, as it prevents the compiler performing some checks.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
Link: https://lore.kernel.org/r/20210223112003.2223332-1-geert+renesas@glider.be
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
In IEC 62439-3 EntryForgetTime is defined with a value of 400 ms. When a
node does not send any frame within this time, the sequence number check
for can be ignored. This solves communication issues with Cisco IE 2000
in Redbox mode.

Fixes: f421436 ("net/hsr: Add support for the High-availability Seamless Redundancy protocol (HSRv0)")
Signed-off-by: Marco Wenzel <marco.wenzel@a-eberle.de>
Reviewed-by: George McCollister <george.mccollister@gmail.com>
Tested-by: George McCollister <george.mccollister@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Link: https://lore.kernel.org/r/20210224094653.1440-1-marco.wenzel@a-eberle.de
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
After filling RX ring slot with new skb it's required to free old skb.
Immediately on error or later in the net subsystem.

Fixes: 4feffea ("net: broadcom: bcm4908enet: add BCM4908 controller driver")
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
Link: https://lore.kernel.org/r/20210224151842.2419-1-zajec5@gmail.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Missing increment was resulting in poll function always returning 0
instead of amount of processed packets.

Fixes: 4feffea ("net: broadcom: bcm4908enet: add BCM4908 controller driver")
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
Link: https://lore.kernel.org/r/20210224151842.2419-2-zajec5@gmail.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
…etadata

nfs and cifs on Linux currently have a mount parameter "actimeo" to control
metadata (attribute) caching but cifs does not have additional mount
parameters to allow distinguishing between caching directory metadata
(e.g. needed to revalidate paths) and that for files.

Add new mount parameter "acdirmax" to allow caching metadata for
directories more loosely than file data.  NFS adjusts metadata
caching from acdirmin to acdirmax (and another two mount parms
for files) but to reduce complexity, it is safer to just introduce
the one mount parm to allow caching directories longer. The
defaults for acdirmax and actimeo (for cifs.ko) are conservative,
1 second (NFS defaults acdirmax to 60 seconds). For many workloads,
setting acdirmax to a higher value is safe and will improve
performance.  This patch leaves unchanged the default values
for caching metadata for files and directories but gives the
user more flexibility in adjusting them safely for their workload
via the new mount parm.

Signed-off-by: Steve French <stfrench@microsoft.com>
Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
Reviewed-By: Tom Talpey <tom@talpey.com>
…ache timeout

The new optional mount parm, "acdirmax" allows caching the metadata
for a directory longer than file metadata, which can be very helpful
for performance.  Convert cifs_inode_needs_reval to check acdirmax
for revalidating directory metadata.

Signed-off-by: Steve French <stfrench@microsoft.com>
Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
Reviewed-By: Tom Talpey <tom@talpey.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
…tadata timeout

The new optional mount parameter "acregmax" allows a different
timeout for file metadata ("acdirmax" now allows controlling timeout
for directory metadata).  Setting "actimeo" still works as before,
and changes timeout for both files and directories, but
specifying "acregmax" or "acdirmax" allows overriding the
default more granularly which can be a big performance benefit
on some workloads. "acregmax" is already used by NFS as a mount
parameter (albeit with a larger default and thus looser caching).

Suggested-by: Tom Talpey <tom@talpey.com>
Reviewed-By: Tom Talpey <tom@talpey.com>
Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
Currently, the psample netlink skb is allocated with a size that does
not account for the nested 'PSAMPLE_ATTR_TUNNEL' attribute and the
padding required for the 64-bit attribute 'PSAMPLE_TUNNEL_KEY_ATTR_ID'.
This can result in failure to add attributes to the netlink skb due
to insufficient tail room. The following error message is printed to
the kernel log: "Could not create psample log message".

Fix this by adjusting the allocation size to take into account the
nested attribute and the padding.

Fixes: d8bed68 ("net: psample: Add tunnel support")
CC: Yotam Gigi <yotam.gi@gmail.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: Chris Mi <cmi@nvidia.com>
Link: https://lore.kernel.org/r/20210225075145.184314-1-cmi@nvidia.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
The update to kmem_cache_free trace event added printing of the slab name in
the trace event. But it only stores the pointer of the name which will be
printed as a string when the event is read some time in the future. This is
dangerous because the name could be freed in the mean time and when reading
the trace event it would try to dereference the string name by the pointer
to the name that has been freed.

Instead, use the trace event helper macros __string(), __assign_str(), and
__get_str() that are for this very case.

Cc: Jacob Wen <jian.w.wen@oracle.com>
Fixes: 3544de8 ("mm, tracing: record slab name for kmem_cache_free()")
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
@plbossart

ghost commented Mar 1, 2021

Copy link
Copy Markdown
Member Author

Same results as last Friday, all tests are broken -> bisect required. Gah.

@plbossart

ghost commented Mar 1, 2021

Copy link
Copy Markdown
Member Author

it's possibly an sof-test/CI issue:

modprobe snd-sof-pci
export TPLG=/lib/firmware/intel/sof-tplg/sof-tgl-rt715-rt711-rt1308-mono.tplg 
~/sof-test/test-case/check-pause-resume.sh -c 10 -m playback
...
2021-03-01 19:02:15 UTC [INFO] Test Result: PASS!

root@plb-Latitude-9520:~# ps -edf | grep aplay
root        1360       1 95 13:01 ?        00:00:41 aplay -D hw:0,0 -r 48000 -c 2 -f S16_LE -vv -i /dev/zero -q
root        1419       1 96 13:02 ?        00:00:37 aplay -D hw:0,2 -r 48000 -c 2 -f S16_LE -vv -i /dev/zero -q
root        1478       1 95 13:02 ?        00:00:34 aplay -D hw:0,5 -r 48000 -c 2 -f S16_LE -vv -i /dev/zero -q
root        1539       1 94 13:02 ?        00:00:31 aplay -D hw:0,6 -r 48000 -c 2 -f S16_LE -vv -i /dev/zero -q
root        1598       1 95 13:02 ?        00:00:27 aplay -D hw:0,7 -r 48000 -c 2 -f S16_LE -vv -i /dev/zero -q
root        1669    1120  0 13:02 pts/0    00:00:00 grep --color=auto aplay

Killing those processes restores functionality. The question is why aren't they killed already and what changed?

@marc-hb @fredoh9 @xiulipan @aiChaoSONG FYI, this looks real bad.

@fredoh9

ghost commented Mar 1, 2021

Copy link
Copy Markdown
Collaborator

I was looking at "audio open error: Device or resource busy" errors. Let me check why aplay or arecord didnot be killed after the test.

@plbossart

ghost commented Mar 1, 2021

Copy link
Copy Markdown
Member Author

I was looking at "audio open error: Device or resource busy" errors. Let me check why aplay or arecord didnot be killed after the test.

I started a bisect since this worked with 5.11, this could also be a kernel regression, who knows?

@fredoh9

ghost commented Mar 1, 2021 •

Copy link
Copy Markdown
Collaborator

aplay or arecord interactive mode(-i) didn't be killed after the pause-resume test. 100% reproducible with this pr.

Current sof-test(check-pause-resume) do not kill the process after exit of expect script. topic/sof-dev works fine, so it might be a kernel regression.

@plbossart

ghost commented Mar 1, 2021

Copy link
Copy Markdown
Member Author

Definitively a TTY issue indeed

33d4ae98859873ddd49e22e4ca724387548b3d89 is the first bad commit
commit 33d4ae98859873ddd49e22e4ca724387548b3d89
Author: Corey Minyard <cminyard@mvista.com>
Date:   Mon Nov 23 18:49:02 2020 -0600

    drivers:tty:pty: Fix a race causing data loss on close
    
    Remove the tty_vhangup() from the pty code and just release the
    redirect.  The tty_vhangup() results in data loss and data out of order
    issues.
    
    If you write to a pty master an immediately close the pty master, the
    receiver might get a chunk of data dropped, but then receive some later
    data.  That's obviously something rather unexpected for a user.  It
    certainly confused my test program.
    
    It turns out that tty_vhangup() on the slave pty gets called from
    pty_close(), and that causes the data on the slave side to be flushed,
    but due to races more data can be copied into the slave side's buffer
    after that.  Consider the following sequence:
    
    thread1          thread2         thread3
    -------          -------         -------
     |                |-write data into buffer,
     |                | n_tty buffer is filled
     |                | along with other buffers
     |                |-pty_close(master)
     |                |--tty_vhangup(slave)
     |                |---tty_ldisc_hangup()
     |                |----n_tty_flush_buffer()
     |                |-----reset_buffer_flags()
     |-n_tty_read()   |
     |--up_read(&tty->termios_rwsem);
     |                |------down_read(&tty->termios_rwsem)
     |                |------clear n_tty buffer contents
     |                |------up_read(&tty->termios_rwsem)
     |--tty_buffer_flush_work()       |
     |--schedules work calling        |
     |  flush_to_ldisc()              |
     |                                |-flush_to_ldisc()
     |                                |--receive_buf()
     |                                |---tty_port_default_receive_buf()
     |                                |----tty_ldisc_receive_buf()
     |                                |-----n_tty_receive_buf2()
     |                                |------n_tty_receive_buf_common()
     |                                |-------down_read(&tty->termios_rwsem)
     |                                |-------__receive_buf()
     |                                |       copies data into n_tty buffer
     |                                |-------up_read(&tty->termios_rwsem)
     |--down_read(&tty->termios_rwsem)
     |--copy buffer data to user
    
    >From this sequence, you can see that thread2 writes to the buffer then
    only clears the part of the buffer in n_tty.  The n_tty receive buffer
    code then copies more data into the n_tty buffer.
    
    But part of the vhangup, releasing the redirect, is still required to
    avoid issues with consoles running on pty slaves.  So do that.
    As far as I can tell, that is all that should be required.
    
    Signed-off-by: Corey Minyard <cminyard@mvista.com>
    Link: https://lore.kernel.org/r/20201124004902.1398477-3-minyard@acm.org
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

 drivers/tty/pty.c    | 15 +++++++++++++--
 drivers/tty/tty_io.c |  5 +++--
 2 files changed, 16 insertions(+), 4 deletions(-)

This reverts commit 33d4ae9.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@marc-hb

ghost commented Mar 1, 2021

Copy link
Copy Markdown
Collaborator

https://lwn.net/Articles/343828/

There are dark areas of the kernel where only the bravest hackers dare to tread. Places where the code is twisted, the requirements are complex, and everything depends on ancient code which has seen little change over the years because even the most qualified developers fear the consequences. Arguably, no part of the kernel is darker and scarier than the serial terminal (TTY) code.

(2009)

@fredoh9

ghost commented Mar 1, 2021

Copy link
Copy Markdown
Collaborator

There are dark areas of the kernel...
everything depends on ancient code which has seen little change over the years...

hehehe~

@fredoh9

ghost commented Mar 1, 2021

Copy link
Copy Markdown
Collaborator

@plbossart I verified without the TTY you identified. It worked fine on my ICL. We can wait for the device test result.

@plbossart

ghost commented Mar 1, 2021

Copy link
Copy Markdown
Member Author

@plbossart I verified without the TTY you identified. It worked fine on my ICL. We can wait for the device test result.

Same here, the revert solves the issue on my local TGL test device. I let the author + maintainer know.

Pierre-Louis Bossart added 5 commits March 2, 2021 08:22
An unsigned long variable should rely on '%lu' format strings, not '%zd'

Fixes: a1a6a4c ("iwlwifi: pnvm: implement reading PNVM from UEFI")
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
v5.12-rc1 flags new warnings with make W=1, fix missing or broken
function descriptors.

sound/pci/hda/hda_codec.c:3492: warning: expecting prototype for
snd_hda_input_mux_info_info(). Prototype was for
snd_hda_input_mux_info() instead

sound/pci/hda/hda_codec.c:3521: warning: expecting prototype for
snd_hda_input_mux_info_put(). Prototype was for
snd_hda_input_mux_put() instead

sound/pci/hda/hda_codec.c:3958: warning: expecting prototype for
_snd_hda_pin_ctl(). Prototype was for _snd_hda_set_pin_ctl() instead

sound/pci/hda/hda_jack.c:223: warning: expecting prototype for
snd_hda_set_dirty_all(). Prototype was for
snd_hda_jack_set_dirty_all() instead

sound/pci/hda/hda_jack.c:309: warning: expecting prototype for
snd_hda_jack_detect_enable_mst(). Prototype was for
snd_hda_jack_detect_enable_callback_mst() instead

sound/pci/hda/hda_generic.c:3933: warning: expecting prototype for
snd_dha_gen_add_mute_led_cdev(). Prototype was for
snd_hda_gen_add_mute_led_cdev() instead

sound/pci/hda/hda_generic.c:4093: warning: expecting prototype for
snd_dha_gen_add_micmute_led_cdev(). Prototype was for
snd_hda_gen_add_micmute_led_cdev() instead

sound/pci/hda/patch_ca0132.c:2357: warning: expecting prototype for
Prepare and send the SCP message to DSP(). Prototype was for
dspio_scp() instead

sound/pci/hda/patch_ca0132.c:2883: warning: expecting prototype for
Allocate router ports(). Prototype was for dsp_allocate_router_ports()
instead

sound/pci/hda/patch_ca0132.c:3202: warning: expecting prototype for
Write a block of data into DSP code or data RAM using pre(). Prototype
was for dspxfr_one_seg() instead

sound/pci/hda/patch_ca0132.c:3397: warning: expecting prototype for
data overlay to DSP memories(). Prototype was for dspxfr_image()
instead

sound/hda/hdac_regmap.c:393: warning: expecting prototype for
snd_hdac_regmap_init(). Prototype was for snd_hdac_regmap_exit()
instead

sound/hda/ext/hdac_ext_controller.c:142: warning: expecting prototype
for snd_hdac_ext_bus_get_link_index(). Prototype was for
snd_hdac_ext_bus_get_link() instead

sound/hda/ext/hdac_ext_stream.c:140: warning: expecting prototype for
snd_hdac_ext_linkstream_start(). Prototype was for
snd_hdac_ext_link_stream_start() instead

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
v5.12-rc1 flags new warnings with make W=1, fix missing or broken
function descriptors.

sound/soc/codecs/nau8825.c:298: warning: wrong kernel-doc identifier
on line:
 * Ramp up the headphone volume change gradually to target level.

sound/soc/codecs/nau8825.c:358: warning: expecting prototype for This
func(). Prototype was for nau8825_intlog10_dec3() instead

sound/soc/codecs/nau8825.c:411: warning: wrong kernel-doc identifier
on line:
 * computes cross talk suppression sidetone gain.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
v5.12-rc1 flags new warnings with make W=1, fix missing or broken
function descriptors.

sound/soc/intel/atom/sst/sst_loader.c:85: warning: expecting prototype
for sst_start_merrifield(). Prototype was for sst_start_mrfld()
instead

sound/soc/intel/atom/sst/sst_acpi.c:339: warning: expecting prototype
for intel_sst_remove(). Prototype was for sst_acpi_remove() instead

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
v5.12-rc1 flags new warnings with make W=1, fix missing or broken
function descriptors.

sound/soc/soc-dai.c:167: warning: expecting prototype for
snd_soc_xlate_tdm_slot(). Prototype was for
snd_soc_xlate_tdm_slot_mask() instead

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart

ghost commented Mar 2, 2021

Copy link
Copy Markdown
Member Author

Added a couple of commits for fix warnings and compilation issues

@plbossart

ghost commented Mar 2, 2021

Copy link
Copy Markdown
Member Author

@fredoh9 can we run an Intel daily test with this PR? The results look fine now, but more evidence wouldn't hurt to avoid disturbing developers.

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My local tests pass, so if daily test is ok, good for me.

@fredoh9

ghost commented Mar 3, 2021 •

Copy link
Copy Markdown
Collaborator

Manual daily test ID is 2414.
suspend/resume test failed in EHL_RVP_I2S due to i915 error. Other than this, all results are good.

[ 24.661004] kernel: i915 0000:00:02.0: [drm] *ERROR* Suspending crtc's failed with -22

@plbossart

ghost commented Mar 3, 2021

Copy link
Copy Markdown
Member Author

@kv2019i I am not sure how to deal with the EHL_RVP issue above. Those boards are in short supply and this should be a dedicated debug effort. I'll let you make a call if you want to merge since this is a DRM problem beyond my understanding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.