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

Upstream: inspect ops() abstraction, move "private" functions #563

Closed
plbossart opened this issue Jan 18, 2019 · 1 comment
Closed

Upstream: inspect ops() abstraction, move "private" functions #563

plbossart opened this issue Jan 18, 2019 · 1 comment
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@plbossart
Copy link
Member

Copy/paste from emails:

>> +       /* doorbell */
>> +       .irq_handler    = byt_irq_handler,
>> +       .irq_thread     = byt_irq_thread,
>> +
> What is the reason for having irq_handler/irq_thread functions inside the
> snd_sof_dsp_ops structure?
>
> These functions are never used outside via sdev->ops pointer.

Good point indeed, thanks for raising it. We were in the middle of
tagging which ops are required/optional (feedback from Mark) but we
started from the core and should have looked at the structure definition.

Most drivers are "self-contained" and can reference the irq_thread and
irq_handler directly.

The exception where the abstraction is used is internal to the HDaudio
stuff:

intel/hda.c:    ret = request_threaded_irq(sdev->ipc_irq,
hda_dsp_ipc_irq_handler,
intel/hda.c: sof_ops(sdev)->irq_thread, IRQF_SHARED,

That's useful since there a minor variations between hardware
generations and you want to hide the hardware-specific parts.

But as you point out, it's a "private" use of ops callbacks, the core
doesn't touch this.

I have no explanation other than legacy/historical reasons or a shortcut
to make one's life easier during development.  Liam and Keyon might know?

We could try and move this to a more "private" structure, the
"chip_info" part might be more suitable for this?
@plbossart plbossart added enhancement New feature or request good first issue Good for newcomers labels Jan 18, 2019
@stripes416 stripes416 self-assigned this Jan 25, 2019
@stripes416
Copy link

Now that I'm back from leave, I'll be picking this back up.

bardliao pushed a commit to bardliao/linux that referenced this issue Sep 19, 2022
bpf_sk_reuseport_detach() calls __rcu_dereference_sk_user_data_with_flags()
to obtain the value of sk->sk_user_data, but that function is only usable
if the RCU read lock is held, and neither that function nor any of its
callers hold it.

Fix this by adding a new helper, __locked_read_sk_user_data_with_flags()
that checks to see if sk->sk_callback_lock() is held and use that here
instead.

Alternatively, making __rcu_dereference_sk_user_data_with_flags() use
rcu_dereference_checked() might suffice.

Without this, the following warning can be occasionally observed:

=============================
WARNING: suspicious RCU usage
6.0.0-rc1-build2+ thesofproject#563 Not tainted
-----------------------------
include/net/sock.h:592 suspicious rcu_dereference_check() usage!

other info that might help us debug this:

rcu_scheduler_active = 2, debug_locks = 1
5 locks held by locktest/29873:
 #0: ffff88812734b550 (&sb->s_type->i_mutex_key#9){+.+.}-{3:3}, at: __sock_release+0x77/0x121
 #1: ffff88812f5621b0 (sk_lock-AF_INET){+.+.}-{0:0}, at: tcp_close+0x1c/0x70
 #2: ffff88810312f5c8 (&h->lhash2[i].lock){+.+.}-{2:2}, at: inet_unhash+0x76/0x1c0
 #3: ffffffff83768bb8 (reuseport_lock){+...}-{2:2}, at: reuseport_detach_sock+0x18/0xdd
 #4: ffff88812f562438 (clock-AF_INET){++..}-{2:2}, at: bpf_sk_reuseport_detach+0x24/0xa4

stack backtrace:
CPU: 1 PID: 29873 Comm: locktest Not tainted 6.0.0-rc1-build2+ thesofproject#563
Hardware name: ASUS All Series/H97-PLUS, BIOS 2306 10/09/2014
Call Trace:
 <TASK>
 dump_stack_lvl+0x4c/0x5f
 bpf_sk_reuseport_detach+0x6d/0xa4
 reuseport_detach_sock+0x75/0xdd
 inet_unhash+0xa5/0x1c0
 tcp_set_state+0x169/0x20f
 ? lockdep_sock_is_held+0x3a/0x3a
 ? __lock_release.isra.0+0x13e/0x220
 ? reacquire_held_locks+0x1bb/0x1bb
 ? hlock_class+0x31/0x96
 ? mark_lock+0x9e/0x1af
 __tcp_close+0x50/0x4b6
 tcp_close+0x28/0x70
 inet_release+0x8e/0xa7
 __sock_release+0x95/0x121
 sock_close+0x14/0x17
 __fput+0x20f/0x36a
 task_work_run+0xa3/0xcc
 exit_to_user_mode_prepare+0x9c/0x14d
 syscall_exit_to_user_mode+0x18/0x44
 entry_SYSCALL_64_after_hwframe+0x63/0xcd

Fixes: cf8c1e9 ("net: refactor bpf_sk_reuseport_detach()")
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Hawkins Jiawei <yin31149@gmail.com>
Link: https://lore.kernel.org/r/166064248071.3502205.10036394558814861778.stgit@warthog.procyon.org.uk
Signed-off-by: Jakub Kicinski <kuba@kernel.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 good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants