Re: [PATCH v3 4/4] pinctrl: qcom: hawi-lpass-lpi: add Hawi LPASS LPI TLMM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Functions like `lpi_gpio_read()` and `lpi_gpio_set_mux()` incorrectly return the result of `pm_runtime_put_autosuspend()`, leading to spurious `-EAGAIN` failures during concurrent access.
- [High] The driver destroys its mutex in `lpi_pinctrl_remove()` and the error path of `lpi_pinctrl_probe()`, but relies on delayed `devm` teardown to unregister the pinctrl and gpiochip devices, creating a window where callbacks can access the destroyed mutex.
--

--- Patch [4]: [PATCH v3 4/4] pinctrl: qcom: hawi-lpass-lpi: add Hawi LPASS LPI TLMM ---
commit b206b5a7cb05f8fc2d4dea4f8c7ff6d338891d30
Author: Prasad Kumpatla <prasad.kumpatla@oss.qualcomm.com>

    pinctrl: qcom: hawi-lpass-lpi: add Hawi LPASS LPI TLMM
    
    Add pin controller driver for the Low Power Audio SubSystem (LPASS)
    Low Power Island (LPI) of Qualcomm Hawi SoC.
    
    The controller has 23 GPIOs with SoundWire, I2S, DMIC, VA I2S,
    ext_mclk1 and slimbus mux functions.
    
    Two pins (gpio13, gpio22) require 6 mux slots due to a test_bus
    entry occupying a middle hardware slot. Add LPI_PINGROUP6 macro
    to pinctrl-lpass-lpi.h to support pins with 5 alternate functions.

> diff --git a/drivers/pinctrl/qcom/pinctrl-hawi-lpass-lpi.c b/drivers/pinctrl/qcom/pinctrl-hawi-lpass-lpi.c
> new file mode 100644
> index 0000000000000..7036bf4c66920
> --- /dev/null
> +++ b/drivers/pinctrl/qcom/pinctrl-hawi-lpass-lpi.c

[ ... ]

While reviewing this new driver, I noticed a couple of pre-existing issues in
the core driver code that this driver uses:

drivers/pinctrl/qcom/pinctrl-lpass-lpi.c:lpi_gpio_read() {
    ...
    lpi_gpio_read_reg(state, pin, addr, val);

    return pm_runtime_put_autosuspend(state->dev);
}

[Severity: High]
This is a pre-existing issue, but does this cause spurious failures?

If multiple threads access the pinctrl device concurrently, for example if a
consumer holds a PM reference via pm_runtime_resume_and_get(), calling
pm_runtime_put_autosuspend() drops the usage counter but it remains greater
than zero. The PM core intentionally returns -EAGAIN in this scenario.

Returning this value causes lpi_gpio_read() to falsely report a failure back
to the consumer despite a successful hardware read. The same pattern is also
present in lpi_gpio_set_mux() and lpi_config_set().

I also noticed this in the probe and remove paths:

drivers/pinctrl/qcom/pinctrl-lpass-lpi.c:lpi_pinctrl_probe() {
    ...
    ret = devm_gpiochip_add_data(dev, &pctrl->chip, pctrl);
    if (ret) {
        dev_err(pctrl->dev, "can't add gpio chip\n");
        goto err_pinctrl;
    }
    return 0;

err_pinctrl:
    mutex_destroy(&pctrl->lock);
    return ret;
}

And similarly during removal:

drivers/pinctrl/qcom/pinctrl-lpass-lpi.c:lpi_pinctrl_remove() {
    struct lpi_pinctrl *pctrl = platform_get_drvdata(pdev);
    int i;

    mutex_destroy(&pctrl->lock);

    for (i = 0; i < pctrl->data->npins; i++)
        pinctrl_generic_remove_group(pctrl->ctrl, i);
}

[Severity: High]
This is a pre-existing issue, but does destroying this mutex before devres
cleanup create a race condition?

The pinctrl and gpiochip devices are registered using devm_ calls, meaning
they remain active until lpi_pinctrl_probe() or lpi_pinctrl_remove() returns
and the driver core executes devres cleanup. If a concurrent operation invokes
a callback like lpi_gpio_set_mux() during this window, it could attempt to
lock the destroyed mutex.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715133243.3170821-1-prasad.kumpatla@oss.qualcomm.com?part=4