Re: [PATCH] ALSA: hda/i915 - fix list corruption with concurrent probes

[Takashi Iwai <tiwai@suse.de>]

On Tue, 06 Oct 2020 18:17:22 +0200,
Kai Vehmanen wrote:
> 
> From: Takashi Iwai <tiwai@suse.de>
> 
> 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@linux.intel.com>
> Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

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
>