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 <[email protected]> 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/[email protected]?part=5
