* [PATCH 0/3] ALSA: hda - Avoid potential deadlock
@ 2015-09-17 10:00 Thierry Reding
2015-09-17 10:00 ` [PATCH 1/3] ALSA: hda/hdmi - Add missing MODALIAS information Thierry Reding
` (5 more replies)
0 siblings, 6 replies; 10+ messages in thread
From: Thierry Reding @ 2015-09-17 10:00 UTC (permalink / raw)
To: Takashi Iwai
Cc: alsa-devel, Tyler Baker, Kevin Hilman, Jon Hunter, linux-tegra,
linux-arm-kernel, linux-kernel
From: Thierry Reding <treding@nvidia.com>
The Tegra HDA controller driver committed in v3.16 causes deadlocks when
loaded as a module. The reason is that the driver core will lock the HDA
controller device upon calling its probe callback and the probe callback
then goes on to create child devices for detected codecs and loads their
modules via a request_module() call. This is problematic because the new
driver will immediately be bound to the device, which will in turn cause
the parent of the codec device (the HDA controller device) to be locked
again, causing a deadlock.
This problem seems to have been present since the modularization of the
HD-audio driver in commit 1289e9e8b42f ("ALSA: hda - Modularize HD-audio
driver"). On Intel platforms this has been worked around by splitting up
the probe sequence into a synchronous and an asynchronous part where the
request_module() calls are asynchronous and hence avoid the deadlock.
An alternative proposal is provided in this series of patches. Rather
than relying on explicit request_module() calls to load kernel modules
for HDA codec drivers, this implements a uevent callback for the HDA bus
to advertises the MODALIAS information to the userspace helper.
Effectively this results in the same modules being loaded, but it uses
the more canonical infrastructure to perform this. Deferring the module
loading to userspace removes the need for the explicit request_module()
calls and works around the recursive locking issue because both drivers
will be bound from separate contexts.
Thierry
Thierry Reding (3):
ALSA: hda/hdmi - Add missing MODALIAS information
ALSA: hda - Advertise MODALIAS in uevent
ALSA: hda: Do not rely on explicit module loading
sound/hda/hda_bus_type.c | 12 +++++++
sound/pci/hda/hda_bind.c | 80 ----------------------------------------------
sound/pci/hda/patch_hdmi.c | 3 ++
3 files changed, 15 insertions(+), 80 deletions(-)
--
2.5.0
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/3] ALSA: hda/hdmi - Add missing MODALIAS information 2015-09-17 10:00 [PATCH 0/3] ALSA: hda - Avoid potential deadlock Thierry Reding @ 2015-09-17 10:00 ` Thierry Reding 2015-09-17 10:00 ` [PATCH 2/3] ALSA: hda - Advertise MODALIAS in uevent Thierry Reding ` (4 subsequent siblings) 5 siblings, 0 replies; 10+ messages in thread From: Thierry Reding @ 2015-09-17 10:00 UTC (permalink / raw) To: Takashi Iwai Cc: alsa-devel, Tyler Baker, Kevin Hilman, Jon Hunter, linux-tegra, linux-arm-kernel, linux-kernel From: Thierry Reding <treding@nvidia.com> Add MODALIAS definitions for the Tegra30 HDMI, Tegra114 HDMI and Tegra210 HDMI/DP codecs. The preset match table already contains corresponding entries, but the MODALIAS list wasn't updated. Signed-off-by: Thierry Reding <treding@nvidia.com> --- sound/pci/hda/patch_hdmi.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index acbfbe087ee8..70f7d873e65e 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -3585,7 +3585,10 @@ MODULE_ALIAS("snd-hda-codec-id:10de0019"); MODULE_ALIAS("snd-hda-codec-id:10de001a"); MODULE_ALIAS("snd-hda-codec-id:10de001b"); MODULE_ALIAS("snd-hda-codec-id:10de001c"); +MODULE_ALIAS("snd-hda-codec-id:10de0020"); +MODULE_ALIAS("snd-hda-codec-id:10de0022"); MODULE_ALIAS("snd-hda-codec-id:10de0028"); +MODULE_ALIAS("snd-hda-codec-id:10de0029"); MODULE_ALIAS("snd-hda-codec-id:10de0040"); MODULE_ALIAS("snd-hda-codec-id:10de0041"); MODULE_ALIAS("snd-hda-codec-id:10de0042"); -- 2.5.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] ALSA: hda - Advertise MODALIAS in uevent 2015-09-17 10:00 [PATCH 0/3] ALSA: hda - Avoid potential deadlock Thierry Reding 2015-09-17 10:00 ` [PATCH 1/3] ALSA: hda/hdmi - Add missing MODALIAS information Thierry Reding @ 2015-09-17 10:00 ` Thierry Reding 2015-09-17 10:00 ` [PATCH 3/3] ALSA: hda: Do not rely on explicit module loading Thierry Reding ` (3 subsequent siblings) 5 siblings, 0 replies; 10+ messages in thread From: Thierry Reding @ 2015-09-17 10:00 UTC (permalink / raw) To: Takashi Iwai Cc: alsa-devel, Tyler Baker, Kevin Hilman, Jon Hunter, linux-tegra, linux-arm-kernel, linux-kernel From: Thierry Reding <treding@nvidia.com> By setting the MODALIAS variable in uevents, userspace helpers will be enabled to load modules via the module alias associated with a device. This information is required to automatically load HDA codec drivers instead of having to explicitly request the various modules in the HDA core code. Signed-off-by: Thierry Reding <treding@nvidia.com> --- sound/hda/hda_bus_type.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/sound/hda/hda_bus_type.c b/sound/hda/hda_bus_type.c index 89c2711baaaf..5adf24c9f9cc 100644 --- a/sound/hda/hda_bus_type.c +++ b/sound/hda/hda_bus_type.c @@ -63,9 +63,21 @@ static int hda_bus_match(struct device *dev, struct device_driver *drv) return 1; } +static int hda_uevent(struct device *dev, struct kobj_uevent_env *env) +{ + struct hdac_device *hdev = dev_to_hdac_dev(dev); + + if (add_uevent_var(env, "MODALIAS=snd-hda-codec-id:%08x", + hdev->vendor_id)) + return -ENOMEM; + + return 0; +} + struct bus_type snd_hda_bus_type = { .name = "hdaudio", .match = hda_bus_match, + .uevent = hda_uevent, }; EXPORT_SYMBOL_GPL(snd_hda_bus_type); -- 2.5.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/3] ALSA: hda: Do not rely on explicit module loading 2015-09-17 10:00 [PATCH 0/3] ALSA: hda - Avoid potential deadlock Thierry Reding 2015-09-17 10:00 ` [PATCH 1/3] ALSA: hda/hdmi - Add missing MODALIAS information Thierry Reding 2015-09-17 10:00 ` [PATCH 2/3] ALSA: hda - Advertise MODALIAS in uevent Thierry Reding @ 2015-09-17 10:00 ` Thierry Reding 2015-09-19 16:42 ` [PATCH 0/3] ALSA: hda - Avoid potential deadlock Takashi Iwai ` (2 subsequent siblings) 5 siblings, 0 replies; 10+ messages in thread From: Thierry Reding @ 2015-09-17 10:00 UTC (permalink / raw) To: Takashi Iwai Cc: alsa-devel, Tyler Baker, Kevin Hilman, Jon Hunter, linux-tegra, linux-arm-kernel, linux-kernel From: Thierry Reding <treding@nvidia.com> With MODALIAS information being properly exported to userspace helpers, there is no need to explicitly request modules. Upon registration of new codec devices, userspace will know which kernel module to load. This removes request_module() from the HDA controller's ->probe() path. Recursively loading a module this way causes a deadlock in the driver core because the HDA controller is locked as part of its own probe but probing its children (the HDA codec devices) will attempt to lock them again. In the past this has been worked around by splitting the probe into synchronous and asynchronous parts, where the request_module() is executed from a workqueue to avoid the deadlock. Since all the information necessary to let userspace load the proper kernel modules is now exported, there is no need for such workarounds anymore. Signed-off-by: Thierry Reding <treding@nvidia.com> --- sound/pci/hda/hda_bind.c | 80 ------------------------------------------------ 1 file changed, 80 deletions(-) diff --git a/sound/pci/hda/hda_bind.c b/sound/pci/hda/hda_bind.c index d5ac25cc7fee..bbc7e2081b0d 100644 --- a/sound/pci/hda/hda_bind.c +++ b/sound/pci/hda/hda_bind.c @@ -146,72 +146,6 @@ void hda_codec_driver_unregister(struct hda_codec_driver *drv) } EXPORT_SYMBOL_GPL(hda_codec_driver_unregister); -static inline bool codec_probed(struct hda_codec *codec) -{ - return device_attach(hda_codec_dev(codec)) > 0 && codec->preset; -} - -/* try to auto-load and bind the codec module */ -static void codec_bind_module(struct hda_codec *codec) -{ -#ifdef MODULE - request_module("snd-hda-codec-id:%08x", codec->core.vendor_id); - if (codec_probed(codec)) - return; - request_module("snd-hda-codec-id:%04x*", - (codec->core.vendor_id >> 16) & 0xffff); - if (codec_probed(codec)) - return; -#endif -} - -#if IS_ENABLED(CONFIG_SND_HDA_CODEC_HDMI) -/* if all audio out widgets are digital, let's assume the codec as a HDMI/DP */ -static bool is_likely_hdmi_codec(struct hda_codec *codec) -{ - hda_nid_t nid; - - for_each_hda_codec_node(nid, codec) { - unsigned int wcaps = get_wcaps(codec, nid); - switch (get_wcaps_type(wcaps)) { - case AC_WID_AUD_IN: - return false; /* HDMI parser supports only HDMI out */ - case AC_WID_AUD_OUT: - if (!(wcaps & AC_WCAP_DIGITAL)) - return false; - break; - } - } - return true; -} -#else -/* no HDMI codec parser support */ -#define is_likely_hdmi_codec(codec) false -#endif /* CONFIG_SND_HDA_CODEC_HDMI */ - -static int codec_bind_generic(struct hda_codec *codec) -{ - if (codec->probe_id) - return -ENODEV; - - if (is_likely_hdmi_codec(codec)) { - codec->probe_id = HDA_CODEC_ID_GENERIC_HDMI; -#if IS_MODULE(CONFIG_SND_HDA_CODEC_HDMI) - request_module("snd-hda-codec-hdmi"); -#endif - if (codec_probed(codec)) - return 0; - } - - codec->probe_id = HDA_CODEC_ID_GENERIC; -#if IS_MODULE(CONFIG_SND_HDA_GENERIC) - request_module("snd-hda-codec-generic"); -#endif - if (codec_probed(codec)) - return 0; - return -ENODEV; -} - #if IS_ENABLED(CONFIG_SND_HDA_GENERIC) #define is_generic_config(codec) \ (codec->modelname && !strcmp(codec->modelname, "generic")) @@ -241,25 +175,11 @@ int snd_hda_codec_configure(struct hda_codec *codec) if (err < 0) return err; - if (!codec->preset) - codec_bind_module(codec); - if (!codec->preset) { - err = codec_bind_generic(codec); - if (err < 0) { - codec_err(codec, "Unable to bind the codec\n"); - goto error; - } - } - /* audio codec should override the mixer name */ if (codec->core.afg || !*codec->card->mixername) snprintf(codec->card->mixername, sizeof(codec->card->mixername), "%s %s", codec->core.vendor_name, codec->core.chip_name); return 0; - - error: - snd_hdac_device_unregister(&codec->core); - return err; } EXPORT_SYMBOL_GPL(snd_hda_codec_configure); -- 2.5.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 0/3] ALSA: hda - Avoid potential deadlock 2015-09-17 10:00 [PATCH 0/3] ALSA: hda - Avoid potential deadlock Thierry Reding ` (2 preceding siblings ...) 2015-09-17 10:00 ` [PATCH 3/3] ALSA: hda: Do not rely on explicit module loading Thierry Reding @ 2015-09-19 16:42 ` Takashi Iwai 2015-09-23 9:03 ` Takashi Iwai 2015-09-23 18:01 ` Kevin Hilman 5 siblings, 0 replies; 10+ messages in thread From: Takashi Iwai @ 2015-09-19 16:42 UTC (permalink / raw) To: Thierry Reding Cc: alsa-devel, Kevin Hilman, Tyler Baker, linux-arm-kernel, Jon Hunter, linux-kernel, linux-tegra On Thu, 17 Sep 2015 12:00:03 +0200, Thierry Reding wrote: > > From: Thierry Reding <treding@nvidia.com> > > The Tegra HDA controller driver committed in v3.16 causes deadlocks when > loaded as a module. The reason is that the driver core will lock the HDA > controller device upon calling its probe callback and the probe callback > then goes on to create child devices for detected codecs and loads their > modules via a request_module() call. This is problematic because the new > driver will immediately be bound to the device, which will in turn cause > the parent of the codec device (the HDA controller device) to be locked > again, causing a deadlock. > > This problem seems to have been present since the modularization of the > HD-audio driver in commit 1289e9e8b42f ("ALSA: hda - Modularize HD-audio > driver"). On Intel platforms this has been worked around by splitting up > the probe sequence into a synchronous and an asynchronous part where the > request_module() calls are asynchronous and hence avoid the deadlock. > > An alternative proposal is provided in this series of patches. Rather > than relying on explicit request_module() calls to load kernel modules > for HDA codec drivers, this implements a uevent callback for the HDA bus > to advertises the MODALIAS information to the userspace helper. > > Effectively this results in the same modules being loaded, but it uses > the more canonical infrastructure to perform this. Deferring the module > loading to userspace removes the need for the explicit request_module() > calls and works around the recursive locking issue because both drivers > will be bound from separate contexts. This looks like a sane cleanup at a glance, but I can't review in details now because I'm traveling on vacation. I'll take a closer look in the next week. thanks, Takashi > > Thierry > > Thierry Reding (3): > ALSA: hda/hdmi - Add missing MODALIAS information > ALSA: hda - Advertise MODALIAS in uevent > ALSA: hda: Do not rely on explicit module loading > > sound/hda/hda_bus_type.c | 12 +++++++ > sound/pci/hda/hda_bind.c | 80 ---------------------------------------------- > sound/pci/hda/patch_hdmi.c | 3 ++ > 3 files changed, 15 insertions(+), 80 deletions(-) > > -- > 2.5.0 > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/3] ALSA: hda - Avoid potential deadlock 2015-09-17 10:00 [PATCH 0/3] ALSA: hda - Avoid potential deadlock Thierry Reding ` (3 preceding siblings ...) 2015-09-19 16:42 ` [PATCH 0/3] ALSA: hda - Avoid potential deadlock Takashi Iwai @ 2015-09-23 9:03 ` Takashi Iwai 2015-09-24 9:49 ` Takashi Iwai 2015-09-23 18:01 ` Kevin Hilman 5 siblings, 1 reply; 10+ messages in thread From: Takashi Iwai @ 2015-09-23 9:03 UTC (permalink / raw) To: Thierry Reding Cc: alsa-devel, Kevin Hilman, Tyler Baker, linux-arm-kernel, Jon Hunter, linux-kernel, linux-tegra On Thu, 17 Sep 2015 12:00:03 +0200, Thierry Reding wrote: > > From: Thierry Reding <treding@nvidia.com> > > The Tegra HDA controller driver committed in v3.16 causes deadlocks when > loaded as a module. The reason is that the driver core will lock the HDA > controller device upon calling its probe callback and the probe callback > then goes on to create child devices for detected codecs and loads their > modules via a request_module() call. This is problematic because the new > driver will immediately be bound to the device, which will in turn cause > the parent of the codec device (the HDA controller device) to be locked > again, causing a deadlock. > > This problem seems to have been present since the modularization of the > HD-audio driver in commit 1289e9e8b42f ("ALSA: hda - Modularize HD-audio > driver"). On Intel platforms this has been worked around by splitting up > the probe sequence into a synchronous and an asynchronous part where the > request_module() calls are asynchronous and hence avoid the deadlock. > > An alternative proposal is provided in this series of patches. Rather > than relying on explicit request_module() calls to load kernel modules > for HDA codec drivers, this implements a uevent callback for the HDA bus > to advertises the MODALIAS information to the userspace helper. > > Effectively this results in the same modules being loaded, but it uses > the more canonical infrastructure to perform this. Deferring the module > loading to userspace removes the need for the explicit request_module() > calls and works around the recursive locking issue because both drivers > will be bound from separate contexts. While this looks definitely like the right direction to go, I'm afraid that this will give a few major regressions. First off, there is no way to bind with the generic codec driver. There are two generic drivers, one for HDMI/DP and one for normal audio. Binding to them is judged by parsing the codec widgets whether they are digital-only. So, either user-space or kernel needs to parse the codec widgets beforehand. If we rip off all binding magic as in your patch, this has to be done by udev. With the sysfs stuff, now it should be possible, but this would break the existing system. Another possible regression is the matching with the vendor-only alias. Maybe the current wildcard works, but we need to double check. So, unless these are addressed, I think we need another quick band-aid over snd-hda-tegra just doing the async probe like snd-hda-intel. Of course, as already written, converting to the standard udev probe would be best. We can finally get rid of the manual alias entries by tweaking the scripts/* stuff. But I'd like to avoid regressions as the first priority. thanks, Takashi ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/3] ALSA: hda - Avoid potential deadlock 2015-09-23 9:03 ` Takashi Iwai @ 2015-09-24 9:49 ` Takashi Iwai 2015-09-24 10:50 ` Thierry Reding 0 siblings, 1 reply; 10+ messages in thread From: Takashi Iwai @ 2015-09-24 9:49 UTC (permalink / raw) To: Thierry Reding Cc: alsa-devel, Kevin Hilman, Tyler Baker, linux-arm-kernel, Jon Hunter, linux-kernel, linux-tegra On Wed, 23 Sep 2015 11:03:44 +0200, Takashi Iwai wrote: > > On Thu, 17 Sep 2015 12:00:03 +0200, > Thierry Reding wrote: > > > > From: Thierry Reding <treding@nvidia.com> > > > > The Tegra HDA controller driver committed in v3.16 causes deadlocks when > > loaded as a module. The reason is that the driver core will lock the HDA > > controller device upon calling its probe callback and the probe callback > > then goes on to create child devices for detected codecs and loads their > > modules via a request_module() call. This is problematic because the new > > driver will immediately be bound to the device, which will in turn cause > > the parent of the codec device (the HDA controller device) to be locked > > again, causing a deadlock. > > > > This problem seems to have been present since the modularization of the > > HD-audio driver in commit 1289e9e8b42f ("ALSA: hda - Modularize HD-audio > > driver"). On Intel platforms this has been worked around by splitting up > > the probe sequence into a synchronous and an asynchronous part where the > > request_module() calls are asynchronous and hence avoid the deadlock. > > > > An alternative proposal is provided in this series of patches. Rather > > than relying on explicit request_module() calls to load kernel modules > > for HDA codec drivers, this implements a uevent callback for the HDA bus > > to advertises the MODALIAS information to the userspace helper. > > > > Effectively this results in the same modules being loaded, but it uses > > the more canonical infrastructure to perform this. Deferring the module > > loading to userspace removes the need for the explicit request_module() > > calls and works around the recursive locking issue because both drivers > > will be bound from separate contexts. > > While this looks definitely like the right direction to go, I'm afraid > that this will give a few major regressions. First off, there is no > way to bind with the generic codec driver. There are two generic > drivers, one for HDMI/DP and one for normal audio. Binding to them is > judged by parsing the codec widgets whether they are digital-only. > So, either user-space or kernel needs to parse the codec widgets > beforehand. If we rip off all binding magic as in your patch, this > has to be done by udev. With the sysfs stuff, now it should be > possible, but this would break the existing system. > > Another possible regression is the matching with the vendor-only > alias. Maybe the current wildcard works, but we need to double > check. > > So, unless these are addressed, I think we need another quick band-aid > over snd-hda-tegra just doing the async probe like snd-hda-intel. Does the patch below work? I only did a quick compile test. thanks, Takashi -- 8< -- From: Takashi Iwai <tiwai@suse.de> Subject: [PATCH] ALSA: hda/tegra - async probe for avoiding module loading deadlock The Tegra HD-audio controller driver causes deadlocks when loaded as a module since the driver invokes request_module() at binding with the codec driver. This patch works around it by deferring the probe in a work like Intel HD-audio controller driver does. Although hovering the codec probe stuff into udev would be a better solution, it may cause other regressions, so let's try this band-aid fix until the more proper solution gets landed. Reported-by: Thierry Reding <treding@nvidia.com> Cc: <stable@vger.kernel.org> Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/pci/hda/hda_tegra.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c index 477742cb70a2..58c0aad37284 100644 --- a/sound/pci/hda/hda_tegra.c +++ b/sound/pci/hda/hda_tegra.c @@ -73,6 +73,7 @@ struct hda_tegra { struct clk *hda2codec_2x_clk; struct clk *hda2hdmi_clk; void __iomem *regs; + struct work_struct probe_work; }; #ifdef CONFIG_PM @@ -294,7 +295,9 @@ static int hda_tegra_dev_disconnect(struct snd_device *device) static int hda_tegra_dev_free(struct snd_device *device) { struct azx *chip = device->device_data; + struct hda_tegra *hda = container_of(chip, struct hda_tegra, chip); + cancel_work_sync(&hda->probe_work); if (azx_bus(chip)->chip_init) { azx_stop_all_streams(chip); azx_stop_chip(chip); @@ -426,6 +429,9 @@ static int hda_tegra_first_init(struct azx *chip, struct platform_device *pdev) /* * constructor */ + +static void hda_tegra_probe_work(struct work_struct *work); + static int hda_tegra_create(struct snd_card *card, unsigned int driver_caps, struct hda_tegra *hda) @@ -452,6 +458,8 @@ static int hda_tegra_create(struct snd_card *card, chip->single_cmd = false; chip->snoop = true; + INIT_WORK(&hda->probe_work, hda_tegra_probe_work); + err = azx_bus_init(chip, NULL, &hda_tegra_io_ops); if (err < 0) return err; @@ -499,6 +507,21 @@ static int hda_tegra_probe(struct platform_device *pdev) card->private_data = chip; dev_set_drvdata(&pdev->dev, card); + schedule_work(&hda->probe_work); + + return 0; + +out_free: + snd_card_free(card); + return err; +} + +static void hda_tegra_probe_work(struct work_struct *work) +{ + struct hda_tegra *hda = container_of(work, struct hda_tegra, probe_work); + struct azx *chip = &hda->chip; + struct platform_device *pdev = to_platform_device(hda->dev); + int err; err = hda_tegra_first_init(chip, pdev); if (err < 0) @@ -520,11 +543,8 @@ static int hda_tegra_probe(struct platform_device *pdev) chip->running = 1; snd_hda_set_power_save(&chip->bus, power_save * 1000); - return 0; - -out_free: - snd_card_free(card); - return err; + out_free: + return; /* no error return from async probe */ } static int hda_tegra_remove(struct platform_device *pdev) -- 2.5.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 0/3] ALSA: hda - Avoid potential deadlock 2015-09-24 9:49 ` Takashi Iwai @ 2015-09-24 10:50 ` Thierry Reding 2015-09-24 11:49 ` Takashi Iwai 0 siblings, 1 reply; 10+ messages in thread From: Thierry Reding @ 2015-09-24 10:50 UTC (permalink / raw) To: Takashi Iwai Cc: alsa-devel, Kevin Hilman, Tyler Baker, linux-arm-kernel, Jon Hunter, linux-kernel, linux-tegra [-- Attachment #1: Type: text/plain, Size: 6642 bytes --] On Thu, Sep 24, 2015 at 11:49:57AM +0200, Takashi Iwai wrote: > On Wed, 23 Sep 2015 11:03:44 +0200, > Takashi Iwai wrote: > > > > On Thu, 17 Sep 2015 12:00:03 +0200, > > Thierry Reding wrote: > > > > > > From: Thierry Reding <treding@nvidia.com> > > > > > > The Tegra HDA controller driver committed in v3.16 causes deadlocks when > > > loaded as a module. The reason is that the driver core will lock the HDA > > > controller device upon calling its probe callback and the probe callback > > > then goes on to create child devices for detected codecs and loads their > > > modules via a request_module() call. This is problematic because the new > > > driver will immediately be bound to the device, which will in turn cause > > > the parent of the codec device (the HDA controller device) to be locked > > > again, causing a deadlock. > > > > > > This problem seems to have been present since the modularization of the > > > HD-audio driver in commit 1289e9e8b42f ("ALSA: hda - Modularize HD-audio > > > driver"). On Intel platforms this has been worked around by splitting up > > > the probe sequence into a synchronous and an asynchronous part where the > > > request_module() calls are asynchronous and hence avoid the deadlock. > > > > > > An alternative proposal is provided in this series of patches. Rather > > > than relying on explicit request_module() calls to load kernel modules > > > for HDA codec drivers, this implements a uevent callback for the HDA bus > > > to advertises the MODALIAS information to the userspace helper. > > > > > > Effectively this results in the same modules being loaded, but it uses > > > the more canonical infrastructure to perform this. Deferring the module > > > loading to userspace removes the need for the explicit request_module() > > > calls and works around the recursive locking issue because both drivers > > > will be bound from separate contexts. > > > > While this looks definitely like the right direction to go, I'm afraid > > that this will give a few major regressions. First off, there is no > > way to bind with the generic codec driver. There are two generic > > drivers, one for HDMI/DP and one for normal audio. Binding to them is > > judged by parsing the codec widgets whether they are digital-only. > > So, either user-space or kernel needs to parse the codec widgets > > beforehand. If we rip off all binding magic as in your patch, this > > has to be done by udev. With the sysfs stuff, now it should be > > possible, but this would break the existing system. > > > > Another possible regression is the matching with the vendor-only > > alias. Maybe the current wildcard works, but we need to double > > check. > > > > So, unless these are addressed, I think we need another quick band-aid > > over snd-hda-tegra just doing the async probe like snd-hda-intel. > > Does the patch below work? I only did a quick compile test. > > > thanks, > > Takashi > > -- 8< -- > From: Takashi Iwai <tiwai@suse.de> > Subject: [PATCH] ALSA: hda/tegra - async probe for avoiding module loading > deadlock > > The Tegra HD-audio controller driver causes deadlocks when loaded as a > module since the driver invokes request_module() at binding with the > codec driver. This patch works around it by deferring the probe in a > work like Intel HD-audio controller driver does. Although hovering > the codec probe stuff into udev would be a better solution, it may > cause other regressions, so let's try this band-aid fix until the more > proper solution gets landed. > > Reported-by: Thierry Reding <treding@nvidia.com> > Cc: <stable@vger.kernel.org> > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > sound/pci/hda/hda_tegra.c | 30 +++++++++++++++++++++++++----- > 1 file changed, 25 insertions(+), 5 deletions(-) Yes, that fixes the hang that I was seeing: Tested-by: Thierry Reding <treding@nvidia.com> As a matter of fact this resembles a patch that Jon had worked on to solve this. I'm slightly concerned that merging a band-aid like this is going to remove any incentive to fix this properly, though. Thierry > diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c > index 477742cb70a2..58c0aad37284 100644 > --- a/sound/pci/hda/hda_tegra.c > +++ b/sound/pci/hda/hda_tegra.c > @@ -73,6 +73,7 @@ struct hda_tegra { > struct clk *hda2codec_2x_clk; > struct clk *hda2hdmi_clk; > void __iomem *regs; > + struct work_struct probe_work; > }; > > #ifdef CONFIG_PM > @@ -294,7 +295,9 @@ static int hda_tegra_dev_disconnect(struct snd_device *device) > static int hda_tegra_dev_free(struct snd_device *device) > { > struct azx *chip = device->device_data; > + struct hda_tegra *hda = container_of(chip, struct hda_tegra, chip); > > + cancel_work_sync(&hda->probe_work); > if (azx_bus(chip)->chip_init) { > azx_stop_all_streams(chip); > azx_stop_chip(chip); > @@ -426,6 +429,9 @@ static int hda_tegra_first_init(struct azx *chip, struct platform_device *pdev) > /* > * constructor > */ > + > +static void hda_tegra_probe_work(struct work_struct *work); > + > static int hda_tegra_create(struct snd_card *card, > unsigned int driver_caps, > struct hda_tegra *hda) > @@ -452,6 +458,8 @@ static int hda_tegra_create(struct snd_card *card, > chip->single_cmd = false; > chip->snoop = true; > > + INIT_WORK(&hda->probe_work, hda_tegra_probe_work); > + > err = azx_bus_init(chip, NULL, &hda_tegra_io_ops); > if (err < 0) > return err; > @@ -499,6 +507,21 @@ static int hda_tegra_probe(struct platform_device *pdev) > card->private_data = chip; > > dev_set_drvdata(&pdev->dev, card); > + schedule_work(&hda->probe_work); > + > + return 0; > + > +out_free: > + snd_card_free(card); > + return err; > +} > + > +static void hda_tegra_probe_work(struct work_struct *work) > +{ > + struct hda_tegra *hda = container_of(work, struct hda_tegra, probe_work); > + struct azx *chip = &hda->chip; > + struct platform_device *pdev = to_platform_device(hda->dev); > + int err; > > err = hda_tegra_first_init(chip, pdev); > if (err < 0) > @@ -520,11 +543,8 @@ static int hda_tegra_probe(struct platform_device *pdev) > chip->running = 1; > snd_hda_set_power_save(&chip->bus, power_save * 1000); > > - return 0; > - > -out_free: > - snd_card_free(card); > - return err; > + out_free: > + return; /* no error return from async probe */ > } > > static int hda_tegra_remove(struct platform_device *pdev) > -- > 2.5.1 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/3] ALSA: hda - Avoid potential deadlock 2015-09-24 10:50 ` Thierry Reding @ 2015-09-24 11:49 ` Takashi Iwai 0 siblings, 0 replies; 10+ messages in thread From: Takashi Iwai @ 2015-09-24 11:49 UTC (permalink / raw) To: Thierry Reding Cc: alsa-devel, Kevin Hilman, Tyler Baker, linux-arm-kernel, Jon Hunter, linux-kernel, linux-tegra On Thu, 24 Sep 2015 12:50:10 +0200, Thierry Reding wrote: > > On Thu, Sep 24, 2015 at 11:49:57AM +0200, Takashi Iwai wrote: > > On Wed, 23 Sep 2015 11:03:44 +0200, > > Takashi Iwai wrote: > > > > > > On Thu, 17 Sep 2015 12:00:03 +0200, > > > Thierry Reding wrote: > > > > > > > > From: Thierry Reding <treding@nvidia.com> > > > > > > > > The Tegra HDA controller driver committed in v3.16 causes deadlocks when > > > > loaded as a module. The reason is that the driver core will lock the HDA > > > > controller device upon calling its probe callback and the probe callback > > > > then goes on to create child devices for detected codecs and loads their > > > > modules via a request_module() call. This is problematic because the new > > > > driver will immediately be bound to the device, which will in turn cause > > > > the parent of the codec device (the HDA controller device) to be locked > > > > again, causing a deadlock. > > > > > > > > This problem seems to have been present since the modularization of the > > > > HD-audio driver in commit 1289e9e8b42f ("ALSA: hda - Modularize HD-audio > > > > driver"). On Intel platforms this has been worked around by splitting up > > > > the probe sequence into a synchronous and an asynchronous part where the > > > > request_module() calls are asynchronous and hence avoid the deadlock. > > > > > > > > An alternative proposal is provided in this series of patches. Rather > > > > than relying on explicit request_module() calls to load kernel modules > > > > for HDA codec drivers, this implements a uevent callback for the HDA bus > > > > to advertises the MODALIAS information to the userspace helper. > > > > > > > > Effectively this results in the same modules being loaded, but it uses > > > > the more canonical infrastructure to perform this. Deferring the module > > > > loading to userspace removes the need for the explicit request_module() > > > > calls and works around the recursive locking issue because both drivers > > > > will be bound from separate contexts. > > > > > > While this looks definitely like the right direction to go, I'm afraid > > > that this will give a few major regressions. First off, there is no > > > way to bind with the generic codec driver. There are two generic > > > drivers, one for HDMI/DP and one for normal audio. Binding to them is > > > judged by parsing the codec widgets whether they are digital-only. > > > So, either user-space or kernel needs to parse the codec widgets > > > beforehand. If we rip off all binding magic as in your patch, this > > > has to be done by udev. With the sysfs stuff, now it should be > > > possible, but this would break the existing system. > > > > > > Another possible regression is the matching with the vendor-only > > > alias. Maybe the current wildcard works, but we need to double > > > check. > > > > > > So, unless these are addressed, I think we need another quick band-aid > > > over snd-hda-tegra just doing the async probe like snd-hda-intel. > > > > Does the patch below work? I only did a quick compile test. > > > > > > thanks, > > > > Takashi > > > > -- 8< -- > > From: Takashi Iwai <tiwai@suse.de> > > Subject: [PATCH] ALSA: hda/tegra - async probe for avoiding module loading > > deadlock > > > > The Tegra HD-audio controller driver causes deadlocks when loaded as a > > module since the driver invokes request_module() at binding with the > > codec driver. This patch works around it by deferring the probe in a > > work like Intel HD-audio controller driver does. Although hovering > > the codec probe stuff into udev would be a better solution, it may > > cause other regressions, so let's try this band-aid fix until the more > > proper solution gets landed. > > > > Reported-by: Thierry Reding <treding@nvidia.com> > > Cc: <stable@vger.kernel.org> > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > --- > > sound/pci/hda/hda_tegra.c | 30 +++++++++++++++++++++++++----- > > 1 file changed, 25 insertions(+), 5 deletions(-) > > Yes, that fixes the hang that I was seeing: > > Tested-by: Thierry Reding <treding@nvidia.com> Thanks! I'll queue this for the next pull request. > As a matter of fact this resembles a patch that Jon had worked on to > solve this. I'm slightly concerned that merging a band-aid like this > is going to remove any incentive to fix this properly, though. Yeah, it's neither elegant nor cleaner solution but it's certainly safer. Takashi > Thierry > > > diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c > > index 477742cb70a2..58c0aad37284 100644 > > --- a/sound/pci/hda/hda_tegra.c > > +++ b/sound/pci/hda/hda_tegra.c > > @@ -73,6 +73,7 @@ struct hda_tegra { > > struct clk *hda2codec_2x_clk; > > struct clk *hda2hdmi_clk; > > void __iomem *regs; > > + struct work_struct probe_work; > > }; > > > > #ifdef CONFIG_PM > > @@ -294,7 +295,9 @@ static int hda_tegra_dev_disconnect(struct snd_device *device) > > static int hda_tegra_dev_free(struct snd_device *device) > > { > > struct azx *chip = device->device_data; > > + struct hda_tegra *hda = container_of(chip, struct hda_tegra, chip); > > > > + cancel_work_sync(&hda->probe_work); > > if (azx_bus(chip)->chip_init) { > > azx_stop_all_streams(chip); > > azx_stop_chip(chip); > > @@ -426,6 +429,9 @@ static int hda_tegra_first_init(struct azx *chip, struct platform_device *pdev) > > /* > > * constructor > > */ > > + > > +static void hda_tegra_probe_work(struct work_struct *work); > > + > > static int hda_tegra_create(struct snd_card *card, > > unsigned int driver_caps, > > struct hda_tegra *hda) > > @@ -452,6 +458,8 @@ static int hda_tegra_create(struct snd_card *card, > > chip->single_cmd = false; > > chip->snoop = true; > > > > + INIT_WORK(&hda->probe_work, hda_tegra_probe_work); > > + > > err = azx_bus_init(chip, NULL, &hda_tegra_io_ops); > > if (err < 0) > > return err; > > @@ -499,6 +507,21 @@ static int hda_tegra_probe(struct platform_device *pdev) > > card->private_data = chip; > > > > dev_set_drvdata(&pdev->dev, card); > > + schedule_work(&hda->probe_work); > > + > > + return 0; > > + > > +out_free: > > + snd_card_free(card); > > + return err; > > +} > > + > > +static void hda_tegra_probe_work(struct work_struct *work) > > +{ > > + struct hda_tegra *hda = container_of(work, struct hda_tegra, probe_work); > > + struct azx *chip = &hda->chip; > > + struct platform_device *pdev = to_platform_device(hda->dev); > > + int err; > > > > err = hda_tegra_first_init(chip, pdev); > > if (err < 0) > > @@ -520,11 +543,8 @@ static int hda_tegra_probe(struct platform_device *pdev) > > chip->running = 1; > > snd_hda_set_power_save(&chip->bus, power_save * 1000); > > > > - return 0; > > - > > -out_free: > > - snd_card_free(card); > > - return err; > > + out_free: > > + return; /* no error return from async probe */ > > } > > > > static int hda_tegra_remove(struct platform_device *pdev) > > -- > > 2.5.1 > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/3] ALSA: hda - Avoid potential deadlock 2015-09-17 10:00 [PATCH 0/3] ALSA: hda - Avoid potential deadlock Thierry Reding ` (4 preceding siblings ...) 2015-09-23 9:03 ` Takashi Iwai @ 2015-09-23 18:01 ` Kevin Hilman 5 siblings, 0 replies; 10+ messages in thread From: Kevin Hilman @ 2015-09-23 18:01 UTC (permalink / raw) To: Thierry Reding Cc: Takashi Iwai, alsa-devel, Tyler Baker, Jon Hunter, linux-tegra, linux-arm-kernel, linux-kernel Thierry Reding <thierry.reding@gmail.com> writes: > From: Thierry Reding <treding@nvidia.com> > > The Tegra HDA controller driver committed in v3.16 causes deadlocks when > loaded as a module. The reason is that the driver core will lock the HDA > controller device upon calling its probe callback and the probe callback > then goes on to create child devices for detected codecs and loads their > modules via a request_module() call. This is problematic because the new > driver will immediately be bound to the device, which will in turn cause > the parent of the codec device (the HDA controller device) to be locked > again, causing a deadlock. > > This problem seems to have been present since the modularization of the > HD-audio driver in commit 1289e9e8b42f ("ALSA: hda - Modularize HD-audio > driver"). On Intel platforms this has been worked around by splitting up > the probe sequence into a synchronous and an asynchronous part where the > request_module() calls are asynchronous and hence avoid the deadlock. > > An alternative proposal is provided in this series of patches. Rather > than relying on explicit request_module() calls to load kernel modules > for HDA codec drivers, this implements a uevent callback for the HDA bus > to advertises the MODALIAS information to the userspace helper. > > Effectively this results in the same modules being loaded, but it uses > the more canonical infrastructure to perform this. Deferring the module > loading to userspace removes the need for the explicit request_module() > calls and works around the recursive locking issue because both drivers > will be bound from separate contexts. Tested-by: Kevin Hilman <khilman@linaro.org> FWIW, I tested this on top of next-20150923, and it solves the boot problem I'm seeing on tegra124-jetson-tk1 with module loading. Kevin ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-09-24 11:49 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-09-17 10:00 [PATCH 0/3] ALSA: hda - Avoid potential deadlock Thierry Reding 2015-09-17 10:00 ` [PATCH 1/3] ALSA: hda/hdmi - Add missing MODALIAS information Thierry Reding 2015-09-17 10:00 ` [PATCH 2/3] ALSA: hda - Advertise MODALIAS in uevent Thierry Reding 2015-09-17 10:00 ` [PATCH 3/3] ALSA: hda: Do not rely on explicit module loading Thierry Reding 2015-09-19 16:42 ` [PATCH 0/3] ALSA: hda - Avoid potential deadlock Takashi Iwai 2015-09-23 9:03 ` Takashi Iwai 2015-09-24 9:49 ` Takashi Iwai 2015-09-24 10:50 ` Thierry Reding 2015-09-24 11:49 ` Takashi Iwai 2015-09-23 18:01 ` Kevin Hilman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).