* Re: [PATCH v2] ASoC: sunxi: sun50i-dmic: Reorder clock enable sequence
2026-06-02 11:31 [PATCH v2] ASoC: sunxi: sun50i-dmic: Reorder clock enable sequence phucduc.bui
@ 2026-06-02 11:42 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-06-02 11:42 UTC (permalink / raw)
To: phucduc.bui; +Cc: linux-sunxi
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [High] ALSA kcontrols can be accessed while the device is runtime suspended, leading to an external abort or system hang.
- [High] The driver manually disables runtime PM and suspends the hardware in remove() before devm unwinding unregisters the ASoC component, leading to potential MMIO accesses to unclocked registers and a kernel panic.
- [High] Missing reset_control_assert() on probe error paths and in driver removal.
--
commit da5361e2c9ff102ecac5566a7b33923066b10fbd
Author: bui duc phuc <phucduc.bui@gmail.com>
ASoC: sunxi: sun50i-dmic: Reorder clock enable sequence
Reorder the runtime resume clock enable sequence to match the hardware
dependency and ensure symmetry with the suspend path.
The bus clock drives the register interface and must be enabled before
the DMIC module clock. This aligns the resume sequence to be the
exact reverse of the suspend sequence.
> diff --git a/sound/soc/sunxi/sun50i-dmic.c b/sound/soc/sunxi/sun50i-dmic.c
> index eddfebe166169..7aff1afdf265f 100644
> --- a/sound/soc/sunxi/sun50i-dmic.c
> +++ b/sound/soc/sunxi/sun50i-dmic.c
> @@ -323,16 +323,16 @@ static int sun50i_dmic_runtime_suspend(struct device *dev)
>
> static int sun50i_dmic_runtime_resume(struct device *dev)
> {
> - struct sun50i_dmic_dev *host = dev_get_drvdata(dev);
> + struct sun50i_dmic_dev *host = dev_get_drvdata(dev);
> int ret;
>
> - ret = clk_prepare_enable(host->dmic_clk);
> + ret = clk_prepare_enable(host->bus_clk);
> if (ret)
> return ret;
>
> - ret = clk_prepare_enable(host->bus_clk);
> + ret = clk_prepare_enable(host->dmic_clk);
> if (ret) {
> - clk_disable_unprepare(host->dmic_clk);
> + clk_disable_unprepare(host->bus_clk);
> return ret;
> }
>
[Severity: High]
This is a pre-existing issue, but can the ALSA volume controls be accessed
while the device is runtime suspended?
The regmap configuration does not enable runtime PM:
static const struct regmap_config sun50i_dmic_regmap_config = {
.reg_bits = 32,
.reg_stride = 4,
.val_bits = 32,
.max_register = SUN50I_DMIC_VERSION,
.cache_type = REGCACHE_NONE,
};
If a user application manipulates the exposed kcontrols in
sun50i_dmic_controls() while the audio stream is inactive, the device will be
in the RPM_SUSPENDED state with the bus clock disabled.
Since regmap will attempt an MMIO access on an unclocked peripheral without
waking it up, does this cause a synchronous external abort?
[Severity: High]
This is a pre-existing issue, but does sun50i_dmic_remove() create a race
condition with the devres teardown?
In sun50i_dmic_probe(), the ASoC component is registered using the devm API:
ret = devm_snd_soc_register_component(&pdev->dev, &sun50i_dmic_component,
&sun50i_dmic_dai, 1);
However, sun50i_dmic_remove() manually suspends the hardware and turns off
the clocks before the component is unregistered:
static void sun50i_dmic_remove(struct platform_device *pdev)
{
pm_runtime_disable(&pdev->dev);
if (!pm_runtime_status_suspended(&pdev->dev))
sun50i_dmic_runtime_suspend(&pdev->dev);
}
If the driver is unbound while an audio stream is actively running, the
clocks will be disabled first. Later, during devres unwinding, the ALSA
core tears down the stream and invokes sun50i_dmic_trigger() with
SNDRV_PCM_TRIGGER_STOP.
Will the regmap_update_bits() calls in the trigger callback attempt to
access unclocked hardware and cause a kernel panic?
[Severity: High]
This is a pre-existing issue, but is there a missing reset assertion during
driver removal or probe failure?
In sun50i_dmic_probe(), the reset line is deasserted:
host->rst = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
if (IS_ERR(host->rst))
return dev_err_probe(&pdev->dev, PTR_ERR(host->rst),
"Failed to get reset.\n");
reset_control_deassert(host->rst);
However, I don't see a corresponding reset_control_assert() in the
error paths of probe, nor in sun50i_dmic_remove().
Does this leave the IP block actively running out of reset across driver
unbinds or re-probes, potentially causing logic errors or audio capture
failures on subsequent driver reloads?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260602113136.49494-1-phucduc.bui@gmail.com?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread