Re: [PATCH v6 3/5] rtc: pcf85363: add timestamp support with configurable timestamp mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] An I2C read error during event collection causes an early return, bypassing hardware flag clearance. Combined with returning IRQ_HANDLED, this leads to an infinite interrupt storm.
--

commit 71137d386eeb017385125a0418e12cdc1cafbbf2
Author: Lakshay Piplani <lakshay.piplani@nxp.com>

rtc: pcf85363: add timestamp support with configurable timestamp mode

Add support for the timestamp capture registers on PCF85263/PCF85363,
which latch the time on selected events (TS pin or battery switch-over).
The capture source is selected via the nxp,timestamp-mode device tree
property and latched values are exported through read-only sysfs
attributes, refreshed on read so poll-only boards still latch events.

> diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
> index c8c04312bc583..5cc854d617f3a 100644
> --- a/drivers/rtc/rtc-pcf85363.c
> +++ b/drivers/rtc/rtc-pcf85363.c

[ ... ]

> +static int pcf85363_collect_events(struct pcf85363 *pcf85363)
>  {
>  	unsigned int flags;
>  	int handled = 0;
> -	int ret;
> +	int i, ret;
>  
>  	ret = regmap_read(pcf85363->regmap, CTRL_FLAGS, &flags);
>  	if (ret)
>  		return ret;
>  
> +	for (i = 0; i < PCF85363_NUM_TS; i++) {
> +		if (!(flags & pcf85363_ts_flag[i]))
> +			continue;
> +
> +		if (pcf85363->ts_last_event[i]) {
> +			ret = pcf85363_clear_flags(pcf85363,
> +						   pcf85363_ts_flag[i]);

[ ... ]

> +		} else {
> +			ret = regmap_bulk_read(pcf85363->regmap,
> +					       pcf85363_ts_base[i],
> +					       pcf85363->ts[i].regs,
> +					       PCF85363_TS_LEN);
> +			if (ret)
> +				return ret;

[Severity: High]
If this bulk read fails, the function returns early, bypassing the call to
pcf85363_clear_flags() below and leaving the interrupt asserted in the
hardware. Is it possible for this early return to create a persistent
interrupt condition?

> +
> +			pcf85363->ts[i].valid = true;
> +
> +			ret = pcf85363_clear_flags(pcf85363,
> +						   pcf85363_ts_flag[i]);
> +			if (ret)
> +				return ret;
> +
> +			handled |= pcf85363_ts_flag[i];
> +		}
> +	}
> +

[ ... ]

> @@ -359,7 +488,7 @@ static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
>  		handled = true;
>  	}
>  
> -	if (flags & FLAGS_BSF) {
> +	if (flags & (FLAGS_TSR1F | FLAGS_TSR2F | FLAGS_TSR3F | FLAGS_BSF)) {
>  		guard(mutex)(&pcf85363->lock);
>  
>  		err = pcf85363_collect_events(pcf85363);
> @@ -368,8 +497,8 @@ static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
>  					    "failed to collect events: %d\n",
>  					    err);
>  		/*
> -		 * This is our interrupt source even if servicing it hit an
> -		 * I/O error, so acknowledge the interrupt either way.
> +		 * These are our interrupt sources even if servicing them hit
> +		 * an I/O error, so acknowledge the interrupt either way.
>  		 */
>  		handled = true;
>  	}

[Severity: High]
By unconditionally setting handled = true even after an I/O error, the
handler returns IRQ_HANDLED while the hardware interrupt line might still be
asserted (since pcf85363_collect_events() bails out early on a read error
without clearing the flags).

For a level-triggered interrupt, doesn't returning IRQ_HANDLED without clearing
the source circumvent the kernel's spurious IRQ detector (which relies on
IRQ_NONE to mask stuck interrupts)?

Could this result in an infinite interrupt storm that will peg a CPU core?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914092126.3757418-1-lakshay.piplani@nxp.com?part=3