Re: [PATCH v2 2/4] rtc: pcf8525: Add NXP PCF8525 RTC driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Read-modify-write on registers containing multiple Write-0-to-Clear (W0C) flags causes concurrent interrupt events to be silently dropped.
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race between the threaded IRQ handler and sysfs store operations can resurrect a cleared timestamp and drop a new one.
--

--- Patch [2]: [PATCH v2 2/4] rtc: pcf8525: Add NXP PCF8525 RTC driver ---
commit c245e33078f78e502aef6b16350b763f5d4857a7
Author: Shiv Prakash Gupta <shivprakash.gupta@nxp.com>
Subject: rtc: pcf8525: Add NXP PCF8525 RTC driver

This commit adds a new I2C driver for the NXP PCF8525 RTC. It provides
support for reading and setting time, alarms, timestamp capture, backup
switch-over configuration, and crystal aging offset correction.

> diff --git a/drivers/rtc/rtc-pcf8525.c b/drivers/rtc/rtc-pcf8525.c
> index 0000000000000..843d4bb266847
> --- /dev/null
> +++ b/drivers/rtc/rtc-pcf8525.c

[ ... ]

> +static int pcf8525_param_set(struct device *dev, struct rtc_param *param)
> +{
> +	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
> +	unsigned int mode;
> +
> +	switch (param->param) {
> +	case RTC_PARAM_BACKUP_SWITCH_MODE:
> +		switch (param->uvalue) {
> +		case RTC_BSM_LEVEL:
> +			mode = 0;
> +			break;
> +		case RTC_BSM_DIRECT:
> +			mode = 1;
> +			break;
> +		case RTC_BSM_DISABLED:
> +			mode = 2;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	return regmap_update_bits(pcf8525->regmap,
> +				  PCF8525_REG_CTRL3,
> +				  PCF8525_CTRL3_PWRMNG_MASK,
> +				  FIELD_PREP(PCF8525_CTRL3_PWRMNG_MASK, mode));

[Severity: High]
Does this read-modify-write operation drop concurrent hardware events?

If CTRL3 contains Write-0-to-Clear (W0C) flags (like the Battery Flag BF),
regmap_update_bits() will read the register, modify the mask, and write it
back. 

If a W0C flag is 0 when read, it writes 0 back. If that flag asserted in
hardware between the read and the write, writing 0 will unintentionally clear
it, causing the event to be silently dropped.

[ ... ]

> +static int pcf8525_alarm_irq_enable(struct device *dev, unsigned int enable)
> +{
> +	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_CTRL2,
> +				 PCF8525_CTRL2_AIE,
> +				 enable ? PCF8525_CTRL2_AIE : 0);

[Severity: High]
Can this similarly drop concurrent alarm, minute/second, or watchdog events?

CTRL2 contains multiple Write-0-to-Clear flags (AF, MSF, WDTF). By using
regmap_update_bits() here and in other functions like pcf8525_set_alarm()
and pcf8525_irq(), writing back the original 0 will clear any newly asserted
flags, permanently dropping the hardware interrupt events.

[ ... ]

> +static ssize_t timestamp_store_common(struct device *dev, const char *buf,
> +				      size_t count, int id)
> +{
> +	struct pcf8525 *pcf8525 = dev_get_drvdata(dev->parent);
> +	int ret;
> +
> +	if (id < 0 || id > 1)
> +		return -EINVAL;
> +
> +	if (pcf8525->irq_enabled) {
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&pcf8525->ts_lock, flags);
> +		pcf8525->ts_valid[id] = false;
> +		spin_unlock_irqrestore(&pcf8525->ts_lock, flags);
> +
> +		/* Also clear the hardware timestamp registers. */
> +		regmap_write(pcf8525->regmap, PCF8525_REG_RESET,
> +			     PCF8525_RESET_CTS_CMD);
> +		return count;
> +	}

[ ... ]

> +static irqreturn_t pcf8525_irq(int irq, void *data)
> +{
> +	struct device *dev = data;
> +	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
> +	unsigned int ctrl2, ctrl4;
> +	int ret;

[ ... ]

> +	/* Timestamp */
> +	if (ctrl4 & PCF8525_CTRL4_TSF) {
> +		time64_t ts0, ts1;
> +		bool ok0, ok1;
> +		unsigned long flags;
> +
> +		/*
> +		 * W0C: clear TSF before reading the timestamp registers so
> +		 * that a new event that fires during the I2C read is not lost.
> +		 */
> +		regmap_update_bits(pcf8525->regmap, PCF8525_REG_CTRL4,
> +				   PCF8525_CTRL4_TSF, 0);
> +
> +		/* Read hardware outside the spinlock (I2C operations may sleep). */
> +		ok0 = !pcf8525_ts_read(dev, 0, &ts0);
> +		ok1 = !pcf8525_ts_read(dev, 1, &ts1);
> +
> +		spin_lock_irqsave(&pcf8525->ts_lock, flags);
> +		if (ok0 && !pcf8525->ts_valid[0]) {
> +			pcf8525->ts[0] = ts0;
> +			pcf8525->ts_valid[0] = true;
> +		}

[Severity: High]
Does this sequence have a TOCTOU race with timestamp_store_common()?

If a user clears the timestamp via sysfs while pcf8525_irq() is performing the
unlocked I2C read, timestamp_store_common() will set ts_valid = false and clear
the hardware registers.

If a new hardware event fires immediately after, capturing a new timestamp,
pcf8525_irq() will finish its read, take the lock, and see !ts_valid[0] because
it was cleared by sysfs. It will then resurrect the stale read timestamp and
mark it valid again.

When the second IRQ fires for the new hardware event, it will find ts_valid
already true and silently discard the newly captured timestamp.

Could a mutex be used to serialize the entire sequence of hardware I2C access
and software state updates instead of using a spinlock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824065531.693701-1-shivprakash.gupta@nxp.com?part=2