[PATCH v3 0/2] 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.
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 allow snd-hda 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.
This would allow userspace tools to 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-20220428.
---
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 (2): module: update dependencies at try_module_get() ALSA: hda - identify when audio is provided by a video driver
include/linux/module.h | 4 +++- kernel/module/main.c | 33 +++++++++++++++++++++++++++++++-- sound/hda/hdac_component.c | 2 +- 3 files changed, 35 insertions(+), 4 deletions(-)
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 Signed-off-by: Mauro Carvalho Chehab mchehab@kernel.org ---
See [PATCH v3 0/2] at: https://lore.kernel.org/all/cover.1651326000.git.mchehab@kernel.org/
include/linux/module.h | 4 +++- kernel/module/main.c | 33 +++++++++++++++++++++++++++++++-- 2 files changed, 34 insertions(+), 3 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h index 46d4d5f2516e..836851baaad4 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -620,7 +620,9 @@ 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(struct module *module, struct module *this); + +#define try_module_get(mod) __try_module_get(mod, THIS_MODULE)
extern void module_put(struct module *module);
diff --git a/kernel/module/main.c b/kernel/module/main.c index 05a42d8fcd7a..d63ebf52392b 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -631,6 +631,33 @@ 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; + + if (!this || !this->name) + return -EINVAL; + + if (mod == this) + return 0; + + mutex_lock(&module_mutex); + + ret = ref_module(this, mod); + +#ifdef CONFIG_MODULE_UNLOAD + if (ret) + goto ret; + + 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) { @@ -841,7 +868,7 @@ void __module_get(struct module *module) } EXPORT_SYMBOL(__module_get);
-bool try_module_get(struct module *module) +bool __try_module_get(struct module *module, struct module *this) { bool ret = true;
@@ -856,9 +883,11 @@ bool try_module_get(struct module *module)
preempt_enable(); } + if (ret) + ref_module_dependency(module, this); return ret; } -EXPORT_SYMBOL(try_module_get); +EXPORT_SYMBOL(__try_module_get);
void module_put(struct module *module) {
On Sat, Apr 30, 2022 at 02:41:47PM +0100, Mauro Carvalho Chehab wrote:
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 Signed-off-by: Mauro Carvalho Chehab mchehab@kernel.org
Reviewed-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
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.
Signed-off-by: Mauro Carvalho Chehab mchehab@kernel.org ---
See [PATCH v3 0/2] at: https://lore.kernel.org/all/cover.1651326000.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..30e130457272 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(acomp->ops->owner, dev->driver->owner)) { ret = -ENODEV; goto out_unbind; }
Hi Mauro,
I love your patch! Yet something to improve:
[auto build test ERROR on mcgrof/modules-next] [also build test ERROR on linus/master v5.18-rc4 next-20220429] [cannot apply to tiwai-sound/for-next] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Mauro-Carvalho-Chehab/Let-use... base: https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git modules-next config: ia64-randconfig-r023-20220428 (https://download.01.org/0day-ci/archive/20220501/202205010035.FAx0YtE1-lkp@i...) compiler: ia64-linux-gcc (GCC) 11.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/intel-lab-lkp/linux/commit/32f6557b5cc77c3cc2fcf6e68f11d9... git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Mauro-Carvalho-Chehab/Let-userspace-know-when-snd-hda-intel-needs-i915/20220430-214332 git checkout 32f6557b5cc77c3cc2fcf6e68f11d989e31c954d # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=ia64 SHELL=/bin/bash sound/hda/
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>):
In file included from arch/ia64/include/asm/pgtable.h:153, from include/linux/pgtable.h:6, from arch/ia64/include/asm/uaccess.h:40, from include/linux/uaccess.h:11, from arch/ia64/include/asm/sections.h:11, from include/linux/interrupt.h:21, from include/linux/pci.h:38, from sound/hda/hdac_component.c:6: arch/ia64/include/asm/mmu_context.h: In function 'reload_context': arch/ia64/include/asm/mmu_context.h:127:48: warning: variable 'old_rr4' set but not used [-Wunused-but-set-variable] 127 | unsigned long rr0, rr1, rr2, rr3, rr4, old_rr4; | ^~~~~~~ sound/hda/hdac_component.c: In function 'hdac_component_master_bind':
sound/hda/hdac_component.c:202:14: error: implicit declaration of function '__try_module_get'; did you mean 'try_module_get'? [-Werror=implicit-function-declaration]
202 | if (!__try_module_get(acomp->ops->owner, dev->driver->owner)) { | ^~~~~~~~~~~~~~~~ | try_module_get cc1: some warnings being treated as errors
vim +202 sound/hda/hdac_component.c
3 4 #include <linux/init.h> 5 #include <linux/module.h>
6 #include <linux/pci.h>
7 #include <linux/component.h> 8 #include <sound/core.h> 9 #include <sound/hdaudio.h> 10 #include <sound/hda_component.h> 11 #include <sound/hda_register.h> 12 13 static void hdac_acomp_release(struct device *dev, void *res) 14 { 15 } 16 17 static struct drm_audio_component *hdac_get_acomp(struct device *dev) 18 { 19 return devres_find(dev, hdac_acomp_release, NULL, NULL); 20 } 21 22 /** 23 * snd_hdac_set_codec_wakeup - Enable / disable HDMI/DP codec wakeup 24 * @bus: HDA core bus 25 * @enable: enable or disable the wakeup 26 * 27 * This function is supposed to be used only by a HD-audio controller 28 * driver that needs the interaction with graphics driver. 29 * 30 * This function should be called during the chip reset, also called at 31 * resume for updating STATESTS register read. 32 * 33 * Returns zero for success or a negative error code. 34 */ 35 int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable) 36 { 37 struct drm_audio_component *acomp = bus->audio_component; 38 39 if (!acomp || !acomp->ops) 40 return -ENODEV; 41 42 if (!acomp->ops->codec_wake_override) 43 return 0; 44 45 dev_dbg(bus->dev, "%s codec wakeup\n", 46 enable ? "enable" : "disable"); 47 48 acomp->ops->codec_wake_override(acomp->dev, enable); 49 50 return 0; 51 } 52 EXPORT_SYMBOL_GPL(snd_hdac_set_codec_wakeup); 53 54 /** 55 * snd_hdac_display_power - Power up / down the power refcount 56 * @bus: HDA core bus 57 * @idx: HDA codec address, pass HDA_CODEC_IDX_CONTROLLER for controller 58 * @enable: power up or down 59 * 60 * This function is used by either HD-audio controller or codec driver that 61 * needs the interaction with graphics driver. 62 * 63 * This function updates the power status, and calls the get_power() and 64 * put_power() ops accordingly, toggling the codec wakeup, too. 65 */ 66 void snd_hdac_display_power(struct hdac_bus *bus, unsigned int idx, bool enable) 67 { 68 struct drm_audio_component *acomp = bus->audio_component; 69 70 dev_dbg(bus->dev, "display power %s\n", 71 enable ? "enable" : "disable"); 72 73 mutex_lock(&bus->lock); 74 if (enable) 75 set_bit(idx, &bus->display_power_status); 76 else 77 clear_bit(idx, &bus->display_power_status); 78 79 if (!acomp || !acomp->ops) 80 goto unlock; 81 82 if (bus->display_power_status) { 83 if (!bus->display_power_active) { 84 unsigned long cookie = -1; 85 86 if (acomp->ops->get_power) 87 cookie = acomp->ops->get_power(acomp->dev); 88 89 snd_hdac_set_codec_wakeup(bus, true); 90 snd_hdac_set_codec_wakeup(bus, false); 91 bus->display_power_active = cookie; 92 } 93 } else { 94 if (bus->display_power_active) { 95 unsigned long cookie = bus->display_power_active; 96 97 if (acomp->ops->put_power) 98 acomp->ops->put_power(acomp->dev, cookie); 99 100 bus->display_power_active = 0; 101 } 102 } 103 unlock: 104 mutex_unlock(&bus->lock); 105 } 106 EXPORT_SYMBOL_GPL(snd_hdac_display_power); 107 108 /** 109 * snd_hdac_sync_audio_rate - Set N/CTS based on the sample rate 110 * @codec: HDA codec 111 * @nid: the pin widget NID 112 * @dev_id: device identifier 113 * @rate: the sample rate to set 114 * 115 * This function is supposed to be used only by a HD-audio controller 116 * driver that needs the interaction with graphics driver. 117 * 118 * This function sets N/CTS value based on the given sample rate. 119 * Returns zero for success, or a negative error code. 120 */ 121 int snd_hdac_sync_audio_rate(struct hdac_device *codec, hda_nid_t nid, 122 int dev_id, int rate) 123 { 124 struct hdac_bus *bus = codec->bus; 125 struct drm_audio_component *acomp = bus->audio_component; 126 int port, pipe; 127 128 if (!acomp || !acomp->ops || !acomp->ops->sync_audio_rate) 129 return -ENODEV; 130 port = nid; 131 if (acomp->audio_ops && acomp->audio_ops->pin2port) { 132 port = acomp->audio_ops->pin2port(codec, nid); 133 if (port < 0) 134 return -EINVAL; 135 } 136 pipe = dev_id; 137 return acomp->ops->sync_audio_rate(acomp->dev, port, pipe, rate); 138 } 139 EXPORT_SYMBOL_GPL(snd_hdac_sync_audio_rate); 140 141 /** 142 * snd_hdac_acomp_get_eld - Get the audio state and ELD via component 143 * @codec: HDA codec 144 * @nid: the pin widget NID 145 * @dev_id: device identifier 146 * @audio_enabled: the pointer to store the current audio state 147 * @buffer: the buffer pointer to store ELD bytes 148 * @max_bytes: the max bytes to be stored on @buffer 149 * 150 * This function is supposed to be used only by a HD-audio controller 151 * driver that needs the interaction with graphics driver. 152 * 153 * This function queries the current state of the audio on the given 154 * digital port and fetches the ELD bytes onto the given buffer. 155 * It returns the number of bytes for the total ELD data, zero for 156 * invalid ELD, or a negative error code. 157 * 158 * The return size is the total bytes required for the whole ELD bytes, 159 * thus it may be over @max_bytes. If it's over @max_bytes, it implies 160 * that only a part of ELD bytes have been fetched. 161 */ 162 int snd_hdac_acomp_get_eld(struct hdac_device *codec, hda_nid_t nid, int dev_id, 163 bool *audio_enabled, char *buffer, int max_bytes) 164 { 165 struct hdac_bus *bus = codec->bus; 166 struct drm_audio_component *acomp = bus->audio_component; 167 int port, pipe; 168 169 if (!acomp || !acomp->ops || !acomp->ops->get_eld) 170 return -ENODEV; 171 172 port = nid; 173 if (acomp->audio_ops && acomp->audio_ops->pin2port) { 174 port = acomp->audio_ops->pin2port(codec, nid); 175 if (port < 0) 176 return -EINVAL; 177 } 178 pipe = dev_id; 179 return acomp->ops->get_eld(acomp->dev, port, pipe, audio_enabled, 180 buffer, max_bytes); 181 } 182 EXPORT_SYMBOL_GPL(snd_hdac_acomp_get_eld); 183 184 static int hdac_component_master_bind(struct device *dev) 185 { 186 struct drm_audio_component *acomp = hdac_get_acomp(dev); 187 int ret; 188 189 if (WARN_ON(!acomp)) 190 return -EINVAL; 191 192 ret = component_bind_all(dev, acomp); 193 if (ret < 0) 194 return ret; 195 196 if (WARN_ON(!(acomp->dev && acomp->ops))) { 197 ret = -EINVAL; 198 goto out_unbind; 199 } 200 201 /* pin the module to avoid dynamic unbinding, but only if given */
202 if (!__try_module_get(acomp->ops->owner, dev->driver->owner)) {
203 ret = -ENODEV; 204 goto out_unbind; 205 } 206 207 if (acomp->audio_ops && acomp->audio_ops->master_bind) { 208 ret = acomp->audio_ops->master_bind(dev, acomp); 209 if (ret < 0) 210 goto module_put; 211 } 212 213 complete_all(&acomp->master_bind_complete); 214 return 0; 215 216 module_put: 217 module_put(acomp->ops->owner); 218 out_unbind: 219 component_unbind_all(dev, acomp); 220 complete_all(&acomp->master_bind_complete); 221 222 return ret; 223 } 224
Hi Mauro,
I love your patch! Yet something to improve:
[auto build test ERROR on mcgrof/modules-next] [also build test ERROR on linus/master v5.18-rc4 next-20220429] [cannot apply to tiwai-sound/for-next] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Mauro-Carvalho-Chehab/Let-use... base: https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git modules-next config: riscv-buildonly-randconfig-r003-20220428 (https://download.01.org/0day-ci/archive/20220501/202205010257.ZFhZYEG9-lkp@i...) compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 400775649969b9baf3bc2a510266e7912bb16ae9) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install riscv cross compiling tool for clang build # apt-get install binutils-riscv64-linux-gnu # https://github.com/intel-lab-lkp/linux/commit/32f6557b5cc77c3cc2fcf6e68f11d9... git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Mauro-Carvalho-Chehab/Let-userspace-know-when-snd-hda-intel-needs-i915/20220430-214332 git checkout 32f6557b5cc77c3cc2fcf6e68f11d989e31c954d # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash sound/hda/
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>):
sound/hda/hdac_component.c:202:7: error: call to undeclared function '__try_module_get'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
if (!__try_module_get(acomp->ops->owner, dev->driver->owner)) { ^ sound/hda/hdac_component.c:202:7: note: did you mean 'try_module_get'? include/linux/module.h:759:20: note: 'try_module_get' declared here static inline bool try_module_get(struct module *module) ^ 1 error generated.
vim +/__try_module_get +202 sound/hda/hdac_component.c
183 184 static int hdac_component_master_bind(struct device *dev) 185 { 186 struct drm_audio_component *acomp = hdac_get_acomp(dev); 187 int ret; 188 189 if (WARN_ON(!acomp)) 190 return -EINVAL; 191 192 ret = component_bind_all(dev, acomp); 193 if (ret < 0) 194 return ret; 195 196 if (WARN_ON(!(acomp->dev && acomp->ops))) { 197 ret = -EINVAL; 198 goto out_unbind; 199 } 200 201 /* pin the module to avoid dynamic unbinding, but only if given */
202 if (!__try_module_get(acomp->ops->owner, dev->driver->owner)) {
203 ret = -ENODEV; 204 goto out_unbind; 205 } 206 207 if (acomp->audio_ops && acomp->audio_ops->master_bind) { 208 ret = acomp->audio_ops->master_bind(dev, acomp); 209 if (ret < 0) 210 goto module_put; 211 } 212 213 complete_all(&acomp->master_bind_complete); 214 return 0; 215 216 module_put: 217 module_put(acomp->ops->owner); 218 out_unbind: 219 component_unbind_all(dev, acomp); 220 complete_all(&acomp->master_bind_complete); 221 222 return ret; 223 } 224
participants (3)
-
Greg KH -
kernel test robot -
Mauro Carvalho Chehab