Re: [PATCH v2] ALSA: hda: Reset response counter when a verb times out

[Takashi Iwai]

On Sat, 10 Oct 2026 04:29:32 +0200,
songxiebing wrote:
> 
> From: Bob Song <songxiebing@xxxxxxxxxx>
> 
> On some controllers the HDA link reports a verb timeout and the driver
> falls back to polling mode.  After that, every subsequent verb sent to
> that codec address keeps timing out (roughly one second per verb), while
> the hardware itself looks perfectly healthy: the RIRB write pointer keeps
> advancing, interrupts are still delivered, and verbs that carry no reply
> payload -- notably writes -- still take effect on the codec.
> 
> The reason is that bus->rirb.cmds[addr] is incremented for every verb and
> only decremented in snd_hdac_bus_update_rirb() when a matching RIRB
> response is read, without any rollback for a response that never arrives.
> A controller that cannot fall back to the single-command mode
> (chip->fallback_to_single_cmd == 0, e.g. the ACPI platform, Tegra and CIX
> controllers) returns an error right away and never performs the bus reset
> that would reinitialize CORB/RIRB and clear the counter, so a single lost
> response leaves bus->rirb.cmds[addr] non-zero forever.  Each following
> verb then still gets one response that only brings the count back to the
> stale baseline of 1, and snd_hdac_bus_get_response() keeps timing out even
> though the codec answers normally.
> 
> Handle it in the core helper snd_hdac_bus_exec_verb_unlocked(): whenever
> get_response() reports an error, reset the pending counter for the codec
> address, so a late response is simply treated as a spurious response and
> the following verbs recover.  Doing it here covers all HD-audio
> controllers instead of relying on each controller's fallback setting.
> 
> Add a small helper snd_hdac_bus_reset_response_counter() for this.
> 
> Signed-off-by: Bob Song <songxiebing@xxxxxxxxxx>
> ---
> Changes in v2:
> - Move the fix from azx_rirb_get_response() in the HDA controller helper
>   into the HD-audio core (snd_hdac_bus_exec_verb_unlocked()), as suggested
>   by Takashi Iwai, so that it covers all HD-audio controllers and not only
>   the ones using the shared azx helper.
> - Add the snd_hdac_bus_reset_response_counter() helper instead of
>   open-coding the counter reset.
> 
>  include/sound/hdaudio.h     |  2 ++
>  sound/hda/core/bus.c        |  6 +++++-
>  sound/hda/core/controller.c | 20 ++++++++++++++++++++
>  3 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> index aa994d6e6d35..b0fbb61fa093 100644
> --- a/include/sound/hdaudio.h
> +++ b/include/sound/hdaudio.h
> @@ -398,6 +398,8 @@ void snd_hdac_codec_link_down(struct hdac_device *codec);
>  int snd_hdac_bus_send_cmd(struct hdac_bus *bus, unsigned int val);
>  int snd_hdac_bus_get_response(struct hdac_bus *bus, unsigned int addr,
>  			      unsigned int *res);
> +void snd_hdac_bus_reset_response_counter(struct hdac_bus *bus,
> +					 unsigned int addr);
>  int snd_hdac_bus_parse_capabilities(struct hdac_bus *bus);
>  
>  bool snd_hdac_bus_init_chip(struct hdac_bus *bus, bool full_reset);
> diff --git a/sound/hda/core/bus.c b/sound/hda/core/bus.c
> index 81498f1e413e..ecc4b7a780a4 100644
> --- a/sound/hda/core/bus.c
> +++ b/sound/hda/core/bus.c
> @@ -121,11 +121,15 @@ int snd_hdac_bus_exec_verb_unlocked(struct hdac_bus *bus, unsigned int addr,
>  			break;
>  		/* process pending verbs */
>  		err = bus->ops->get_response(bus, addr, &tmp);
> -		if (err)
> +		if (err) {
> +			snd_hdac_bus_reset_response_counter(bus, addr);
>  			break;
> +		}
>  	}
>  	if (!err && res) {
>  		err = bus->ops->get_response(bus, addr, res);
> +		if (err)
> +			snd_hdac_bus_reset_response_counter(bus, addr);
>  		trace_hda_get_response(bus, addr, *res);
>  	}
>  	return err;
> diff --git a/sound/hda/core/controller.c b/sound/hda/core/controller.c
> index 78855ac357c6..1babc5109049 100644
> --- a/sound/hda/core/controller.c
> +++ b/sound/hda/core/controller.c
> @@ -396,6 +396,26 @@ int snd_hdac_bus_get_response(struct hdac_bus *bus, unsigned int addr,
>  }
>  EXPORT_SYMBOL_GPL(snd_hdac_bus_get_response);
>  
> +/**
> + * snd_hdac_bus_reset_response_counter - reset the pending response count
> + * @bus: HD-audio core bus
> + * @addr: codec address
> + *
> + * Drop the number of commands that are still waiting for a response from
> + * the given codec address.  This is called when a verb is known to be dead,
> + * e.g. after a response timeout, so that a lost response won't leave the
> + * counter non-zero forever and block all later verbs to the same codec.
> + */
> +void snd_hdac_bus_reset_response_counter(struct hdac_bus *bus, unsigned int addr)
> +{
> +	if (addr >= HDA_MAX_CODECS)
> +		return;
> +
> +	guard(spinlock_irq)(&bus->reg_lock);
> +	bus->rirb.cmds[addr] = 0;
> +}
> +EXPORT_SYMBOL_GPL(snd_hdac_bus_reset_response_counter);

Do we need to expose a symbol?  It's used in hda-core, and both bus.c
and controller.c are compiled together into it.
Let's avoid unneeded exports until really needed.


thanks,

Takashi