[RFC PATCH 0/2] ALSA: hda - acomp probe fix for i915
Hi,
this simple bugfix started to feel a bit like getting stuck in quicksand, so I'm looking for some early input via this RFC series.
Basicly hdac_i915.c should not use global state to track communication with i915 driver. But how to get handle of "hdac_bus*? I considered a few options:
1) add hdac_bus as a member of drm_audio_component.h -> seems wrong as this is really an audio side implementation)
2) embed copy of drm_audio_component to 'struct hdac_bus', so I could use container_of() on the device handle to get to the bus -> wasted space to keep a copy at hdac_bus level (note: snd-hda-codec-hdmi do this by embedding a copy of ops to "struct hdmi_spec")
3) add another devres entry to store the hdac_bus directly in acomp_init and a new helper function to query it
I now implemented option 3 in this RFC series as it seemed cleanest and most local to hdac_component.c, where the problem stems from. It's still somewhat messy, and I'm wondering if I'm overlooking some obvious alternative. We could dig this deeper into i915 specific code, but OTOH, hdac_bus is an argument snd_hdac_acomp_init(), so it's common for all.
Kai Vehmanen (2): ALSA: hda - keep track of HDA core bus instance in acomp ALSA: hda/i915 - fix list corruption with concurrent probes
include/sound/hda_component.h | 5 +++++ include/sound/hdaudio.h | 2 ++ sound/hda/hdac_component.c | 34 ++++++++++++++++++++++++++++++++++ sound/hda/hdac_i915.c | 16 +++++++++------- 4 files changed, 50 insertions(+), 7 deletions(-)
In current HDA component implementation, HDA controller drivers use snd_hdac_acomp_init() and pass it the HDA bus instance (hdac_bus). When registration to component framework is done, the HDA controller device instance 'hdac_bus->dev' is used.
In the component bind/unbind callbacks, only the device handle is passed as context, and it is not possible to look up the HDA bus instance. Fix this gap by adding a separate devres entry for the bus handle.
Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com --- include/sound/hda_component.h | 5 +++++ sound/hda/hdac_component.c | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+)
diff --git a/include/sound/hda_component.h b/include/sound/hda_component.h index d4804c72d959..476cc4e2083c 100644 --- a/include/sound/hda_component.h +++ b/include/sound/hda_component.h @@ -25,6 +25,7 @@ int snd_hdac_acomp_init(struct hdac_bus *bus, int snd_hdac_acomp_exit(struct hdac_bus *bus); int snd_hdac_acomp_register_notifier(struct hdac_bus *bus, const struct drm_audio_component_audio_ops *ops); +struct hdac_bus *snd_hdac_acomp_get_bus(struct device *dev); #else static inline int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable) { @@ -62,6 +63,10 @@ static inline int snd_hdac_acomp_register_notifier(struct hdac_bus *bus, { return -ENODEV; } +static struct hdac_bus *snd_hdac_acomp_get_bus(struct device *dev) +{ + return NULL; +} #endif
#endif /* __SOUND_HDA_COMPONENT_H */ diff --git a/sound/hda/hdac_component.c b/sound/hda/hdac_component.c index 89126c6fd216..d9d2675982f0 100644 --- a/sound/hda/hdac_component.c +++ b/sound/hda/hdac_component.c @@ -19,6 +19,29 @@ static struct drm_audio_component *hdac_get_acomp(struct device *dev) return devres_find(dev, hdac_acomp_release, NULL, NULL); }
+static void hdac_acomp_bus_release(struct device *dev, void *res) +{ +} + +/** + * snd_hdac_acomp_get_bus - Get HDA core bus instance + * @dev: device + * + * If audio component registration has been done, this function + * will return the matching HDA core bus. Returns NULL otherwise. + */ +struct hdac_bus *snd_hdac_acomp_get_bus(struct device *dev) +{ + struct drm_audio_component *acomp = hdac_get_acomp(dev); + struct hdac_bus **bus = + devres_find(dev, hdac_acomp_bus_release, NULL, NULL); + if (!acomp || !*bus) + return NULL; + + return *bus; +} +EXPORT_SYMBOL_GPL(snd_hdac_acomp_get_bus); + /** * snd_hdac_set_codec_wakeup - Enable / disable HDMI/DP codec wakeup * @bus: HDA core bus @@ -286,6 +309,7 @@ int snd_hdac_acomp_init(struct hdac_bus *bus, struct component_match *match = NULL; struct device *dev = bus->dev; struct drm_audio_component *acomp; + struct hdac_bus **bus_ptr; int ret;
if (WARN_ON(hdac_get_acomp(dev))) @@ -299,6 +323,14 @@ int snd_hdac_acomp_init(struct hdac_bus *bus, bus->audio_component = acomp; devres_add(dev, acomp);
+ bus_ptr = devres_alloc(hdac_acomp_bus_release, sizeof(*bus_ptr), GFP_KERNEL); + if (!bus_ptr) { + ret = -ENOMEM; + goto out_err_busptr; + } + *bus_ptr = bus; + devres_add(dev, bus_ptr); + component_match_add_typed(dev, &match, match_master, bus); ret = component_master_add_with_match(dev, &hdac_component_master_ops, match); @@ -308,6 +340,8 @@ int snd_hdac_acomp_init(struct hdac_bus *bus, return 0;
out_err: + devres_destroy(dev, hdac_acomp_bus_release, NULL, NULL); +out_err_busptr: bus->audio_component = NULL; devres_destroy(dev, hdac_acomp_release, NULL, NULL); dev_info(dev, "failed to add audio component master (%d)\n", ret);
Current hdac_i915 uses a static completion instance to synchronous communication with i915 driver. This design is not safe if multiple HDA controllers are active and talking to different i915 instances, and can lead to list corruption and failed audio driver probe.
Fix the design by storing the completion object to hdac_bus, and signaling completions on a per-bus basis.
Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com --- include/sound/hdaudio.h | 2 ++ sound/hda/hdac_i915.c | 16 +++++++++------- 2 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h index 6eed61e6cf8a..116c89074d79 100644 --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -6,6 +6,7 @@ #ifndef __SOUND_HDAUDIO_H #define __SOUND_HDAUDIO_H
+#include <linux/completion.h> #include <linux/device.h> #include <linux/interrupt.h> #include <linux/io.h> @@ -359,6 +360,7 @@ struct hdac_bus { struct drm_audio_component *audio_component; long display_power_status; unsigned long display_power_active; + struct completion display_bind_complete;
/* parameters required for enhanced capabilities */ int num_streams; diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c index 5f0a1aa6ad84..fee3d379d7e0 100644 --- a/sound/hda/hdac_i915.c +++ b/sound/hda/hdac_i915.c @@ -11,8 +11,6 @@ #include <sound/hda_i915.h> #include <sound/hda_register.h>
-static struct completion bind_complete; - #define IS_HSW_CONTROLLER(pci) (((pci)->device == 0x0a0c) || \ ((pci)->device == 0x0c0c) || \ ((pci)->device == 0x0d0c) || \ @@ -133,9 +131,14 @@ static bool i915_gfx_present(void) static int i915_master_bind(struct device *dev, struct drm_audio_component *acomp) { - complete_all(&bind_complete); + struct hdac_bus *bus = snd_hdac_acomp_get_bus(dev); + + if (!bus) + return -EINVAL; + complete_all(&bus->display_bind_complete); /* clear audio_ops here as it was needed only for completion call */ acomp->audio_ops = NULL; + return 0; }
@@ -163,8 +166,7 @@ int snd_hdac_i915_init(struct hdac_bus *bus) if (!i915_gfx_present()) return -ENODEV;
- init_completion(&bind_complete); - + init_completion(&bus->display_bind_complete); err = snd_hdac_acomp_init(bus, &i915_init_ops, i915_component_master_match, sizeof(struct i915_audio_component) - sizeof(*acomp)); @@ -177,8 +179,8 @@ int snd_hdac_i915_init(struct hdac_bus *bus) if (!IS_ENABLED(CONFIG_MODULES) || !request_module("i915")) { /* 60s timeout */ - wait_for_completion_timeout(&bind_complete, - msecs_to_jiffies(60 * 1000)); + wait_for_completion_timeout(&bus->display_bind_complete, + msecs_to_jiffies(60 * 1000)); } } if (!acomp->ops) {
On Tue, 06 Oct 2020 13:30:40 +0200, Kai Vehmanen wrote:
Hi,
this simple bugfix started to feel a bit like getting stuck in quicksand, so I'm looking for some early input via this RFC series.
Basicly hdac_i915.c should not use global state to track communication with i915 driver. But how to get handle of "hdac_bus*? I considered a few options:
- add hdac_bus as a member of drm_audio_component.h
-> seems wrong as this is really an audio side implementation)
- embed copy of drm_audio_component to 'struct hdac_bus', so I could use container_of() on the device handle to get to the bus
-> wasted space to keep a copy at hdac_bus level (note: snd-hda-codec-hdmi do this by embedding a copy of ops to "struct hdmi_spec")
- add another devres entry to store the hdac_bus directly in acomp_init and a new helper function to query it
I now implemented option 3 in this RFC series as it seemed cleanest and most local to hdac_component.c, where the problem stems from. It's still somewhat messy, and I'm wondering if I'm overlooking some obvious alternative. We could dig this deeper into i915 specific code, but OTOH, hdac_bus is an argument snd_hdac_acomp_init(), so it's common for all.
Kai Vehmanen (2): ALSA: hda - keep track of HDA core bus instance in acomp ALSA: hda/i915 - fix list corruption with concurrent probes
Another option would be to move the completion into the common acomp helper from i915-specific one. That is,
- Add bind_complete field into struct drm_audio_component, initialize it at snd_hdac_acomp_init()
- Call complete_all(&acomp->bind_complete) at the end of hdac_component_master_bind()
- Remove / replace i915's own completion with the hdac's one. The i915_init_ops can be dropped.
thanks,
Takashi
On Tue, 06 Oct 2020 14:13:40 +0200, Takashi Iwai wrote:
On Tue, 06 Oct 2020 13:30:40 +0200, Kai Vehmanen wrote:
Hi,
this simple bugfix started to feel a bit like getting stuck in quicksand, so I'm looking for some early input via this RFC series.
Basicly hdac_i915.c should not use global state to track communication with i915 driver. But how to get handle of "hdac_bus*? I considered a few options:
- add hdac_bus as a member of drm_audio_component.h
-> seems wrong as this is really an audio side implementation)
- embed copy of drm_audio_component to 'struct hdac_bus', so I could use container_of() on the device handle to get to the bus
-> wasted space to keep a copy at hdac_bus level (note: snd-hda-codec-hdmi do this by embedding a copy of ops to "struct hdmi_spec")
- add another devres entry to store the hdac_bus directly in acomp_init and a new helper function to query it
I now implemented option 3 in this RFC series as it seemed cleanest and most local to hdac_component.c, where the problem stems from. It's still somewhat messy, and I'm wondering if I'm overlooking some obvious alternative. We could dig this deeper into i915 specific code, but OTOH, hdac_bus is an argument snd_hdac_acomp_init(), so it's common for all.
Kai Vehmanen (2): ALSA: hda - keep track of HDA core bus instance in acomp ALSA: hda/i915 - fix list corruption with concurrent probes
Another option would be to move the completion into the common acomp helper from i915-specific one. That is,
Add bind_complete field into struct drm_audio_component, initialize it at snd_hdac_acomp_init()
Call complete_all(&acomp->bind_complete) at the end of hdac_component_master_bind()
Remove / replace i915's own completion with the hdac's one. The i915_init_ops can be dropped.
So something like below (totally untested).
Takashi
--- --- a/include/drm/drm_audio_component.h +++ b/include/drm/drm_audio_component.h @@ -117,6 +117,10 @@ struct drm_audio_component { * @audio_ops: Ops implemented by hda driver, called by DRM driver */ const struct drm_audio_component_audio_ops *audio_ops; + /** + * @master_bind_complete: completion held during component master binding + */ + struct completion master_bind_complete; };
#endif /* _DRM_AUDIO_COMPONENT_H_ */ --- a/sound/hda/hdac_component.c +++ b/sound/hda/hdac_component.c @@ -210,12 +210,14 @@ static int hdac_component_master_bind(struct device *dev) goto module_put; }
+ complete_all(&acomp->master_bind_complete); return 0;
module_put: module_put(acomp->ops->owner); out_unbind: component_unbind_all(dev, acomp); + complete_all(&acomp->master_bind_complete);
return ret; } @@ -296,6 +298,7 @@ int snd_hdac_acomp_init(struct hdac_bus *bus, if (!acomp) return -ENOMEM; acomp->audio_ops = aops; + init_completion(&acomp->master_bind_complete); bus->audio_component = acomp; devres_add(dev, acomp);
--- a/sound/hda/hdac_i915.c +++ b/sound/hda/hdac_i915.c @@ -11,8 +11,6 @@ #include <sound/hda_i915.h> #include <sound/hda_register.h>
-static struct completion bind_complete; - #define IS_HSW_CONTROLLER(pci) (((pci)->device == 0x0a0c) || \ ((pci)->device == 0x0c0c) || \ ((pci)->device == 0x0d0c) || \ @@ -130,19 +128,6 @@ static bool i915_gfx_present(void) return pci_dev_present(ids); }
-static int i915_master_bind(struct device *dev, - struct drm_audio_component *acomp) -{ - complete_all(&bind_complete); - /* clear audio_ops here as it was needed only for completion call */ - acomp->audio_ops = NULL; - return 0; -} - -static const struct drm_audio_component_audio_ops i915_init_ops = { - .master_bind = i915_master_bind -}; - /** * snd_hdac_i915_init - Initialize i915 audio component * @bus: HDA core bus @@ -163,9 +148,7 @@ int snd_hdac_i915_init(struct hdac_bus *bus) if (!i915_gfx_present()) return -ENODEV;
- init_completion(&bind_complete); - - err = snd_hdac_acomp_init(bus, &i915_init_ops, + err = snd_hdac_acomp_init(bus, NULL, i915_component_master_match, sizeof(struct i915_audio_component) - sizeof(*acomp)); if (err < 0) @@ -177,7 +160,7 @@ int snd_hdac_i915_init(struct hdac_bus *bus) if (!IS_ENABLED(CONFIG_MODULES) || !request_module("i915")) { /* 60s timeout */ - wait_for_completion_timeout(&bind_complete, + wait_for_completion_timeout(&acomp->master_bind_complete, msecs_to_jiffies(60 * 1000)); } }
Hi,
On Tue, 6 Oct 2020, Takashi Iwai wrote:
On Tue, 06 Oct 2020 14:13:40 +0200, Takashi Iwai wrote:
Another option would be to move the completion into the common acomp helper from i915-specific one. That is,
[...]
So something like below (totally untested).
thanks Takashi for the quick reply. I did a quick test on a system with two controllers and seems to work as expected.
Getting rid of i915_master_bind is a nice plus definitely for this option. And the change in drm headers seems simple enough.
I'll do a bit more testing and send an updated set based on your approach.
Br, Kai
participants (2)
-
Kai Vehmanen -
Takashi Iwai