Re: [RESEND v4 2/5] rtc: pcf85363: support reporting battery switch-over via RTC_VL

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

New issues:
- [Medium] The IRQ handler silently clears the Battery Switch-over Flag (BSF) from the hardware register without saving its state, causing battery switch-over events to be lost if an interrupt triggers before userspace calls RTC_VL_READ.

Pre-existing issues:
- [High] `pcf85363_probe()` unconditionally wipes the `CTRL_FLAGS` register, destroying boot-time battery switch-over events before they can be read via `RTC_VL_READ`.
--

--- Patch [2]: [RESEND v4 2/5] rtc: pcf85363: support reporting battery switch-over via RTC_VL ---
commit 641bd03440f1f0094b5ca119b02a17fb2dd848b5
Author: Lakshay Piplani <[email protected]>

rtc: pcf85363: support reporting battery switch-over via RTC_VL

Add battery switch-over reporting for PCF85263/PCF85363 using the standard
RTC_VL_* ioctl interface. When the backup supply takes over, the BSF flag
is exposed to userspace through RTC_VL_READ and can be cleared using
RTC_VL_CLR.

> diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
> index 540042b9eec8f..c03d5a65c5f79 100644
> --- a/drivers/rtc/rtc-pcf85363.c
> +++ b/drivers/rtc/rtc-pcf85363.c
[ ... ]
> @@ -295,23 +296,67 @@ static int pcf85363_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>  static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
>  {
>  	struct pcf85363 *pcf85363 = i2c_get_clientdata(dev_id);
> +	bool handled = false;
>  	unsigned int flags;
>  	int err;
>  
>  	err = regmap_read(pcf85363->regmap, CTRL_FLAGS, &flags);
> +
>  	if (err)
>  		return IRQ_NONE;
>  
> +	if (flags) {
> +		dev_dbg(&pcf85363->rtc->dev, "IRQ flags: 0x%02x%s%s\n",
> +			flags, (flags & FLAGS_A1F) ? " [A1F]" : "",
> +			(flags & FLAGS_BSF) ? " [BSF]" : "");
> +	}
> +
>  	if (flags & FLAGS_A1F) {
>  		rtc_update_irq(pcf85363->rtc, 1, RTC_IRQF | RTC_AF);
>  		regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_A1F, 0);
> -		return IRQ_HANDLED;
> +		handled = true;
>  	}
>  
> -	return IRQ_NONE;
> +	if (flags & FLAGS_BSF) {
> +		regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_BSF, 0);
> +		handled = true;
> +	}

[Severity: Medium]
Does this unconditionally clear the hardware Battery Switch-over Flag (BSF)
without saving its state?

If an interrupt triggers before userspace calls RTC_VL_READ, won't the
battery switch-over event be lost since the hardware flag is cleared but not
cached in software?

> +
> +	return handled ? IRQ_HANDLED : IRQ_NONE;
> +}
> +
> +static int pcf85363_rtc_ioctl(struct device *dev,
> +			      unsigned int cmd, unsigned long arg)
> +{
> +	struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
> +	unsigned int val;
> +	int ret;
> +
> +	switch (cmd) {
> +	case RTC_VL_READ: {
> +		u32 status = 0;
> +
> +		ret = regmap_read(pcf85363->regmap, CTRL_FLAGS, &val);

[Severity: Medium]
Because RTC_VL_READ reads directly from the hardware register, it will return
0 if the IRQ handler has already cleared the flag.

> +
> +		if (ret)
> +			return ret;
> +
> +		if (val & FLAGS_BSF)
> +			status |= RTC_VL_BACKUP_SWITCH;
> +
> +		return put_user(status, (u32 __user *)arg);
> +	}
> +
> +	case RTC_VL_CLR:
> +		return regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_BSF, 0);
> +
> +	default:
> +		return -ENOIOCTLCMD;
> +	}
>  }

[Severity: High]
This isn't a bug introduced by this patch, but the pre-existing code
in pcf85363_probe() appears to break the boot-time power loss detection
intended by this new feature.

During system boot, pcf85363_probe() unconditionally clears all hardware flags
if an IRQ or wakeup source is configured:

drivers/rtc/rtc-pcf85363.c:pcf85363_probe() {
    ...
	if (client->irq > 0 || wakeup_source) {
		regmap_write(pcf85363->regmap, CTRL_FLAGS, 0);
		regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
				   PIN_IO_INTAPM, PIN_IO_INTA_OUT);
	}
    ...
}

Does this destroy the battery switch-over event before userspace can query it
with RTC_VL_READ?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2