v5.17.0-rc3: Regression bisecting failing suspend to disk: ALSA: hda: realtek: Fix race at concurrent COEF updates

v5.17.0-rc3: Regression bisecting failing suspend to disk: ALSA: hda: realtek: Fix race at concurrent COEF updates

[Julian Wollrath]

Dear Takashi Iwai,

since v5.17.0-rc3 my system (HP Elitebook 845 G7) fails to suspend to
disk and instead just hangs. Rebooting via the magic sysrq keys is
still possible though. I bisected the regression to commit
b837a9f5ab3bdfab9233c9f98a6bef717673a3e5 (ALSA: hda: realtek: Fix race
at concurrent COEF updates). Reverting that commit removes the problem.
I you need more information, I am happy to provide it.


Best regards,
Julian Wollrath

--
 ()  ascii ribbon campaign - against html e-mail
 /\                        - against proprietary attachments

Re: v5.17.0-rc3: Regression bisecting failing suspend to disk: ALSA: hda: realtek: Fix race at concurrent COEF updates

[Takashi Iwai]

On Mon, 14 Feb 2022 13:28:38 +0100,
Julian Wollrath wrote:
> 
> Dear Takashi Iwai,
> 
> since v5.17.0-rc3 my system (HP Elitebook 845 G7) fails to suspend to
> disk and instead just hangs. Rebooting via the magic sysrq keys is
> still possible though. I bisected the regression to commit
> b837a9f5ab3bdfab9233c9f98a6bef717673a3e5 (ALSA: hda: realtek: Fix race
> at concurrent COEF updates). Reverting that commit removes the problem.
> I you need more information, I am happy to provide it.

Does the patch below help?


thanks,

Takashi

--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -138,6 +138,22 @@ struct alc_spec {
  * COEF access helper functions
  */
 
+static void coef_mutex_lock(struct hda_codec *codec)
+{
+	struct alc_spec *spec = codec->spec;
+
+	snd_hda_power_up_pm(codec);
+	mutex_lock(&spec->coef_mutex);
+}
+
+static void coef_mutex_unlock(struct hda_codec *codec)
+{
+	struct alc_spec *spec = codec->spec;
+
+	mutex_unlock(&spec->coef_mutex);
+	snd_hda_power_down_pm(codec);
+}
+
 static int __alc_read_coefex_idx(struct hda_codec *codec, hda_nid_t nid,
 				 unsigned int coef_idx)
 {
@@ -151,12 +167,11 @@ static int __alc_read_coefex_idx(struct hda_codec *codec, hda_nid_t nid,
 static int alc_read_coefex_idx(struct hda_codec *codec, hda_nid_t nid,
 			       unsigned int coef_idx)
 {
-	struct alc_spec *spec = codec->spec;
 	unsigned int val;
 
-	mutex_lock(&spec->coef_mutex);
+	coef_mutex_lock(codec);
 	val = __alc_read_coefex_idx(codec, nid, coef_idx);
-	mutex_unlock(&spec->coef_mutex);
+	coef_mutex_unlock(codec);
 	return val;
 }
 
@@ -173,11 +188,9 @@ static void __alc_write_coefex_idx(struct hda_codec *codec, hda_nid_t nid,
 static void alc_write_coefex_idx(struct hda_codec *codec, hda_nid_t nid,
 				 unsigned int coef_idx, unsigned int coef_val)
 {
-	struct alc_spec *spec = codec->spec;
-
-	mutex_lock(&spec->coef_mutex);
+	coef_mutex_lock(codec);
 	__alc_write_coefex_idx(codec, nid, coef_idx, coef_val);
-	mutex_unlock(&spec->coef_mutex);
+	coef_mutex_unlock(codec);
 }
 
 #define alc_write_coef_idx(codec, coef_idx, coef_val) \
@@ -198,11 +211,9 @@ static void alc_update_coefex_idx(struct hda_codec *codec, hda_nid_t nid,
 				  unsigned int coef_idx, unsigned int mask,
 				  unsigned int bits_set)
 {
-	struct alc_spec *spec = codec->spec;
-
-	mutex_lock(&spec->coef_mutex);
+	coef_mutex_lock(codec);
 	__alc_update_coefex_idx(codec, nid, coef_idx, mask, bits_set);
-	mutex_unlock(&spec->coef_mutex);
+	coef_mutex_unlock(codec);
 }
 
 #define alc_update_coef_idx(codec, coef_idx, mask, bits_set)	\
@@ -235,9 +246,7 @@ struct coef_fw {
 static void alc_process_coef_fw(struct hda_codec *codec,
 				const struct coef_fw *fw)
 {
-	struct alc_spec *spec = codec->spec;
-
-	mutex_lock(&spec->coef_mutex);
+	coef_mutex_lock(codec);
 	for (; fw->nid; fw++) {
 		if (fw->mask == (unsigned short)-1)
 			__alc_write_coefex_idx(codec, fw->nid, fw->idx, fw->val);
@@ -245,7 +254,7 @@ static void alc_process_coef_fw(struct hda_codec *codec,
 			__alc_update_coefex_idx(codec, fw->nid, fw->idx,
 						fw->mask, fw->val);
 	}
-	mutex_unlock(&spec->coef_mutex);
+	coef_mutex_unlock(codec);
 }
 
 /*

Re: v5.17.0-rc3: Regression bisecting failing suspend to disk: ALSA: hda: realtek: Fix race at concurrent COEF updates

[Julian Wollrath]

Dear Takashi,

> Does the patch below help?

yes, that patch does the trick and removes the problem.


Thank you!

Julian

>
>
> thanks,
>
> Takashi
>
> --- a/sound/pci/hda/patch_realtek.c
> +++ b/sound/pci/hda/patch_realtek.c
> @@ -138,6 +138,22 @@ struct alc_spec {
>   * COEF access helper functions
>   */
>
> +static void coef_mutex_lock(struct hda_codec *codec)
> +{
> +	struct alc_spec *spec = codec->spec;
> +
> +	snd_hda_power_up_pm(codec);
> +	mutex_lock(&spec->coef_mutex);
> +}
> +
> +static void coef_mutex_unlock(struct hda_codec *codec)
> +{
> +	struct alc_spec *spec = codec->spec;
> +
> +	mutex_unlock(&spec->coef_mutex);
> +	snd_hda_power_down_pm(codec);
> +}
> +
>  static int __alc_read_coefex_idx(struct hda_codec *codec, hda_nid_t
> nid, unsigned int coef_idx)
>  {
> @@ -151,12 +167,11 @@ static int __alc_read_coefex_idx(struct
> hda_codec *codec, hda_nid_t nid, static int
> alc_read_coefex_idx(struct hda_codec *codec, hda_nid_t nid, unsigned
> int coef_idx) {
> -	struct alc_spec *spec = codec->spec;
>  	unsigned int val;
>
> -	mutex_lock(&spec->coef_mutex);
> +	coef_mutex_lock(codec);
>  	val = __alc_read_coefex_idx(codec, nid, coef_idx);
> -	mutex_unlock(&spec->coef_mutex);
> +	coef_mutex_unlock(codec);
>  	return val;
>  }
>
> @@ -173,11 +188,9 @@ static void __alc_write_coefex_idx(struct
> hda_codec *codec, hda_nid_t nid, static void
> alc_write_coefex_idx(struct hda_codec *codec, hda_nid_t nid, unsigned
> int coef_idx, unsigned int coef_val) {
> -	struct alc_spec *spec = codec->spec;
> -
> -	mutex_lock(&spec->coef_mutex);
> +	coef_mutex_lock(codec);
>  	__alc_write_coefex_idx(codec, nid, coef_idx, coef_val);
> -	mutex_unlock(&spec->coef_mutex);
> +	coef_mutex_unlock(codec);
>  }
>
>  #define alc_write_coef_idx(codec, coef_idx, coef_val) \
> @@ -198,11 +211,9 @@ static void alc_update_coefex_idx(struct
> hda_codec *codec, hda_nid_t nid, unsigned int coef_idx, unsigned int
> mask, unsigned int bits_set)
>  {
> -	struct alc_spec *spec = codec->spec;
> -
> -	mutex_lock(&spec->coef_mutex);
> +	coef_mutex_lock(codec);
>  	__alc_update_coefex_idx(codec, nid, coef_idx, mask,
> bits_set);
> -	mutex_unlock(&spec->coef_mutex);
> +	coef_mutex_unlock(codec);
>  }
>
>  #define alc_update_coef_idx(codec, coef_idx, mask, bits_set)	\
> @@ -235,9 +246,7 @@ struct coef_fw {
>  static void alc_process_coef_fw(struct hda_codec *codec,
>  				const struct coef_fw *fw)
>  {
> -	struct alc_spec *spec = codec->spec;
> -
> -	mutex_lock(&spec->coef_mutex);
> +	coef_mutex_lock(codec);
>  	for (; fw->nid; fw++) {
>  		if (fw->mask == (unsigned short)-1)
>  			__alc_write_coefex_idx(codec, fw->nid,
> fw->idx, fw->val); @@ -245,7 +254,7 @@ static void
> alc_process_coef_fw(struct hda_codec *codec,
> __alc_update_coefex_idx(codec, fw->nid, fw->idx, fw->mask, fw->val);
>  	}
> -	mutex_unlock(&spec->coef_mutex);
> +	coef_mutex_unlock(codec);
>  }
>
>  /*



--
 ()  ascii ribbon campaign - against html e-mail
 /\                        - against proprietary attachments

Re: v5.17.0-rc3: Regression bisecting failing suspend to disk: ALSA: hda: realtek: Fix race at concurrent COEF updates

[Takashi Iwai]

On Mon, 14 Feb 2022 13:54:35 +0100,
Julian Wollrath wrote:
> 
> Dear Takashi,
> 
> > Does the patch below help?
> 
> yes, that patch does the trick and removes the problem.

Great, I'll submit the proper patch and merge for the next PR.


thanks,

Takashi