hda codec unbind refcount hang

hda codec unbind refcount hang

[Ville Syrjälä]

Hi Takashi,

commit 7206998f578d ("ALSA: hda: Fix potential deadlock at codec
unbinding") introduced a problem on at least one of my older machines.

The problem happens when hda_codec_driver_remove() encounters a
codec without any pcms (and thus the refcount is 1) and tries to
call refcount_dec(). Turns out refcount_dec() doesn't like to be
used for dropping the refcount to 0, and instead if spews a warning
and does its saturate thing. The subsequent wait_event() is then
permanently stuck waiting on the saturated refcount.

I've definitely seen the same kind of pattern used elsewhere
in the kernel as well, so the fact that refcount_t can't be used
to implement it is a bit of surprise to me. I guess most other
places still use atomic_t instead.

-- 
Ville Syrjälä
Intel

Re: hda codec unbind refcount hang

[Takashi Iwai]

On Fri, 09 Sep 2022 17:45:25 +0200,
Ville Syrjälä wrote:
> 
> Hi Takashi,
> 
> commit 7206998f578d ("ALSA: hda: Fix potential deadlock at codec
> unbinding") introduced a problem on at least one of my older machines.
> 
> The problem happens when hda_codec_driver_remove() encounters a
> codec without any pcms (and thus the refcount is 1) and tries to
> call refcount_dec(). Turns out refcount_dec() doesn't like to be
> used for dropping the refcount to 0, and instead if spews a warning
> and does its saturate thing. The subsequent wait_event() is then
> permanently stuck waiting on the saturated refcount.
> 
> I've definitely seen the same kind of pattern used elsewhere
> in the kernel as well, so the fact that refcount_t can't be used
> to implement it is a bit of surprise to me. I guess most other
> places still use atomic_t instead.

Does the patch below work around it?  It seem to be a subtle
difference between refcount_dec() and refcount_dec_and_test().


thanks,

Takashi

-- 8< --
--- a/sound/pci/hda/hda_bind.c
+++ b/sound/pci/hda/hda_bind.c
@@ -157,10 +157,11 @@ static int hda_codec_driver_remove(struct device *dev)
 		return codec->bus->core.ext_ops->hdev_detach(&codec->core);
 	}
 
-	refcount_dec(&codec->pcm_ref);
-	snd_hda_codec_disconnect_pcms(codec);
-	snd_hda_jack_tbl_disconnect(codec);
-	wait_event(codec->remove_sleep, !refcount_read(&codec->pcm_ref));
+	if (!refcount_dec_and_test(&codec->pcm_ref)) {
+		snd_hda_codec_disconnect_pcms(codec);
+		snd_hda_jack_tbl_disconnect(codec);
+		wait_event(codec->remove_sleep, !refcount_read(&codec->pcm_ref));
+	}
 	snd_power_sync_ref(codec->bus->card);
 
 	if (codec->patch_ops.free)

Re: hda codec unbind refcount hang

[Ville Syrjälä]

On Fri, Sep 09, 2022 at 05:59:47PM +0200, Takashi Iwai wrote:
> On Fri, 09 Sep 2022 17:45:25 +0200,
> Ville Syrjälä wrote:
> > 
> > Hi Takashi,
> > 
> > commit 7206998f578d ("ALSA: hda: Fix potential deadlock at codec
> > unbinding") introduced a problem on at least one of my older machines.
> > 
> > The problem happens when hda_codec_driver_remove() encounters a
> > codec without any pcms (and thus the refcount is 1) and tries to
> > call refcount_dec(). Turns out refcount_dec() doesn't like to be
> > used for dropping the refcount to 0, and instead if spews a warning
> > and does its saturate thing. The subsequent wait_event() is then
> > permanently stuck waiting on the saturated refcount.
> > 
> > I've definitely seen the same kind of pattern used elsewhere
> > in the kernel as well, so the fact that refcount_t can't be used
> > to implement it is a bit of surprise to me. I guess most other
> > places still use atomic_t instead.
> 
> Does the patch below work around it?  It seem to be a subtle
> difference between refcount_dec() and refcount_dec_and_test().

Aye, this works.

Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> 
> thanks,
> 
> Takashi
> 
> -- 8< --
> --- a/sound/pci/hda/hda_bind.c
> +++ b/sound/pci/hda/hda_bind.c
> @@ -157,10 +157,11 @@ static int hda_codec_driver_remove(struct device *dev)
>  		return codec->bus->core.ext_ops->hdev_detach(&codec->core);
>  	}
>  
> -	refcount_dec(&codec->pcm_ref);
> -	snd_hda_codec_disconnect_pcms(codec);
> -	snd_hda_jack_tbl_disconnect(codec);
> -	wait_event(codec->remove_sleep, !refcount_read(&codec->pcm_ref));
> +	if (!refcount_dec_and_test(&codec->pcm_ref)) {
> +		snd_hda_codec_disconnect_pcms(codec);
> +		snd_hda_jack_tbl_disconnect(codec);
> +		wait_event(codec->remove_sleep, !refcount_read(&codec->pcm_ref));
> +	}
>  	snd_power_sync_ref(codec->bus->card);
>  
>  	if (codec->patch_ops.free)

-- 
Ville Syrjälä
Intel

Re: hda codec unbind refcount hang

[Takashi Iwai]

On Fri, 09 Sep 2022 21:39:19 +0200,
Ville Syrjälä wrote:
> 
> On Fri, Sep 09, 2022 at 05:59:47PM +0200, Takashi Iwai wrote:
> > On Fri, 09 Sep 2022 17:45:25 +0200,
> > Ville Syrjälä wrote:
> > > 
> > > Hi Takashi,
> > > 
> > > commit 7206998f578d ("ALSA: hda: Fix potential deadlock at codec
> > > unbinding") introduced a problem on at least one of my older machines.
> > > 
> > > The problem happens when hda_codec_driver_remove() encounters a
> > > codec without any pcms (and thus the refcount is 1) and tries to
> > > call refcount_dec(). Turns out refcount_dec() doesn't like to be
> > > used for dropping the refcount to 0, and instead if spews a warning
> > > and does its saturate thing. The subsequent wait_event() is then
> > > permanently stuck waiting on the saturated refcount.
> > > 
> > > I've definitely seen the same kind of pattern used elsewhere
> > > in the kernel as well, so the fact that refcount_t can't be used
> > > to implement it is a bit of surprise to me. I guess most other
> > > places still use atomic_t instead.
> > 
> > Does the patch below work around it?  It seem to be a subtle
> > difference between refcount_dec() and refcount_dec_and_test().
> 
> Aye, this works.
> 
> Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Good to hear.

I think the below is slightly safer, assuring the other *_disconnect()
calls.

Could you give it a try again?  Once after confirming it works, I'll
re-submit and merge to my tree.


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: hda: Fix hang at HD-audio codec unbinding due to refcount saturation
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

We fixed the potential deadlock at dynamic unbinding the HD-audio
codec at the commit 7206998f578d ("ALSA: hda: Fix potential deadlock
at codec unbinding"), but ironically, this caused another potential
deadlock.  The current code uses refcount_dec() and waits for the
pending task with wait_event for dropping the refcount to 0.  This
works fine when PCMs are assigned and actually waiting for the
refcount drop.

Meanwhile, when there was no PCM assigned, the refcount_dec() call
itself was supposed to drop to zero -- alas, it doesn't in reality;
refcount_dec() complains, spews kernel warning and it saturates
instead of dropping to 0, due to the nature of refcount_dec()
implementation.  This eventually blocks the wait_event() wakeup and
the code get stuck there.

For avoiding the problem, we call refcount_dec_and_test() and skips
the sync-wait if it already reaches to zero.

The patch does a slight code reshuffling to make sure to invoke other
disconnect calls before the sync-wait, too.

Fixes: 7206998f578d ("ALSA: hda: Fix potential deadlock at codec unbinding")
Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: <stable@vger.kernel.org>
Link: https://lore.kernel.org/r/YxtflWQnslMHVlU7@intel.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_bind.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/pci/hda/hda_bind.c b/sound/pci/hda/hda_bind.c
index cae9a975cbcc..1a868dd9dc4b 100644
--- a/sound/pci/hda/hda_bind.c
+++ b/sound/pci/hda/hda_bind.c
@@ -157,10 +157,10 @@ static int hda_codec_driver_remove(struct device *dev)
 		return codec->bus->core.ext_ops->hdev_detach(&codec->core);
 	}
 
-	refcount_dec(&codec->pcm_ref);
 	snd_hda_codec_disconnect_pcms(codec);
 	snd_hda_jack_tbl_disconnect(codec);
-	wait_event(codec->remove_sleep, !refcount_read(&codec->pcm_ref));
+	if (!refcount_dec_and_test(&codec->pcm_ref))
+		wait_event(codec->remove_sleep, !refcount_read(&codec->pcm_ref));
 	snd_power_sync_ref(codec->bus->card);
 
 	if (codec->patch_ops.free)
-- 
2.35.3

Re: hda codec unbind refcount hang

[Ville Syrjälä]

On Sat, Sep 10, 2022 at 12:22:04PM +0200, Takashi Iwai wrote:
> On Fri, 09 Sep 2022 21:39:19 +0200,
> Ville Syrjälä wrote:
> > 
> > On Fri, Sep 09, 2022 at 05:59:47PM +0200, Takashi Iwai wrote:
> > > On Fri, 09 Sep 2022 17:45:25 +0200,
> > > Ville Syrjälä wrote:
> > > > 
> > > > Hi Takashi,
> > > > 
> > > > commit 7206998f578d ("ALSA: hda: Fix potential deadlock at codec
> > > > unbinding") introduced a problem on at least one of my older machines.
> > > > 
> > > > The problem happens when hda_codec_driver_remove() encounters a
> > > > codec without any pcms (and thus the refcount is 1) and tries to
> > > > call refcount_dec(). Turns out refcount_dec() doesn't like to be
> > > > used for dropping the refcount to 0, and instead if spews a warning
> > > > and does its saturate thing. The subsequent wait_event() is then
> > > > permanently stuck waiting on the saturated refcount.
> > > > 
> > > > I've definitely seen the same kind of pattern used elsewhere
> > > > in the kernel as well, so the fact that refcount_t can't be used
> > > > to implement it is a bit of surprise to me. I guess most other
> > > > places still use atomic_t instead.
> > > 
> > > Does the patch below work around it?  It seem to be a subtle
> > > difference between refcount_dec() and refcount_dec_and_test().
> > 
> > Aye, this works.
> > 
> > Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Good to hear.
> 
> I think the below is slightly safer, assuring the other *_disconnect()
> calls.
> 
> Could you give it a try again?  Once after confirming it works, I'll
> re-submit and merge to my tree.

This works too. Thanks

Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> 
> thanks,
> 
> Takashi
> 
> -- 8< --
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] ALSA: hda: Fix hang at HD-audio codec unbinding due to refcount saturation
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> We fixed the potential deadlock at dynamic unbinding the HD-audio
> codec at the commit 7206998f578d ("ALSA: hda: Fix potential deadlock
> at codec unbinding"), but ironically, this caused another potential
> deadlock.  The current code uses refcount_dec() and waits for the
> pending task with wait_event for dropping the refcount to 0.  This
> works fine when PCMs are assigned and actually waiting for the
> refcount drop.
> 
> Meanwhile, when there was no PCM assigned, the refcount_dec() call
> itself was supposed to drop to zero -- alas, it doesn't in reality;
> refcount_dec() complains, spews kernel warning and it saturates
> instead of dropping to 0, due to the nature of refcount_dec()
> implementation.  This eventually blocks the wait_event() wakeup and
> the code get stuck there.
> 
> For avoiding the problem, we call refcount_dec_and_test() and skips
> the sync-wait if it already reaches to zero.
> 
> The patch does a slight code reshuffling to make sure to invoke other
> disconnect calls before the sync-wait, too.
> 
> Fixes: 7206998f578d ("ALSA: hda: Fix potential deadlock at codec unbinding")
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: <stable@vger.kernel.org>
> Link: https://lore.kernel.org/r/YxtflWQnslMHVlU7@intel.com
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/pci/hda/hda_bind.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_bind.c b/sound/pci/hda/hda_bind.c
> index cae9a975cbcc..1a868dd9dc4b 100644
> --- a/sound/pci/hda/hda_bind.c
> +++ b/sound/pci/hda/hda_bind.c
> @@ -157,10 +157,10 @@ static int hda_codec_driver_remove(struct device *dev)
>  		return codec->bus->core.ext_ops->hdev_detach(&codec->core);
>  	}
>  
> -	refcount_dec(&codec->pcm_ref);
>  	snd_hda_codec_disconnect_pcms(codec);
>  	snd_hda_jack_tbl_disconnect(codec);
> -	wait_event(codec->remove_sleep, !refcount_read(&codec->pcm_ref));
> +	if (!refcount_dec_and_test(&codec->pcm_ref))
> +		wait_event(codec->remove_sleep, !refcount_read(&codec->pcm_ref));
>  	snd_power_sync_ref(codec->bus->card);
>  
>  	if (codec->patch_ops.free)
> -- 
> 2.35.3

-- 
Ville Syrjälä
Intel