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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Unsynchronized concurrent access to the shared timestamp state (`ts` and `ts_valid`) between the threaded IRQ handler and sysfs callbacks.
- [High] Read-modify-write on Write-0-to-Clear (W0C) interrupt flags drops concurrent interrupts.
- [High] Hardware timestamp engine freezes after the first event in IRQ mode due to missing CTS command.
- [High] The alarm interrupt is not properly configured as a system wake source, preventing the RTC alarm from waking the system from suspend.
- [Low] The I2C device ID array uses positional initializers rather than named initializers.
--

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

rtc: pcf8525: Add NXP PCF8525 RTC driver

Add support for time and date, alarms, timestamp capture, backup
switch-over configuration, external crystal configuration and clock
offset correction.

--- a/drivers/rtc/rtc-pcf8525.c
+++ b/drivers/rtc/rtc-pcf8525.c
[ ... ]
> +static ssize_t timestamp_show_common(struct device *dev, char *buf, int id)
> +{
> +	struct pcf8525 *pcf8525 = dev_get_drvdata(dev->parent);
> +	unsigned int ctrl4;
> +	time64_t ts;
> +	int ret;
> +
> +	if (id < 0 || id > 1)
> +		return 0;
> +
> +	if (pcf8525->irq_enabled) {
> +		if (!pcf8525->ts_valid[id])
> +			return 0;
> +		ts = pcf8525->ts[id];
> +		return sysfs_emit(buf, "%llu\n", (unsigned long long)ts);

[Severity: High]
Is it possible for the threaded IRQ handler to update the timestamp state
concurrently with this sysfs read?

If a user-space process reads the sysfs timestamp attribute at the same time
an RTC timestamp interrupt is being processed in pcf8525_ts_snapshot(),
this could result in a torn read on 32-bit platforms where time64_t requires
two 32-bit memory accesses.

Should a mutex or memory barrier be used to synchronize access to the
timestamp state?

> +	}
[ ... ]
> +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) {
> +		pcf8525->ts_valid[id] = false;
> +		return count;
> +	}

[Severity: High]
When in IRQ mode, this function returns early without sending the Clear
Timestamp (CTS) command to the hardware.

Since the hardware is configured in freeze mode (PCF8525_TS_CTL1_TSM), it
will ignore new events until the CTS command is issued. The interrupt handler
pcf8525_irq() only clears the TSF interrupt flag.

Will this cause the timestamp engine to stop capturing events after the first
one in IRQ mode?

> +
> +	/*
> +	 * Clear timestamp via RESET CTS command.
[ ... ]
> +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;
> +
> +	ret = regmap_read(pcf8525->regmap, PCF8525_REG_CTRL2, &ctrl2);
> +	if (ret)
> +		return IRQ_NONE;
> +
> +	ret = regmap_read(pcf8525->regmap, PCF8525_REG_CTRL4, &ctrl4);
> +	if (ret)
> +		return IRQ_NONE;
> +
> +	if (!(ctrl2 & (PCF8525_CTRL2_AF | PCF8525_CTRL2_MSF)) &&
> +	    !(ctrl4 & PCF8525_CTRL4_TSF))
> +		return IRQ_NONE;
> +
> +	/* Timestamp */
> +	if (ctrl4 & PCF8525_CTRL4_TSF) {
> +		pcf8525_ts_snapshot(dev);
> +		/* Clear TSF */
> +		regmap_update_bits(pcf8525->regmap, PCF8525_REG_CTRL4, PCF8525_CTRL4_TSF, 0);
> +	}
> +
> +	/* Alarm */
> +	if (ctrl2 & PCF8525_CTRL2_AF) {
> +		rtc_update_irq(pcf8525->rtc, 1, RTC_IRQF | RTC_AF);
> +		/* Clear AF */
> +		regmap_update_bits(pcf8525->regmap, PCF8525_REG_CTRL2, PCF8525_CTRL2_AF, 0);

[Severity: High]
Does using regmap_update_bits() here risk dropping concurrent interrupts?

This function performs a read-modify-write. If it writes 0 to clear AF or MSF,
it writes back 0 for any other Write-0-to-Clear flag that was 0 at the time
of the read. If a new hardware event sets another W0C flag between the read
and write, the write of 0 will inadvertently clear the new event before it can
be handled.

> +	}
> +
> +	if (ctrl2 & PCF8525_CTRL2_MSF) {
> +		ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_CTRL2, PCF8525_CTRL2_MSF, 0);
> +		if (ret)
> +			return IRQ_NONE;
> +	}
> +
> +	return IRQ_HANDLED;
> +}
[ ... ]
> +static int pcf8525_probe(struct i2c_client *client)
> +{
[ ... ]
> +	/* Optional IRQ */
> +	if (client->irq > 0) {
> +		ret = devm_request_threaded_irq(dev, client->irq,
> +						NULL, pcf8525_irq,
> +						IRQF_ONESHOT,
> +						dev_name(dev), dev);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "failed to request INTA IRQ\n");
> +
> +		pcf8525->irq_enabled = true;
> +
> +		/* Route/unmask alarm + timestamp to INTA by default */
> +		ret = pcf8525_unmask_irqs_intA(pcf8525);
> +		if (ret)
> +			return ret;
> +
> +		device_init_wakeup(dev, true);
> +		set_bit(RTC_FEATURE_ALARM, pcf8525->rtc->features);

[Severity: High]
This declares wakeup capability, but is the IRQ ever connected as a wake
source?

Without calling dev_pm_set_wake_irq() or implementing suspend/resume
callbacks with enable_irq_wake(), the IRQ controller is never instructed to
keep the interrupt armed during suspend.

Could this prevent the RTC alarm from waking the system from suspend?

> +	}
> +
> +	/* Register device */
> +	ret = devm_rtc_register_device(pcf8525->rtc);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id pcf8525_of_match[] = {
> +	{ .compatible = "nxp,pcf8525" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, pcf8525_of_match);
> +
> +static const struct i2c_device_id pcf8525_i2c_id[] = {
> +	{ "pcf8525", 0 },
> +	{ }
> +};

[Severity: Low]
Could this use named initializers?

The I2C subsystem guidelines require initialized arrays of type
struct i2c_device_id to use named initializers.

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