Re: [PATCH 2/2] ASoC: qcom: lpass macro / soundwire: retry deferred clock acquisition

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] ASoC: qcom: 30s sleep in clk_get_retry() stalls sequential
  probe
- [High] soundwire: qcom: unused clk_get_retry_optional() breaks -Werror
  build
- [High] ASoC: qcom: missing Signed-off-by trailer and commit message
  body
- [Low] ASoC: qcom: duplicate clk_get_retry() helpers across five
  drivers
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] soundwire: qcom: ignored clk_prepare_enable() return values [1]
--

commit e7d8a177bfe78eb7a13933e19bc2397f7ed88249
Author: yass27106 <yass27106@gmail.com>

> ASoC: qcom: lpass macro / soundwire: retry deferred clock acquisition
> 
> drivers/soundwire/qcom.c           | 37 +++++++++++++++++++++++-
>  sound/soc/codecs/lpass-rx-macro.c  | 45 ++++++++++++++++++++++++++----
>  sound/soc/codecs/lpass-tx-macro.c  | 45 ++++++++++++++++++++++++++----
>  sound/soc/codecs/lpass-va-macro.c  | 43 +++++++++++++++++++++++++---
>  sound/soc/codecs/lpass-wsa-macro.c | 45 ++++++++++++++++++++++++++----
>  5 files changed, 195 insertions(+), 20 deletions(-)

[Severity: High]
This commit is missing a Signed-off-by trailer and a changelog description
explaining the problem and the reason for the change; the commit body only
contains a diffstat.

> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index c31d921f679f9..afb71624dae2a 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
> @@ -21,6 +21,41 @@
>  #include <sound/pcm_params.h>
>  #include <sound/soc.h>
>  #include "bus.h"
> +#include <linux/delay.h>
> +
> +/*
> + * Bounded retry for deferred clock providers. Clock-provider registration
> + * does not trigger deferred-probe retries, so a consumer that loses the
> + * probe-order race stays deferred forever. Wait up to 30 s for providers
> + * that are registered milliseconds later.
> + */
> +static struct clk *clk_get_retry(struct device *dev, const char *id)
> +{
> +	struct clk *c;
> +	int i;
> +
> +	for (i = 0; i < 60; i++) {
> +		c = devm_clk_get(dev, id);
> +		if (!IS_ERR(c) || PTR_ERR(c) != -EPROBE_DEFER)
> +			return c;
> +		msleep(500);
> +	}
> +	return c;
> +}

[Severity: High]
Won't sleeping for 30 seconds in clk_get_retry() and clk_get_retry_optional()
on -EPROBE_DEFER stall the same thread that needs to probe the clock
provider?

When any clock provider finishes probing, really_probe() -> driver_bound()
calls driver_deferred_probe_trigger() to schedule deferred_probe_work.
Because none of these drivers set PROBE_PREFER_ASYNCHRONOUS, both initial
probing in do_initcalls() and retries in deferred_probe_work_func() run
sequentially on a single thread.

In both link order and Device Tree node order, consumers are queued before
their suppliers: wsa_macro_probe(), rx_macro_probe(), and tx_macro_probe()
request the fsgen clock registered by va_macro_probe(), and qcom_swrm_probe()
requests the iface clock registered by the macro drivers.  Sleeping inside the
consumer's probe callback blocks the initcall or deferred_probe_work thread
from advancing to probe the supplier, guaranteeing the clock cannot appear
during the 30-second sleep while holding device_lock(dev) (and active power
domains in va_macro_probe()).

> +
> +static struct clk *clk_get_retry_optional(struct device *dev, const char *id)
> +{
> +	struct clk *c;
> +	int i;
> +
> +	for (i = 0; i < 60; i++) {
> +		c = devm_clk_get_optional(dev, id);
> +		if (!IS_ERR(c) || PTR_ERR(c) != -EPROBE_DEFER)
> +			return c;
> +		msleep(500);
> +	}
> +	return c;
> +}

[Severity: High]
Does clk_get_retry_optional() have any callers in drivers/soundwire/qcom.c?
qcom_swrm_probe() only calls clk_get_retry(), so defining
clk_get_retry_optional() here triggers a -Wunused-function warning and breaks
builds with CONFIG_WERROR=y.

> @@ -1669,7 +1704,7 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>  		goto err_init;
>  	}
>  
> -	ctrl->hclk = devm_clk_get(dev, "iface");
> +	ctrl->hclk = clk_get_retry(dev, "iface");
>  	if (IS_ERR(ctrl->hclk)) {
>  		ret = dev_err_probe(dev, PTR_ERR(ctrl->hclk), "unable to get iface clock\n");
>  		goto err_init;

[Severity: High]
This problem wasn't introduced by this patch, but clk_prepare_enable() on
ctrl->hclk is called without checking its return value in qcom_swrm_probe(),
qcom_swrm_irq_handler(), and swrm_runtime_resume() [1]:

drivers/soundwire/qcom.c:qcom_swrm_irq_handler() {
	...
	clk_prepare_enable(ctrl->hclk);

	ctrl->reg_read(ctrl, ctrl->reg_layout[SWRM_REG_INTERRUPT_STATUS],
		       &intr_sts);
	...
}

If clk_prepare_enable() fails, subsequent MMIO reads and writes access
unclocked SoundWire controller registers, triggering a synchronous external
abort on Qualcomm platforms.

> diff --git a/sound/soc/codecs/lpass-rx-macro.c b/sound/soc/codecs/lpass-rx-macro.c
> index ab8a1c2b0489b..c953054eb7a38 100644
> --- a/sound/soc/codecs/lpass-rx-macro.c
> +++ b/sound/soc/codecs/lpass-rx-macro.c
> @@ -18,6 +18,41 @@
>  #include <linux/clk-provider.h>
>  
>  #include "lpass-macro-common.h"
> +#include <linux/delay.h>
> +
> +/*
> + * Bounded retry for deferred clock providers. Clock-provider registration
> + * does not trigger deferred-probe retries, so a consumer that loses the
> + * probe-order race stays deferred forever. Wait up to 30 s for providers
> + * that are registered milliseconds later.
> + */
> +static struct clk *clk_get_retry(struct device *dev, const char *id)
> +{
> +	struct clk *c;
> +	int i;
> +
> +	for (i = 0; i < 60; i++) {
> +		c = devm_clk_get(dev, id);
> +		if (!IS_ERR(c) || PTR_ERR(c) != -EPROBE_DEFER)
> +			return c;
> +		msleep(500);
> +	}
> +	return c;
> +}
> +
> +static struct clk *clk_get_retry_optional(struct device *dev, const char *id)
> +{
> +	struct clk *c;
> +	int i;
> +
> +	for (i = 0; i < 60; i++) {
> +		c = devm_clk_get_optional(dev, id);
> +		if (!IS_ERR(c) || PTR_ERR(c) != -EPROBE_DEFER)
> +			return c;
> +		msleep(500);
> +	}
> +	return c;
> +}

[Severity: Low]
clk_get_retry() and clk_get_retry_optional() are duplicated verbatim across
drivers/soundwire/qcom.c, lpass-rx-macro.c, lpass-tx-macro.c,
lpass-va-macro.c, and lpass-wsa-macro.c, even though the four LPASS macro
drivers already share lpass-macro-common.h.

[1] https://sashiko.dev/#/bug/linux-f65cddc9-08ff-4531-9acf-76981d74d914

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/01b2c920412848ab85493acc2d4f5fc1@gmail.com?part=2