Re: [PATCH v3 3/3] ALSA: hda: allow direct complete with a powered-off GPU

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The `azx_prepare` function conditionally returns 1 early to request direct complete, bypassing mandatory ALSA state initialization and workqueue flushing. If direct complete is subsequently aborted by the PM core, the device will suspend in an inconsistent state.

Pre-existing issues:
- [High] The HDA interrupt handler uses a non-atomic runtime PM check, and the runtime suspend callback shuts down the hardware without synchronizing shared IRQs. This allows the interrupt handler to run concurrently while the hardware is being powered down.
--

--- Patch [3]: [PATCH v3 3/3] ALSA: hda: allow direct complete with a powered-off GPU ---
commit 8f27e09b7a8bd094dbb356dbe9893145a18d34a2
Author: Andre Eikmeyer <[email protected]>

ALSA: hda: allow direct complete with a powered-off GPU

The commit allows direct complete during system suspend for a switcheroo
audio client when the bound GPU is already runtime suspended and switched
off. It does this by checking the VGA switcheroo state and returning 1
from azx_prepare().

> diff --git a/sound/hda/controllers/intel.c b/sound/hda/controllers/intel.c
> index 4b03c64e72ab4..6f1c1d596e2ca 100644
> --- a/sound/hda/controllers/intel.c
> +++ b/sound/hda/controllers/intel.c

[ ... ]

> @@ -1025,6 +1027,10 @@ static int azx_prepare(struct device *dev)
>  		return 0;
>  
>  	chip = card->private_data;
> +	/* A positive return allows the PM core to use direct complete. */
> +	if (azx_vga_is_powered_off(chip))
> +		return 1;
> +

[Severity: High]
If the PM core decides to abort direct complete for this device later in the
suspend process, will this leave the device in an inconsistent state?

For example, if a child HDA codec returns 0 from its own prepare callback,
the PM core clears the direct complete flag and will fall back to calling
azx_suspend().

Since this early return bypasses snd_power_change_state() and flush_work(),
the ALSA subsystem will still consider the device to be in SNDRV_CTL_POWER_D0
and the unsol_work may still be queued or running while the hardware gets
shut down.

>  	chip->pm_prepared = 1;
>  	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);

[ ... ]

[Severity: High]
This is a pre-existing issue, but I have a question about the existing
runtime PM handling and shared interrupts in this driver.

Does azx_runtime_suspend() need to call synchronize_irq() before powering
down the hardware?

Currently, azx_interrupt() in sound/hda/common/controller.c uses a
non-atomic pm_runtime_active() check. If a shared interrupt fires for
another device, azx_interrupt() could see the device as active, while
another CPU concurrently executes azx_runtime_suspend() and shuts down the
hardware.

A read from the powered-off hardware would then typically return 0xffffffff.
If this happens between reading INTSTS and RIRBSTS, the 0xff byte read could
incorrectly match RIRB_INT_MASK and trigger further invalid accesses or an
IRQ storm.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3