Re: [PATCH v3 4/4] rtc: pcf85063: add 1 Hz update interrupt (UIE) support

[sashiko-bot@kernel.org]

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

New issues:
- [High] The driver completely ignores the requested alarm enable parameters, breaking generic `alarmtimers` (POSIX timers) and system wakeups.

Pre-existing issues:
- [High] In `pcf85063_rtc_alarm_irq_enable()`, the `PCF85063_CTRL2_AF` (Alarm Flag) bit is omitted from the `regmap_update_bits` mask and value, which can inadvertently clear a pending alarm interrupt.
--

commit 72f77be444d700f19ce672423be21e8db8ec0a3d
Author: Alexander Sverdlin <alexander.sverdlin@siemens.com>

rtc: pcf85063: add 1 Hz update interrupt (UIE) support

The PCF85063A/RV8263 have no dedicated 1 Hz update interrupt, but their
countdown timer can be sourced from a 1 Hz clock (TCF = 1 Hz, T = 1) to
generate a periodic interrupt on the INT pin. Use it to offload UIE to
the hardware, following the pattern of other RTC drivers.

> diff --git a/drivers/rtc/rtc-pcf85063.c b/drivers/rtc/rtc-pcf85063.c
> index a3af86456ccfc..32952e73f2f1c 100644
> --- a/drivers/rtc/rtc-pcf85063.c
> +++ b/drivers/rtc/rtc-pcf85063.c

[ ... ]

> @@ -211,23 +275,33 @@ static int pcf85063_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>  		return ret;
>  
>  	return regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
> -				  PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF,
> -				  alrm->enabled ? PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF : PCF85063_CTRL2_AF);
> +				  PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF | PCF85063_CTRL2_TF,
> +				  pcf85063->rtc->aie_timer.enabled ?
> +				  PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF | PCF85063_CTRL2_TF :
> +				  PCF85063_CTRL2_AF | PCF85063_CTRL2_TF);
>  }

[Severity: High]
Does this code ignore the requested alrm->enabled parameter in favor of
pcf85063->rtc->aie_timer.enabled?

The RTC core multiplexes various timers onto the hardware alarm and passes the
desired hardware state via alrm->enabled here and via the enabled parameter in
pcf85063_rtc_alarm_irq_enable().

By checking aie_timer.enabled instead, it appears the driver will only enable
the hardware alarm for the legacy ioctl alarm. If a generic alarmtimer (such
as a system wakealarm or a POSIX timer) reaches the front of the queue, won't
aie_timer.enabled be false, silencing the requested alarm?

>  static int pcf85063_rtc_alarm_irq_enable(struct device *dev,
>  					 unsigned int enabled)
>  {
>  	struct pcf85063 *pcf85063 = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = pcf85063_set_timer_1hz(pcf85063, pcf85063->rtc->uie_rtctimer.enabled);
> +	if (ret)
> +		return ret;
>  
>  	return regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
> -				  PCF85063_CTRL2_AIE,
> -				  enabled ? PCF85063_CTRL2_AIE : 0);
> +				  PCF85063_CTRL2_AIE | PCF85063_CTRL2_TF,
> +				  pcf85063->rtc->aie_timer.enabled ?
> +				  PCF85063_CTRL2_AIE | PCF85063_CTRL2_TF : PCF85063_CTRL2_TF);
>  }

[Severity: High]
This is a pre-existing issue, but does this read-modify-write omit
PCF85063_CTRL2_AF from the mask, potentially clearing a pending alarm?

Since the AF bit in PCF85063_REG_CTRL2 is write-0-to-clear, if the hardware
asserts the AF interrupt between the internal register read and write of this
operation, the read value will have AF=0. Because AF is not masked,
regmap_update_bits() will write back AF=0, which unintentionally clears the
newly asserted alarm flag.

Also, similar to pcf85063_rtc_set_alarm() above, does this code
inappropriately ignore the enabled parameter passed by the core?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828164445.3907839-1-alexander.sverdlin@siemens.com?part=4