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@[email protected]
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11495/bat-adlm-1/igt@[email protected]
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.html
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(-)
--
2.35.3
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 <[email protected]>
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
---
See [PATCH v6 0/4] at: https://lore.kernel.org/all/[email protected]/
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;
}
--
2.35.3
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 <[email protected]>
---
See [PATCH v6 0/4] at: https://lore.kernel.org/all/[email protected]/
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;
--
2.35.3
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
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 <[email protected]>
> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
> ---
>
> See [PATCH v6 0/4] at: https://lore.kernel.org/all/[email protected]/
>
> 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 <[email protected]>
But maybe don't count that as real review :-)
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hi Luis,
On Mon, 9 May 2022 13:38:28 -0700
Luis Chamberlain <[email protected]> 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 <[email protected]> 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