[PATCH v6 0/4] Let userspace know when snd-hda-intel needs i915
Currently, kernel/module annotates module dependencies when request_symbol is used, but it doesn't cover more complex inter-driver dependencies that are subsystem and/or driver-specific.
That's because module_try_get() and symbol_get() doesn't try to setup the module owner.
In the case of hdmi sound, depending on the CPU/GPU, sometimes the snd_hda_driver can talk directly with the hardware, but sometimes, it uses the i915 driver. When the snd_hda_driver uses i915, it should first be unbind/rmmod, as otherwise trying to unbind/rmmod the i915 driver cause driver issues, as as reported by CI tools with different GPU models: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6415/fi-tgl-1115g4/igt@core_hot... https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11495/bat-adlm-1/igt@i915_mo...
In the past, just a few CPUs were doing such bindings, but this issue now applies to all "modern" Intel CPUs that have onboard graphics, as well as to the newer discrete GPUs.
With the discrete GPU case, the HDA controller is physically separate and requires i915 to power on the hardware for all hardware access. In this case, the issue is hit basicly 100% of the time.
With on-board graphics, i915 driver is needed only when the display codec is accessed. If i915 is unbind during runtime suspend, while snd-hda-intel is still bound, nothing bad happens, but unbinding i915 on other situations may also cause issues.
So, add support at kernel/modules to properly set the holders when try_module_get() and symbol_get() are used.
This allow allow audio drivers to properly annotate when a dependency on a DRM driver dependencies exists, and add a call to such new function at the snd-hda driver when it successfully binds into the DRM driver.
With that, userspace tools can now check and properly remove the audio driver before trying to remove or unbind the GPU driver.
It should be noticed that this series conveys the hidden module dependencies. Other changes are needed in order to allow removing or unbinding the i915 driver while keeping the snd-hda-intel driver loaded/bound. With that regards, there are some discussions on how to improve this at alsa-devel a while back:
https://mailman.alsa-project.org/pipermail/alsa-devel/2021-September/190099....
So, future improvements on both in i915 and the audio drivers could be made. E.g. with discrete GPUs, it's the only codec of the card, so it seems feasible to detach the ALSA card if i915 is bound (using infra made for VGA switcheroo), but, until these improvements are done and land in upstream, audio drivers needs to be unbound if i915 driver goes unbind.
Yet, even if such fixes got merged, this series is still needed, as it makes such dependencies more explicit and easier to debug.
PS.: This series was generated against next-20220506.
---
v6: - dropped an unused function prototype for __symbol_get_gpl(); - addressed several issues that were noticed while testing the series on an slow atom machine; - also add holders when symbol_get() is used.
v5: - while v4 works fine, it ends calling try_module_format() recursively, which is not what it it was supposed to do. So, change the logic to avoid such recursion, by adding a static __try_module_format() and renaming the new version that takes two arguments as try_module_format_owner().
v4: - fix a compilation warning reported by Intel's Kernel robot when !CONFIG_MODULE_UNLOAD or !CONFIG_MODULE.
v3: minor fixes: - fixed a checkpatch warning; - use a single line for the new function prototype.
v2: - the dependencies are now handled directly at try_module_get(). Mauro Carvalho Chehab (4): module: drop prototype for non-existing __symbol_get_gpl() module: update dependencies at try_module_get() module: set holders when symbol_get() is used ALSA: hda - identify when audio is provided by a video driver
drivers/mtd/chips/gen_probe.c | 4 +- include/linux/module.h | 13 +++-- kernel/module/main.c | 76 ++++++++++++++++++++----- samples/hw_breakpoint/data_breakpoint.c | 2 +- sound/hda/hdac_component.c | 2 +- 5 files changed, 72 insertions(+), 25 deletions(-)
There's no such function, and __symbol_get() is already declared as GPL. So, this is likely a left-over.
Signed-off-by: Mauro Carvalho Chehab mchehab@kernel.org ---
See [PATCH v6 0/4] at: https://lore.kernel.org/all/cover.1652113087.git.mchehab@kernel.org/
include/linux/module.h | 1 - 1 file changed, 1 deletion(-)
diff --git a/include/linux/module.h b/include/linux/module.h index ccfbaec82790..77961f5773b6 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -299,7 +299,6 @@ struct notifier_block; extern int modules_disabled; /* for sysctl */ /* Get/put a kernel symbol (calls must be symmetric) */ void *__symbol_get(const char *symbol); -void *__symbol_get_gpl(const char *symbol); #define symbol_get(x) ((typeof(&x))(__symbol_get(__stringify(x))))
/* modules using other modules: kdb wants to see this. */
Sometimes, device drivers are bound into each other via try_module_get(), making such references invisible when looking at /proc/modules or lsmod.
Add a function to allow setting up module references for such cases, and call it when try_module_get() is used.
Reviewed-by: Dan Williams dan.j.williams@intel.com Reviewed-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Mauro Carvalho Chehab mchehab@kernel.org ---
See [PATCH v6 0/4] at: https://lore.kernel.org/all/cover.1652113087.git.mchehab@kernel.org/
include/linux/module.h | 8 +++-- kernel/module/main.c | 73 ++++++++++++++++++++++++++++++++++-------- 2 files changed, 64 insertions(+), 17 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h index 77961f5773b6..a66b9be92ef5 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -618,12 +618,12 @@ extern void __module_get(struct module *module);
/* This is the Right Way to get a module: if it fails, it's being removed, * so pretend it's not there. */ -extern bool try_module_get(struct module *module); +extern bool try_module_get_owner(struct module *module, struct module *this);
extern void module_put(struct module *module);
#else /*!CONFIG_MODULE_UNLOAD*/ -static inline bool try_module_get(struct module *module) +static inline bool try_module_get_owner(struct module *module, struct module *this) { return !module || module_is_live(module); } @@ -752,7 +752,7 @@ static inline void __module_get(struct module *module) { }
-static inline bool try_module_get(struct module *module) +static inline bool try_module_get_owner(struct module *module, struct module *this) { return true; } @@ -893,6 +893,8 @@ static inline bool module_sig_ok(struct module *module) } #endif /* CONFIG_MODULE_SIG */
+#define try_module_get(mod) try_module_get_owner(mod, THIS_MODULE) + int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *, struct module *, unsigned long), void *data); diff --git a/kernel/module/main.c b/kernel/module/main.c index fe44d46c378b..6044aeba0f18 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -139,6 +139,24 @@ int unregister_module_notifier(struct notifier_block *nb) } EXPORT_SYMBOL(unregister_module_notifier);
+static bool __try_module_get(struct module *module) +{ + bool ret = true; + + if (module) { + preempt_disable(); + /* Note: here, we can fail to get a reference */ + if (likely(module_is_live(module) && + atomic_inc_not_zero(&module->refcnt) != 0)) + trace_module_get(module, _RET_IP_); + else + ret = false; + + preempt_enable(); + } + return ret; +} + /* * We require a truly strong try_module_get(): 0 means success. * Otherwise an error is returned due to ongoing or failed @@ -149,7 +167,7 @@ static inline int strong_try_module_get(struct module *mod) BUG_ON(mod && mod->state == MODULE_STATE_UNFORMED); if (mod && mod->state == MODULE_STATE_COMING) return -EBUSY; - if (try_module_get(mod)) + if (__try_module_get(mod)) return 0; else return -ENOENT; @@ -620,6 +638,41 @@ static int ref_module(struct module *a, struct module *b) return 0; }
+static int ref_module_dependency(struct module *mod, struct module *this) +{ + int ret = 0; + + if (!this || !mod || !this->name || !mod->holders_dir) + return -EINVAL; + + if (mod == this) + return 0; + + mutex_lock(&module_mutex); + + if (already_uses(this, mod)) + goto ret; + + ret = strong_try_module_get(mod); + if (ret) + goto ret; + + ret = add_module_usage(this, mod); + if (ret) { + module_put(mod); + goto ret; + } + +#ifdef CONFIG_MODULE_UNLOAD + ret = sysfs_create_link(mod->holders_dir, + &this->mkobj.kobj, this->name); +#endif + +ret: + mutex_unlock(&module_mutex); + return ret; +} + /* Clear the unload stuff of the module. */ static void module_unload_free(struct module *mod) { @@ -830,24 +883,16 @@ void __module_get(struct module *module) } EXPORT_SYMBOL(__module_get);
-bool try_module_get(struct module *module) +bool try_module_get_owner(struct module *module, struct module *this) { - bool ret = true; + int ret = __try_module_get(module);
- if (module) { - preempt_disable(); - /* Note: here, we can fail to get a reference */ - if (likely(module_is_live(module) && - atomic_inc_not_zero(&module->refcnt) != 0)) - trace_module_get(module, _RET_IP_); - else - ret = false; + if (ret) + ref_module_dependency(module, this);
- preempt_enable(); - } return ret; } -EXPORT_SYMBOL(try_module_get); +EXPORT_SYMBOL(try_module_get_owner);
void module_put(struct module *module) {
Greeting,
FYI, we noticed the following commit (built with gcc-11):
commit: a1f1245bef4084ac5f6ccf3d075a3551476fe948 ("[Intel-gfx] [PATCH v6 2/4] module: update dependencies at try_module_get()") url: https://github.com/intel-lab-lkp/linux/commits/Mauro-Carvalho-Chehab/Let-use... base: https://git.kernel.org/cgit/linux/kernel/git/mcgrof/linux.git modules-next patch link: https://lore.kernel.org/alsa-devel/28a942f860ccdee05751dcccc74b70e9d64f2b94....
in testcase: trinity version: trinity-i386-4d2343bd-1_20200320 with following parameters:
runtime: 300s group: group-03
test-description: Trinity is a linux system call fuzz tester. test-url: http://codemonkey.org.uk/projects/trinity/
on test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
If you fix the issue, kindly add following tag Reported-by: kernel test robot oliver.sang@intel.com
[ 21.287386][ T3487] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:280 [ 21.288810][ T3487] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 3487, name: trinity-main [ 21.291046][ T3487] preempt_count: 1, expected: 0 [ 21.292559][ T3487] CPU: 1 PID: 3487 Comm: trinity-main Not tainted 5.18.0-rc1-00031-ga1f1245bef40 #1 [ 21.294755][ T3487] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.0-debian-1.16.0-4 04/01/2014 [ 21.297034][ T3487] Call Trace: [ 21.298338][ T3487] <TASK> [ 21.299507][ T3487] dump_stack_lvl (lib/dump_stack.c:107 (discriminator 1)) [ 21.301012][ T3487] __might_resched.cold (kernel/sched/core.c:9734) [ 21.302642][ T3487] mutex_lock (kernel/locking/mutex.c:280) [ 21.304051][ T3487] try_module_get_owner (kernel/module/main.c:653 kernel/module/main.c:891) [ 21.305665][ T3487] can_create (net/can/af_can.c:102 net/can/af_can.c:142) can [ 21.307157][ T3487] __sock_create (net/socket.c:1469) [ 21.308584][ T3487] __sys_socket (net/socket.c:1519 net/socket.c:1561) [ 21.309971][ T3487] ? __might_fault (mm/memory.c:5324) [ 21.311415][ T3487] ? _copy_from_user (arch/x86/include/asm/uaccess_64.h:46 arch/x86/include/asm/uaccess_64.h:52 lib/usercopy.c:16) [ 21.312850][ T3487] __do_compat_sys_socketcall (net/compat.c:450) [ 21.314418][ T3487] ? switch_fpu_return (arch/x86/kernel/fpu/context.h:50 arch/x86/kernel/fpu/context.h:77 arch/x86/kernel/fpu/core.c:755) [ 21.315916][ T3487] __do_fast_syscall_32 (arch/x86/entry/common.c:112 arch/x86/entry/common.c:178) [ 21.317406][ T3487] do_fast_syscall_32 (arch/x86/entry/common.c:203) [ 21.319647][ T3487] entry_SYSENTER_compat_after_hwframe (arch/x86/entry/entry_64_compat.S:142) [ 21.321308][ T3487] RIP: 0023:0xf7f92549 [ 21.322572][ T3487] Code: 03 74 c0 01 10 05 03 74 b8 01 10 06 03 74 b4 01 10 07 03 74 b0 01 10 08 03 74 d8 01 00 00 00 00 00 51 52 55 89 e5 0f 34 cd 80 <5d> 5a 59 c3 90 90 90 90 8d b4 26 00 00 00 00 8d b4 26 00 00 00 00 All code ======== 0: 03 74 c0 01 add 0x1(%rax,%rax,8),%esi 4: 10 05 03 74 b8 01 adc %al,0x1b87403(%rip) # 0x1b8740d a: 10 06 adc %al,(%rsi) c: 03 74 b4 01 add 0x1(%rsp,%rsi,4),%esi 10: 10 07 adc %al,(%rdi) 12: 03 74 b0 01 add 0x1(%rax,%rsi,4),%esi 16: 10 08 adc %cl,(%rax) 18: 03 74 d8 01 add 0x1(%rax,%rbx,8),%esi 1c: 00 00 add %al,(%rax) 1e: 00 00 add %al,(%rax) 20: 00 51 52 add %dl,0x52(%rcx) 23: 55 push %rbp 24: 89 e5 mov %esp,%ebp 26: 0f 34 sysenter 28: cd 80 int $0x80 2a:* 5d pop %rbp <-- trapping instruction 2b: 5a pop %rdx 2c: 59 pop %rcx 2d: c3 retq 2e: 90 nop 2f: 90 nop 30: 90 nop 31: 90 nop 32: 8d b4 26 00 00 00 00 lea 0x0(%rsi,%riz,1),%esi 39: 8d b4 26 00 00 00 00 lea 0x0(%rsi,%riz,1),%esi
Code starting with the faulting instruction =========================================== 0: 5d pop %rbp 1: 5a pop %rdx 2: 59 pop %rcx 3: c3 retq 4: 90 nop 5: 90 nop 6: 90 nop 7: 90 nop 8: 8d b4 26 00 00 00 00 lea 0x0(%rsi,%riz,1),%esi f: 8d b4 26 00 00 00 00 lea 0x0(%rsi,%riz,1),%esi [ 21.326895][ T3487] RSP: 002b:00000000ffcf9b90 EFLAGS: 00000206 ORIG_RAX: 0000000000000066 [ 21.328923][ T3487] RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00000000ffcf9ba4 [ 21.330892][ T3487] RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000000000003 [ 21.332797][ T3487] RBP: 0000000000000003 R08: 0000000000000000 R09: 0000000000000000 [ 21.334697][ T3487] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000 [ 21.336568][ T3487] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000 [ 21.338481][ T3487] </TASK> [ 21.372109][ T3565] can: broadcast manager protocol [ OK ] Started System Logging Service. [ 23.041578][ T264] LKP: stdout: 243: Kernel tests: Boot OK! [ 23.041588][ T264] [ 28.843305][ T264] LKP: stdout: 243: HOSTNAME vm-snb-99, MAC 52:54:00:12:34:56, kernel 5.18.0-rc1-00031-ga1f1245bef40 1 [ 28.843315][ T264] [ 29.128109][ T264] install debs round one: dpkg -i --force-confdef --force-depends /opt/deb/gawk_1%3a4.1.4+dfsg-1_i386.deb [ 29.128119][ T264] [ 29.135239][ T264] Selecting previously unselected package gawk. [ 29.135247][ T264] [ 29.143980][ T264] (Reading database ... 16210 files and directories currently installed.) [ 29.143988][ T264] [ 29.152286][ T264] Preparing to unpack .../gawk_1%3a4.1.4+dfsg-1_i386.deb ... [ 29.152293][ T264] [ 29.158858][ T264] Unpacking gawk (1:4.1.4+dfsg-1) ... [ 29.158903][ T264] [ 29.166758][ T264] Setting up gawk (1:4.1.4+dfsg-1) ... [ 29.166765][ T264] [ 36.360833][ T264] LKP: stdout: 243: /lkp/lkp/src/bin/run-lkp /lkp/jobs/scheduled/vm-snb-99/trinity-group-03-300s-debian-i386-20191205.cgz-a1f1245bef4084ac5f6ccf3d075a3551476fe948-20220512-45405-6v8k22-5.yaml [ 36.360844][ T264] [ 36.381667][ T264] RESULT_ROOT=/result/trinity/group-03-300s/vm-snb/debian-i386-20191205.cgz/x86_64-rhel-8.3/gcc-11/a1f1245bef4084ac5f6ccf3d075a3551476fe948/3 [ 36.381678][ T264] [ 36.401159][ T264] job=/lkp/jobs/scheduled/vm-snb-99/trinity-group-03-300s-debian-i386-20191205.cgz-a1f1245bef4084ac5f6ccf3d075a3551476fe948-20220512-45405-6v8k22-5.yaml [ 36.401169][ T264] [ 41.230056][ T264] result_service: raw_upload, RESULT_MNT: /internal-lkp-server/result, RESULT_ROOT: /internal-lkp-server/result/trinity/group-03-300s/vm-snb/debian-i386-20191205.cgz/x86_64-rhel-8.3/gcc-11/a1f1245bef4084ac5f6ccf3d075a3551476fe948/3, TMP_RESULT_ROOT: /tmp/lkp/result [ 41.230068][ T264] [ 41.248856][ T264] run-job /lkp/jobs/scheduled/vm-snb-99/trinity-group-03-300s-debian-i386-20191205.cgz-a1f1245bef4084ac5f6ccf3d075a3551476fe948-20220512-45405-6v8k22-5.yaml [ 41.248882][ T264] [ 44.208341][ T264] /usr/bin/wget -q --timeout=1800 --tries=1 --local-encoding=UTF-8 http://internal-lkp-server:80/~lkp/cgi-bin/lkp-jobfile-append-var?job_file=/... -O /dev/null [ 44.208353][ T264] [ 44.219857][ T264] target ucode: [ 44.219891][ T264] [ 44.228816][ T264] Seeding trinity by 167967812 based on vm-snb/debian-i386-20191205.cgz/x86_64-rhel-8.3 [ 44.228827][ T264] [ 54.666581][ T264] 2022-05-12 02:09:19 chroot --userspec nobody:nogroup / trinity -q -q -l off -s 167967812 -N 999999999 -c add_key -c alarm -c bind -c brk -c chroot -c clock_getres -c clock_settime -c close -c execveat -c fadvise64_64 -c fchdir -c fchmodat -c fchown -c fchown16 -c fremovexattr -c fsmount -c fstatat64 -c futimesat -c getcpu -c getgid -c getsockopt -c gettid -c inotify_init1 -c io_uring_register -c ioctl -c listxattr -c llseek -c lsetxattr -c madvise -c migrate_pages -c mknod -c modify_ldt -c move_mount -c mq_getsetattr -c msync -c munlockall -c openat -c personality -c pidfd_open -c prlimit64 -c pwrite64 -c pwritev -c quotactl -c rename -c renameat2 -c rt_sigqueueinfo -c sched_getattr -c sched_getparam -c select -c semctl -c sendmmsg -c sendmsg -c sendto -c set_robust_list -c setfsgid16 -c setgid -c setgroups16 -c setitimer -c setresuid16 -c setuid -c sigaltstack -c signalfd -c statx -c te [ 54.666593][ T264] [ 60.531652][ T264] Trinity 2019.06 Dave Jones davej@codemonkey.org.uk [ 60.531662][ T264] [ 60.538969][ T264] shm:0xf7f88000-0x4734e14 (4 pages) [ 60.538977][ T264] [ 60.647920][ T264] [main] Marking syscall add_key (286) as to be enabled. [ 60.647930][ T264] [ 60.656357][ T264] [main] alarm is marked as AVOID. Skipping [ 60.656366][ T264] [ 60.665687][ T264] [main] Marking syscall alarm (27) as to be enabled. [ 60.665695][ T264] [ 60.673619][ T264] [main] Marking syscall bind (361) as to be enabled. [ 60.673627][ T264] [ 60.682931][ T264] [main] brk is marked as AVOID. Skipping [ 60.682939][ T264] [ 60.689625][ T264] [main] Marking syscall brk (45) as to be enabled. [ 60.689633][ T264] [ 60.697690][ T264] [main] Marking syscall chroot (61) as to be enabled. [ 60.697698][ T264] [ 61.189764][ T264] [main] Marking syscall clock_getres (266) as to be enabled. [ 61.189774][ T264] [ 61.299228][ T264] [main] Marking syscall clock_settime (264) as to be enabled. [ 61.299239][ T264] [ 61.307346][ T264] [main] close is marked as AVOID. Skipping [ 61.307354][ T264] [ 61.314936][ T264] [main] Marking syscall close (6) as to be enabled. [ 61.314944][ T264] [ 61.323575][ T264] [main] Marking syscall execveat (358) as to be enabled. [ 61.323585][ T264] [ 61.334213][ T264] [main] Marking syscall fadvise64_64 (272) as to be enabled. [ 61.334222][ T264] [ 61.344015][ T264] [main] Marking syscall fchdir (133) as to be enabled. [ 61.344023][ T264] [ 61.351111][ T264] [main] Marking syscall fchmodat (306) as to be enabled. [ 61.351119][ T264] [ 61.359986][ T264] [main] Marking syscall fchown (207) as to be enabled. [ 61.359995][ T264] [ 61.367978][ T264] [main] Marking syscall fchown16 (95) as to be enabled. [ 61.367986][ T264] [ 61.380250][ T264] [main] Marking syscall fremovexattr (237) as to be enabled. [ 61.380259][ T264] [ 61.391813][ T264] [main] Marking syscall fsmount (425) as to be enabled. [ 61.391822][ T264] [ 61.399349][ T264] [main] Marking syscall fstatat64 (300) as to be enabled. [ 61.399357][ T264] [ 61.411934][ T264] [main] Marking syscall futimesat (299) as to be enabled.
To reproduce:
# build kernel cd linux cp config-5.18.0-rc1-00031-ga1f1245bef40 .config make HOSTCC=gcc-11 CC=gcc-11 ARCH=x86_64 olddefconfig prepare modules_prepare bzImage modules make HOSTCC=gcc-11 CC=gcc-11 ARCH=x86_64 INSTALL_MOD_PATH=<mod-install-dir> modules_install cd <mod-install-dir> find lib/ | cpio -o -H newc --quiet | gzip > modules.cgz
git clone https://github.com/intel/lkp-tests.git cd lkp-tests bin/lkp qemu -k <bzImage> -m modules.cgz job-script # job-script is attached in this email
# if come across any failure that blocks the test, # please remove ~/.lkp and /lkp dir to run from a clean state.
Some Kernel modules use symbol_get() or symbol_request() in order to bind into other drivers. That's the case, for instance, of media dvb drivers that hook the frontend drivers via I2C using dvb_attach() macro.
When such bindings happen, one needs first to unload/unbind the driver that got the symbol before being able to unload/unbind the module that contains the needed symbol.
Add a logic to document it via /proc/modules and via lsmod.
Signed-off-by: Mauro Carvalho Chehab mchehab@kernel.org ---
See [PATCH v6 0/4] at: https://lore.kernel.org/all/cover.1652113087.git.mchehab@kernel.org/
drivers/mtd/chips/gen_probe.c | 4 ++-- include/linux/module.h | 4 ++-- kernel/module/main.c | 3 ++- samples/hw_breakpoint/data_breakpoint.c | 2 +- 4 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/mtd/chips/gen_probe.c b/drivers/mtd/chips/gen_probe.c index 4d4f97841016..f1e97633ac09 100644 --- a/drivers/mtd/chips/gen_probe.c +++ b/drivers/mtd/chips/gen_probe.c @@ -208,10 +208,10 @@ static inline struct mtd_info *cfi_cmdset_unknown(struct map_info *map, if (!probename) return NULL;
- probe_function = __symbol_get(probename); + probe_function = __symbol_get(probename, THIS_MODULE); if (!probe_function) { request_module("cfi_cmdset_%4.4X", type); - probe_function = __symbol_get(probename); + probe_function = __symbol_get(probename, THIS_MODULE); } kfree(probename);
diff --git a/include/linux/module.h b/include/linux/module.h index a66b9be92ef5..07a77c2618b5 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -298,8 +298,8 @@ struct notifier_block;
extern int modules_disabled; /* for sysctl */ /* Get/put a kernel symbol (calls must be symmetric) */ -void *__symbol_get(const char *symbol); -#define symbol_get(x) ((typeof(&x))(__symbol_get(__stringify(x)))) +void *__symbol_get(const char *symbol, struct module *this); +#define symbol_get(x) ((typeof(&x))(__symbol_get(__stringify(x), THIS_MODULE)))
/* modules using other modules: kdb wants to see this. */ struct module_use { diff --git a/kernel/module/main.c b/kernel/module/main.c index 6044aeba0f18..ec1baa67d6e7 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -1259,7 +1259,7 @@ static void free_module(struct module *mod) #endif }
-void *__symbol_get(const char *symbol) +void *__symbol_get(const char *symbol, struct module *this) { struct find_symbol_arg fsa = { .name = symbol, @@ -1273,6 +1273,7 @@ void *__symbol_get(const char *symbol) return NULL; } preempt_enable(); + ref_module_dependency(fsa.owner, this); return (void *)kernel_symbol_value(fsa.sym); } EXPORT_SYMBOL_GPL(__symbol_get); diff --git a/samples/hw_breakpoint/data_breakpoint.c b/samples/hw_breakpoint/data_breakpoint.c index 418c46fe5ffc..30b3261a894b 100644 --- a/samples/hw_breakpoint/data_breakpoint.c +++ b/samples/hw_breakpoint/data_breakpoint.c @@ -41,7 +41,7 @@ static int __init hw_break_module_init(void) { int ret; struct perf_event_attr attr; - void *addr = __symbol_get(ksym_name); + void *addr = __symbol_get(ksym_name, THIS_MODULE);
if (!addr) return -ENXIO;
On some devices, the hda driver needs to hook into a video driver, in order to be able to properly access the audio hardware and/or the power management function.
That's the case of several snd_hda_intel devices that depends on i915 driver.
Ensure that a proper reference between the snd-hda driver needing such binding is shown at /proc/modules, in order to allow userspace to know about such binding.
Reviewed-by: Takashi Iwai tiwai@suse.de Signed-off-by: Mauro Carvalho Chehab mchehab@kernel.org ---
See [PATCH v6 0/4] at: https://lore.kernel.org/all/cover.1652113087.git.mchehab@kernel.org/
sound/hda/hdac_component.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/hda/hdac_component.c b/sound/hda/hdac_component.c index bb37e7e0bd79..7789873ddf47 100644 --- a/sound/hda/hdac_component.c +++ b/sound/hda/hdac_component.c @@ -199,7 +199,7 @@ static int hdac_component_master_bind(struct device *dev) }
/* pin the module to avoid dynamic unbinding, but only if given */ - if (!try_module_get(acomp->ops->owner)) { + if (!try_module_get_owner(acomp->ops->owner, dev->driver->owner)) { ret = -ENODEV; goto out_unbind; }
On Mon, May 09, 2022 at 06:23:39PM +0200, Mauro Carvalho Chehab wrote:
On some devices, the hda driver needs to hook into a video driver, in order to be able to properly access the audio hardware and/or the power management function.
That's the case of several snd_hda_intel devices that depends on i915 driver.
Ensure that a proper reference between the snd-hda driver needing such binding is shown at /proc/modules, in order to allow userspace to know about such binding.
Reviewed-by: Takashi Iwai tiwai@suse.de Signed-off-by: Mauro Carvalho Chehab mchehab@kernel.org
See [PATCH v6 0/4] at: https://lore.kernel.org/all/cover.1652113087.git.mchehab@kernel.org/
sound/hda/hdac_component.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/hda/hdac_component.c b/sound/hda/hdac_component.c index bb37e7e0bd79..7789873ddf47 100644 --- a/sound/hda/hdac_component.c +++ b/sound/hda/hdac_component.c @@ -199,7 +199,7 @@ static int hdac_component_master_bind(struct device *dev) }
/* pin the module to avoid dynamic unbinding, but only if given */
- if (!try_module_get(acomp->ops->owner)) {
- if (!try_module_get_owner(acomp->ops->owner, dev->driver->owner)) {
I'm still a bit confused why snd-hda does this and why this wasn't put into component.c, but that's kinda a pre-existing issue and I guess could be fixed later on. It really shouldn't be anything specific to snd-hda here.
Anyway I scrolled through the series, it makes a lot more sense than the intial hack to me, so on the series:
Acked-by: Daniel Vetter daniel.vetter@ffwll.ch
But maybe don't count that as real review :-)
Cheers, Daniel
On Mon, May 09, 2022 at 06:23:35PM +0200, Mauro Carvalho Chehab wrote:
Currently, kernel/module annotates module dependencies when request_symbol is used, but it doesn't cover more complex inter-driver dependencies that are subsystem and/or driver-specific.
At this pount v5.18-rc7 is out and so it is too late to soak this in for the proper level of testing I'd like to see for modules-next. So I can review this after the next merge window. I'd want to beat the hell out of this and if possible I'd like to see if we can have some test coverage for the intended goal and how to break it.
Luis
Hi Luis,
On Mon, 9 May 2022 13:38:28 -0700 Luis Chamberlain mcgrof@kernel.org wrote:
On Mon, May 09, 2022 at 06:23:35PM +0200, Mauro Carvalho Chehab wrote:
Currently, kernel/module annotates module dependencies when request_symbol is used, but it doesn't cover more complex inter-driver dependencies that are subsystem and/or driver-specific.
At this pount v5.18-rc7 is out and so it is too late to soak this in for the proper level of testing I'd like to see for modules-next. So I can review this after the next merge window. I'd want to beat the hell out of this and if possible I'd like to see if we can have some test coverage for the intended goal and how to break it.
Any news with regards to this patch series?
Regards, Mauro
On Tue, Sep 20, 2022 at 07:24:54AM +0200, Mauro Carvalho Chehab wrote:
Hi Luis,
On Mon, 9 May 2022 13:38:28 -0700 Luis Chamberlain mcgrof@kernel.org wrote:
On Mon, May 09, 2022 at 06:23:35PM +0200, Mauro Carvalho Chehab wrote:
Currently, kernel/module annotates module dependencies when request_symbol is used, but it doesn't cover more complex inter-driver dependencies that are subsystem and/or driver-specific.
At this pount v5.18-rc7 is out and so it is too late to soak this in for the proper level of testing I'd like to see for modules-next. So I can review this after the next merge window. I'd want to beat the hell out of this and if possible I'd like to see if we can have some test coverage for the intended goal and how to break it.
Any news with regards to this patch series?
0-day had a rant about a bug with it, it would be wonderful if you can fix that bug and rebase. Yet again we're now on v6.0-rc7 but it doesn't mean we can't start testing all this on linux-next. I can just get this merged to linux-next as soon as this is ready for a new spin, but we certainly will have to wait until 6.2 as we haven't yet gotten proper coverage for this on v6.1.
Is there any testing situations you can think of using which can demo this a bit more separately from existing drivers, perhaps a new selftests or something?
Luis
participants (5)
-
Daniel Vetter -
kernel test robot -
Luis Chamberlain -
Mauro Carvalho Chehab -
Mauro Carvalho Chehab