Re: [PATCH v3 3/4] ASoC: qcom: lpass macro / soundwire: asynchronous probe with clock retry

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] soundwire: qcom: infinite recursion in clk_get_retry()
- [Medium] ASoC: qcom: 30s msleep() polling on -EPROBE_DEFER in
  lpass_macro_clk_get_retry()
--

commit 6bd2e1b16c32b85c538fe58be015563a596b1d7d
Author: Daniel Carter <yass27106@gmail.com>

ASoC: qcom: lpass macro / soundwire: asynchronous probe with clock retry

This commit adds 30-second polling retry helpers around clock acquisition
and sets PROBE_PREFER_ASYNCHRONOUS in the Qualcomm SoundWire and LPASS
macro drivers.

> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index c31d921f679f9..0bdeb076a2e0e 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
> @@ -21,6 +21,27 @@
>  #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 = clk_get_retry(dev, id);
> +		if (!IS_ERR(c) || PTR_ERR(c) != -EPROBE_DEFER)
> +			return c;
> +		msleep(500);
> +	}
> +	return c;
> +}

[Severity: High]
Does clk_get_retry() call itself instead of devm_clk_get(dev, id) here?
When qcom_swrm_probe() calls clk_get_retry(dev, "iface"), this recurses
infinitely and overflows the kernel stack.  Also, the commit message says
the retry helpers are shared through lpass-macro-common.h, whereas a
separate copy is defined here.

> diff --git a/sound/soc/codecs/lpass-macro-common.h b/sound/soc/codecs/lpass-macro-common.h
> index 10ad682019fa7..83bc7075a281c 100644
> --- a/sound/soc/codecs/lpass-macro-common.h
> +++ b/sound/soc/codecs/lpass-macro-common.h
[ ... ]
> @@ -74,4 +77,41 @@ static inline const char *lpass_macro_get_codec_version_string(int version)
>  	return "NA";
>  }
>  
> +
> +/*
> + * 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 inline struct clk *
> +lpass_macro_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 inline struct clk *
> +lpass_macro_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;
> +}
> +
>  #endif /* __LPASS_MACRO_COMMON_H__ */

[Severity: Medium]
Why poll with msleep(500) for up to 30 seconds on -EPROBE_DEFER instead of
returning -EPROBE_DEFER to the driver core?

Both suppliers mentioned in the commit message (q6dsp_clock_dev_probe() and
va_macro_register_fsgen_output()) register their clock providers inside
platform_driver probe callbacks, which trigger driver_deferred_probe_trigger()
through really_probe() -> driver_bound() once bound.  Even with
PROBE_PREFER_ASYNCHRONOUS, sleeping for 30 seconds inside probe while holding
device_lock(dev) stalls async_synchronize_full() and wait_for_device_probe()
callers (such as do_init_module() and kernel_init()) whenever a clock supplier
is delayed, missing firmware, or disabled in DT, and repeats the 30-second
stall on subsequent deferred-probe passes.

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