Re: [PATCH v3 2/3] power: sequencing: qcom-wcn: support SW_CTRL wake

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] power: sequencing: missing wake IRQ trigger type in pwrseq_qcom_wcn_probe()
--

commit 67f02e162090cebd2a43e112d0d0073d05afbfee
Author: Linghui Wu <linghui.wu@oss.qualcomm.com>

power: sequencing: qcom-wcn: support SW_CTRL wake

When the WCN PMU is marked with the wakeup-source property, this commit
requests the SW_CTRL GPIO as an input, maps it to an IRQ, and registers
it as a dedicated wake IRQ.

> diff --git a/drivers/power/sequencing/pwrseq-qcom-wcn.c b/drivers/power/sequencing/pwrseq-qcom-wcn.c
> index 636dd7e638917..69b7eb565996e 100644
> --- a/drivers/power/sequencing/pwrseq-qcom-wcn.c
> +++ b/drivers/power/sequencing/pwrseq-qcom-wcn.c

[ ... ]

> @@ -526,6 +535,33 @@ static int pwrseq_qcom_wcn_probe(struct platform_device *pdev)
>  		return dev_err_probe(dev, PTR_ERR(ctx->bt_gpio),
>  				     "Failed to get the Bluetooth enable GPIO\n");
>  
> +	if (device_property_read_bool(dev, "wakeup-source")) {
> +		swctrl_gpio = devm_gpiod_get(dev, "swctrl", GPIOD_IN);
> +		if (IS_ERR(swctrl_gpio))
> +			return dev_err_probe(dev, PTR_ERR(swctrl_gpio),
> +					     "Failed to get the SW_CTRL GPIO\n");
> +
> +		wake_irq = gpiod_to_irq(swctrl_gpio);
> +		if (wake_irq < 0)
> +			return dev_err_probe(dev, wake_irq,
> +					     "Failed to get the SW_CTRL wake IRQ\n");
> +
> +		ret = devm_device_init_wakeup(dev);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to enable wakeup\n");
> +
> +		ret = dev_pm_set_dedicated_wake_irq(dev, wake_irq);

[Severity: High]
Does wake_irq need an IRQ trigger type configured via irq_set_irq_type()
before calling dev_pm_set_dedicated_wake_irq()?

In pwrseq_qcom_wcn_probe(), gpiod_to_irq() maps swctrl_gpio through
gpiochip_to_irq() in drivers/gpio/gpiolib.c with IRQ_TYPE_NONE:

    spec.fwnode = domain->fwnode;
    spec.param_count = 2;
    spec.param[0] = gc->irq.child_offset_to_irq(gc, offset);
    spec.param[1] = IRQ_TYPE_NONE;

    return irq_create_fwspec_mapping(&spec);

Then dev_pm_set_dedicated_wake_irq() calls
__dev_pm_set_dedicated_wake_irq() in drivers/base/power/wakeirq.c, which
requests the threaded IRQ without any IRQF_TRIGGER_* flags:

    err = request_threaded_irq(irq, NULL, handle_threaded_wake_irq,
                               IRQF_ONESHOT | IRQF_NO_AUTOEN,
                               wirq->name, wirq);

In __setup_irq() in kernel/irq/manage.c, because both new->flags and
irqd_get_trigger_type() have trigger type 0 (IRQ_TYPE_NONE),
__irq_set_trigger() is never called:

    if (!shared) {
        /* Setup the type (level, edge polarity) if configured: */
        if (new->flags & IRQF_TRIGGER_MASK) {
            ret = __irq_set_trigger(desc,
                                    new->flags & IRQF_TRIGGER_MASK);

            if (ret)
                goto out_unlock;
        }

On Qualcomm TLMM in drivers/pinctrl/qcom/pinctrl-msm.c, msm_gpio_init()
initializes GPIO IRQs with IRQ_TYPE_NONE and handle_bad_irq:

    girq->default_type = IRQ_TYPE_NONE;
    girq->handler = handle_bad_irq;

Without .irq_set_type() being invoked, the hardware trigger registers in
TLMM/PDC/MPM or SPMI PMIC GPIO are not programmed, and the flow handler on
TLMM remains handle_bad_irq when dev_pm_arm_wake_irq() enables wake_irq
during system suspend.

Other callers combining gpiod_to_irq() with
dev_pm_set_dedicated_wake_irq(), such as pci_configure_wake_irq() in
drivers/pci/of.c and stm32_add_pcie_port() in
drivers/pci/controller/dwc/pcie-stm32.c, call irq_set_irq_type() first.

> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to set the SW_CTRL wake IRQ\n");
> +
> +		ret = devm_add_action_or_reset(dev,
> +					       pwrseq_qcom_wcn_clear_wake_irq, dev);
> +		if (ret)
> +			return ret;
> +	}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019093911.3199891-1-linghui.wu@oss.qualcomm.com?part=2