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

DISCUSSION Julian Wollrath 2 participants 4 messages Feb 14, 2022 Sound

Share & tools

Julian Wollrath

author

2022-02-14 12:33 UTC #0 lore raw

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

Takashi Iwai

2022-02-14 12:46 UTC #1 lore raw

Quoted 9 lines · 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.

Does the patch below help?

thanks,

Takashi

sound/pci/hda/patch_realtek.c +24 ~~-15~~

@@ -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);
 }
 
 /*

Julian Wollrath

author

2022-02-14 12:54 UTC #2 lore raw

Dear Takashi,

Quoted 1 line

Does the patch below help?

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

Thank you!

Julian

Quoted 89 lines

thanks,

Takashi

sound/pci/hda/patch_realtek.c +16 -0

@@ -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) {

sound/pci/hda/patch_realtek.c +2 -3

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

sound/pci/hda/patch_realtek.c +2 -4

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

sound/pci/hda/patch_realtek.c +1 -3

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

sound/pci/hda/patch_realtek.c +2 -4

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

sound/pci/hda/patch_realtek.c static void alc_process_coef_fw(struct hda_codec *codec, · line 260

Quoted 6 lines

sound/pci/hda/patch_realtek.c +1 -1

 	}
-	mutex_unlock(&spec->coef_mutex);
+	coef_mutex_unlock(codec);
 }
 
 /*

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

Takashi Iwai

2022-02-14 13:04 UTC #3 lore raw

Quoted 6 lines · Julian Wollrath

Dear Takashi,
Quoted 1 line
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