Re: [PATCH] rtc: pcf2127: arm the alarm IRQ as a wake IRQ

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] rtc: pcf2127: duplicate wake IRQ registration in pcf2127_probe() on I2C
--

commit bb7257d310771e0aa92e0b399681a4feb16cf42a
Author: Aristo Chen <aristo.chen@canonical.com>

rtc: pcf2127: arm the alarm IRQ as a wake IRQ

This commit registers the RTC alarm IRQ with devm_pm_set_wake_irq() in
pcf2127_probe() so the PM core arms the interrupt as a wakeup source
during system suspend.

> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index ff2023908f5a3..51d8aebf34069 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
[ ... ]
> @@ -1251,6 +1252,12 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>  	if (alarm_irq > 0 || device_property_read_bool(dev, "wakeup-source")) {
>  		device_init_wakeup(dev, true);
>  		set_bit(RTC_FEATURE_ALARM, pcf2127->rtc->features);
> +
> +		if (alarm_irq > 0) {
> +			ret = devm_pm_set_wake_irq(dev, alarm_irq);
> +			if (ret)
> +				dev_warn(dev, "failed to set wake IRQ: %d\n", ret);
> +		}
>  	}
>  
>  	if (pcf2127->cfg->has_int_a_b) {

[Severity: Medium]
Can calling devm_pm_set_wake_irq() unconditionally here trigger a kernel
warning when an I2C-attached PCF2127/2129/2131 has both interrupts and
wakeup-source specified in its device tree node (or has I2C_CLIENT_WAKE set
via ACPI)?

For I2C devices, of_i2c_get_board_info() in drivers/i2c/i2c-core-of.c sets
I2C_CLIENT_WAKE when wakeup-source is present:

    if (of_property_read_bool(node, "wakeup-source"))
            info->flags |= I2C_CLIENT_WAKE;

Before invoking the driver probe callback (pcf2127_i2c_probe() ->
pcf2127_probe()), i2c_device_probe() in drivers/i2c/i2c-core-base.c already
initializes the wake IRQ when I2C_CLIENT_WAKE is set:

    if (client->flags & I2C_CLIENT_WAKE) {
            int wakeirq;

            wakeirq = fwnode_irq_get_byname(fwnode, "wakeup");
            if (wakeirq == -EPROBE_DEFER) {
                    status = dev_err_probe(dev, wakeirq, "can't get wakeirq\n");
                    goto put_sync_adapter;
            }

            device_init_wakeup(&client->dev, true);

            if (wakeirq > 0 && wakeirq != client->irq)
                    status = dev_pm_set_dedicated_wake_irq(dev, wakeirq);
            else if (client->irq > 0)
                    status = dev_pm_set_wake_irq(dev, client->irq);

When pcf2127_probe() then calls devm_pm_set_wake_irq(dev, alarm_irq),
dev_pm_attach_wake_irq() in drivers/base/power/wakeirq.c sees
dev->power.wakeirq is already initialized and fires dev_WARN_ONCE():

    spin_lock_irqsave(&dev->power.lock, flags);
    if (dev_WARN_ONCE(dev, dev->power.wakeirq,
                      "wake irq already initialized\n")) {
            spin_unlock_irqrestore(&dev->power.lock, flags);
            return -EEXIST;
    }

returning -EEXIST and printing the warning in pcf2127_probe().

Would it make sense to skip calling devm_pm_set_wake_irq() when the bus core
has already configured the wakeup source (or when dev->power.wakeirq is
already set)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020125124.1097546-1-aristo.chen@canonical.com?part=1