Re: [PATCH 5/6] soc: qcom: qpace: Add LLCC slice support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] qcom/qpace: unconditional probe failure if LLCC is disabled or absent
- [Medium] qcom/qpace: double deactivation of LLCC slices on failed resume
- [Low] qcom/qpace: missing Kconfig dependency causes linker error when LLCC=m
--

commit 91b78e77e6f189e8963165dc5c4bd1f6ed1995ec
Author: Georgi Djakov <georgi.djakov@oss.qualcomm.com>

soc: qcom: qpace: Add LLCC slice support

This commit adds support for QPaCE to use dedicated LLCC slices for compression
and decompression. It updates the driver to request and activate the LLCC slices
during probe and resume, and deactivate them during suspend and remove.

> diff --git a/drivers/soc/qcom/qpace.c b/drivers/soc/qcom/qpace.c
> index f5b21ffbcbff5..86a935ae2ca93 100644
> --- a/drivers/soc/qcom/qpace.c
> +++ b/drivers/soc/qcom/qpace.c
[ ... ]
> @@ -660,9 +663,35 @@ static int qpace_probe(struct platform_device *pdev)
>  	if (ret)
>  		return dev_err_probe(dev, ret, "failed to map QPaCE registers\n");
>  
> +	priv->llc_comp = llcc_slice_getd(LLCC_QPACE_COMPRESSION);
> +	if (IS_ERR(priv->llc_comp))
> +		return dev_err_probe(dev, PTR_ERR(priv->llc_comp),
> +				     "failed to get compression LLCC slice\n");

[Severity: High]
Does this unconditionally fail the probe if LLCC is disabled or missing from
the device tree? If CONFIG_QCOM_LLCC=n, llcc_slice_getd() returns NULL (which
passes the IS_ERR check). Then the subsequent llcc_slice_activate(NULL)
invokes a macro stub returning -EINVAL, unconditionally failing the probe.

Additionally, if the QPaCE slice is absent from the device tree, it returns
an error pointer, which also aborts probe. Should this be treated as an
optional optimization rather than a hard requirement?

[Severity: Low]
Can this cause a linker error if CONFIG_QCOM_PAGE_COMPRESSION_ENGINE=y but
CONFIG_QCOM_LLCC=m? In this configuration, the built-in QPaCE driver cannot
link against the exported symbols of the LLCC module, but
IS_ENABLED(CONFIG_QCOM_LLCC) evaluates to true, so it attempts to use the
header declarations rather than inline stubs. Is there a missing Kconfig
dependency?

> +
> +	priv->llc_decomp = llcc_slice_getd(LLCC_QPACE_DECOMPRESSION);
> +	if (IS_ERR(priv->llc_decomp)) {
> +		ret = dev_err_probe(dev, PTR_ERR(priv->llc_decomp),
> +				    "failed to get decompression LLCC slice\n");
> +		priv->llc_decomp = NULL;
> +		goto llc_put_comp;
> +	}
> +
> +	ret = llcc_slice_activate(priv->llc_comp);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "failed to activate compression LLCC slice\n");
> +		goto llc_put_decomp;
> +	}
[ ... ]
> @@ -708,6 +745,10 @@ static void qpace_remove(struct platform_device *pdev)
>  	/* No callers remain; tear down the hardware. */
>  	cancel_work_sync(&qpace_priv->disable_work);
>  	qpace_power_off(&pdev->dev);
> +	llcc_slice_deactivate(qpace_priv->llc_decomp);
> +	llcc_slice_deactivate(qpace_priv->llc_comp);

[Severity: Medium]
Can this cause a double-deactivation if a previous qpace_resume() failed? If
an error occurs during qpace_resume() (e.g., if icc_set_bw() fails), the
driver leaves the slices deactivated and returns early.

If the device is subsequently unbound, qpace_remove() unconditionally calls
llcc_slice_deactivate() on both slices. Since their refcounts would already
be 0, wouldn't this underflow the refcount, triggering a
WARN_ON_ONCE(!refcount_dec_if_one(...)) in the LLCC core and sending an
erroneous deactivate command to the hardware?

> +	llcc_slice_putd(qpace_priv->llc_decomp);
> +	llcc_slice_putd(qpace_priv->llc_comp);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930145215.3380977-1-georgi.djakov@oss.qualcomm.com?part=5