Re: [PATCH v2 3/3] 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 msleep() retry in probe stalls
  deferred_probe_work
- [High] soundwire: qcom: unused static clk_get_retry_optional() breaks
  Werror build
- [High] ASoC: qcom: missing Signed-off-by trailer and commit message
  body
- [Low] ASoC: qcom: clk_get_retry() helpers duplicated across five
  driver files
--

commit 9ed54c71b350e8398fe1839f5ec457bda202d8b0
Author: Daniel Carter <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 the commit message
body contains only a diffstat without explaining why this change is
needed.

> 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]
Does sleeping for 30 seconds in clk_get_retry() and
clk_get_retry_optional() block the very probe thread needed to register
the supplier clocks?

None of these drivers sets PROBE_PREFER_ASYNCHRONOUS, so both initcalls
and deferred_probe_work_func() run .probe() callbacks sequentially on a
single thread.  The modified drivers also supply clocks to one another:
va_macro_probe() registers the fsgen clock consumed by rx_macro_probe(),
tx_macro_probe(), and wsa_macro_probe(), while the macro drivers register
the mclk clock consumed as iface by qcom_swrm_probe().

Because drivers/soundwire/ and wsa-macro are linked and ordered in device
trees before va-macro, a consumer probing before va_macro_probe() (or
q6prmcc) sleeps for the full 30 seconds on that single thread, preventing
the supplier from probing and registering the clock until the loop gives
up and returns -EPROBE_DEFER anyway.  This stalls boot or deferred probing
for 30 seconds per device and can expire driver_deferred_probe_timeout,
while in va_macro_probe() sleeping on va->npl also leaves the va->pds
power domains powered on for 30 seconds.

In addition, clock suppliers registered from a driver .probe() callback do
trigger deferred probe retries via really_probe() -> driver_bound() ->
driver_deferred_probe_trigger().

> +
> +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;
> +}
>  
>  #define SWRM_COMP_SW_RESET					0x008
>  #define SWRM_COMP_STATUS					0x014
> @@ -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");

[Severity: High]
Does defining clk_get_retry_optional() in drivers/soundwire/qcom.c without
any callers trigger a -Wunused-function warning and break builds with
CONFIG_WERROR=y?

> 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]
This isn't a bug, but clk_get_retry() and clk_get_retry_optional() are
duplicated verbatim across five source files, four of which already share
sound/soc/codecs/lpass-macro-common.h.

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