Re: [PATCH] ASoC: qcom: q6dsp-lpass-clocks: Set driver data before registering clocks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] ASoC: qcom: q6dsp-lpass-clocks: handle used in Signed-off-by
  trailer
--

commit ca8f162c7c729ca1dd4a09d6760be1eed11d7dd6
Author: yzddMr6 <yzddmr6@gmail.com>

> ASoC: qcom: q6dsp-lpass-clocks: Set driver data before registering clocks
>
> The clk ops of the q6dsp clock controller look up struct q6dsp_cc with
> dev_get_drvdata(), but q6dsp_clock_dev_probe() sets the driver data only
> after it has registered the clocks and added the OF clock provider. A
> consumer that gets and prepares one of the clocks in between
> dereferences a NULL pointer.
>
> fw_devlink does not keep consumers out of that window. The q6prm and
> q6afe clock controllers are described below the ADSP remoteproc node,
> and their devices are only created once the ADSP has booted and its
> GPR/APR services have come up, long after the remoteproc device has
> bound. fw_devlink therefore makes the remoteproc device the supplier of
> their consumers. A consumer such as the LPASS LPI pinctrl then just
> defers until of_clk_get() succeeds, and its retry from the deferred
> probe worker can run while the clock controller is still in its probe.
>
> That happened once on a Xiaomi Pad 8 Pro (SM8750). While the LPI pinctrl
> probe registered its GPIO chip, gpiolib read a GPIO direction, which
> runtime resumed the device and prepared its q6prm vote clocks through
> pm_clk. The prepare op then faulted with a NULL pointer dereference at
> virtual address 0x350 (trimmed):
>
>   pc : clk_vote_q6dsp_block+0x24/0x58 [snd_q6dsp_common]
>   lr : clk_core_prepare+0x7c/0x298
>   Call trace:
>    clk_vote_q6dsp_block+0x24/0x58 [snd_q6dsp_common] (P)
>    clk_core_prepare+0x7c/0x298
>    clk_prepare+0x28/0x44
>    pm_clk_resume+0x10c/0x188
>    pm_generic_runtime_resume+0x2c/0x44
>    ...
>    __pm_runtime_resume+0x50/0x94
>    ...
>    lpi_gpio_get_direction+0x4c/0x8c [pinctrl_lpass_lpi]
>    gpiochip_add_data_with_key+0x7b0/0x10dc
>    devm_gpiochip_add_data_with_key+0x38/0x8c
>    lpi_pinctrl_probe+0x2b8/0x320 [pinctrl_lpass_lpi]
>    ...
>    deferred_probe_work_func+0x88/0xc0
>
> 0x350 is the offset of desc in struct q6dsp_cc; the driver data pointer
> it was loaded from was NULL. The oops left the clk prepare_lock held,
> which blocks every later clk_prepare(), and the system did not finish
> booting.
>
> On mainline the window is short, but it includes a wait for prepare_lock
> in clk_core_reparent_orphans(), which of_clk_add_hw_provider() calls
> right after listing the provider. The kernel that hit this had
> out-of-tree changes to drivers/clk/clk.c that do more work at that
> point, which makes the window wider.
>
> Both q6afe-clocks and q6prm-clocks use q6dsp_clock_dev_probe(). Set the
> driver data before the first clock is registered.
>
> The oops was analysed and the fix written with the help of an AI coding
> assistant.
>
> Fixes: 9ab71ac37240 ("ASoC: qdsp6: q6afe-clocks: move audio-clocks to common file")
> Cc: stable@vger.kernel.org
> Signed-off-by: yzddMr6 <yzddmr6@gmail.com>

[Severity: High]
The Author header and Signed-off-by trailer use an account handle
(yzddMr6) rather than a real human name.  Under the Developer's
Certificate of Origin in Documentation/process/submitting-patches.rst, a
known identity with a real name is required for the patch to be accepted.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115-upstream-q6dsp-clk-drvdata-v1-1-4d7dbea52e52@gmail.com?part=1