On Tue, 06 Oct 2020 18:17:22 +0200,
Kai Vehmanen wrote:
>
> From: Takashi Iwai <tiwai@xxxxxxx>
>
> Current hdac_i915 uses a static completion instance to wait
> for i915 driver to complete the component bind.
>
> This design is not safe if multiple HDA controllers are active and
> communicating with different i915 instances, and can lead to list
> corruption and failed audio driver probe.
>
> Fix the design by moving completion mechanism to common acomp
> code and remove the related code from hdac_i915.
>
> Co-developed-by: Kai Vehmanen <kai.vehmanen@xxxxxxxxxxxxxxx>
> Signed-off-by: Kai Vehmanen <kai.vehmanen@xxxxxxxxxxxxxxx>
> Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
Now I applied it with Fixes tag to 7b882fe3e3e8 ("ALSA: hda - handle
multiple i915 device instances").
thanks,
Takashi
> ---
> include/drm/drm_audio_component.h | 4 ++++
> sound/hda/hdac_component.c | 3 +++
> sound/hda/hdac_i915.c | 23 +++--------------------
> 3 files changed, 10 insertions(+), 20 deletions(-)
>
> diff --git a/include/drm/drm_audio_component.h b/include/drm/drm_audio_component.h
> index a45f93487039..0d36bfd1a4cd 100644
> --- 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_ */
> diff --git a/sound/hda/hdac_component.c b/sound/hda/hdac_component.c
> index 89126c6fd216..bb37e7e0bd79 100644
> --- 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);
>
> diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
> index 5f0a1aa6ad84..454474ac5716 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) || \
> @@ -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,8 +160,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(&acomp->master_bind_complete,
> + msecs_to_jiffies(60 * 1000));
> }
> }
> if (!acomp->ops) {
> --
> 2.28.0
>
