alsa-devel.alsa-project.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ALSA: hda/i915 - fix list corruption with concurrent probes
@ 2020-10-06 16:17 Kai Vehmanen
  2020-10-09 14:46 ` Takashi Iwai
  0 siblings, 1 reply; 2+ messages in thread
From: Kai Vehmanen @ 2020-10-06 16:17 UTC (permalink / raw)
  To: tiwai, alsa-devel, maarten.lankhorst, mripard, tzimmermann,
	airlied, dri-devel, intel-gfx, jani.nikula
  Cc: Kai Vehmanen

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>
---
 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


^ permalink raw reply related	[flat|nested] 2+ messages in thread
* Re: [PATCH] ALSA: hda/i915 - fix list corruption with concurrent probes
  2020-10-06 16:17 [PATCH] ALSA: hda/i915 - fix list corruption with concurrent probes Kai Vehmanen
@ 2020-10-09 14:46 ` Takashi Iwai
  0 siblings, 0 replies; 2+ messages in thread
From: Takashi Iwai @ 2020-10-09 14:46 UTC (permalink / raw)
  To: Kai Vehmanen
  Cc: alsa-devel, airlied, intel-gfx, maarten.lankhorst, mripard,
	jani.nikula, dri-devel, tzimmermann

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
> 

^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-10-09 14:47 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-06 16:17 [PATCH] ALSA: hda/i915 - fix list corruption with concurrent probes Kai Vehmanen
2020-10-09 14:46 ` Takashi Iwai

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).