Re: [PATCH v3] ASoC: dt-bindings: sound: atmel_ac97c: Convert to DT schema

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver fails to properly handle -EPROBE_DEFER when requesting the reset GPIO, incorrectly bypassing hardware reset and permanently falling back to software reset.
- [High] Incorrect initialization and teardown ordering of hardware resources (IRQs, memory maps, clocks) leading to potential kernel panics due to NULL pointer dereferences or unmapped memory access.
--

commit dce8eddfc311c261daebe971da37753c69680689
Author: Manish Baing <manishbaing2789@gmail.com>

ASoC: dt-bindings: sound: atmel_ac97c: Convert to DT schema
    
Convert the Atmel AC97 controller binding from text format to YAML schema.
The binding constraints were updated during conversion to align with actual
driver behavior.

>     - Added previously undocumented 'clocks' and 'clock-names' properties.
>       These are made required, as the driver unconditionally requests them.

[Severity: High]
This isn't a bug introduced by this patch, but since we are aligning with driver
behavior regarding clocks, is the initialization and teardown ordering in
sound/atmel/ac97c.c safe?

In atmel_ac97c_probe(), request_irq() is called before ioremap():

	retval = request_irq(irq, atmel_ac97c_interrupt, 0, "AC97C", chip);
	...
	chip->regs = ioremap(regs->start, resource_size(regs));

If an interrupt fires immediately, will the handler atmel_ac97c_interrupt()
dereference chip->regs via ac97c_readl() while it is still NULL?

Additionally, in the probe error path and atmel_ac97c_remove(), iounmap() and
clk_disable_unprepare() are called before free_irq():

	clk_disable_unprepare(chip->pclk);
	clk_put(chip->pclk);
	iounmap(chip->regs);
	free_irq(chip->irq, chip);

	snd_card_free(card);

Could an interrupt occurring in this window cause the handler to attempt access
to unmapped I/O memory or registers with a disabled clock?

>     - Removed 'ac97-gpios' from the required list, as the driver
>       treats the reset pin as optional, and requiring it breaks existing
>       .dtsi validation.

[Severity: High]
This is a pre-existing issue, but does the driver properly handle probe
deferral when treating the reset pin as optional?

In sound/atmel/ac97c.c:atmel_ac97c_probe(), when requesting the GPIO:

	chip->reset_pin = devm_gpiod_get_index(dev, "ac97", 2, GPIOD_OUT_HIGH);
	if (IS_ERR(chip->reset_pin))
		dev_dbg(dev, "reset pin not available\n");

	atmel_ac97c_reset(chip);

If devm_gpiod_get_index() returns -EPROBE_DEFER, does this check swallow the
deferral request and permanently fall back to a software register reset in
atmel_ac97c_reset() instead of waiting for the hardware reset pin?

	} else {
		ac97c_writel(chip, MR, AC97C_MR_WRST | AC97C_MR_ENA);
		udelay(2);
		ac97c_writel(chip, MR, AC97C_MR_ENA);
	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260530052812.115994-1-manishbaing2789@gmail.com?part=1