Re: [RESEND v7 2/2] rtc: Add NXP PCF85053 driver support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The OF clock provider registered with `of_clk_add_provider()` is never unregistered, leading to a resource leak and use-after-free on driver unbind or probe failure.
- [High] `pcf85053_rtc_set_time()` unconditionally zeroes out the interleaved alarm registers, destroying any configured alarm when setting the time.
- [High] Forcing 24-hour and binary mode in `set_time` and `set_alarm` corrupts existing timekeeping and writes binary data to a BCD-configured secondary interface.
- [Medium] The Oscillator Fail (OF) bit is never cleared after setting a valid time, permanently locking the RTC in an invalid state after a power loss.
- [Medium] `pcf85053_rtc_read_time()` reads the status register but ignores the result, failing to check the Oscillator Fail (OF) bit for invalid time.
--

--- Patch [2]: [RESEND v7 2/2] rtc: Add NXP PCF85053 driver support ---
commit 9c8ab988d6c43472440d6251d1c52f7cbe264dbb
Author: Lakshay Piplani <lakshay.piplani@nxp.com>

rtc: Add NXP PCF85053 driver support

This commit adds support for the NXP PCF85053 I2C RTC, which supports timer
and calendar functionality. It introduces the driver code, Kconfig options,
and Makefile updates.

> diff --git a/drivers/rtc/rtc-pcf85053.c b/drivers/rtc/rtc-pcf85053.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/rtc/rtc-pcf85053.c
[ ... ]
> +static int pcf85053_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct pcf85053 *pcf85053 = dev_get_drvdata(dev);
> +	unsigned int ctrl, st, h12;
> +	bool is_24h, is_bin;
> +	u8 regs[10], hr;
> +	int err;
> +
> +	err = regmap_read(pcf85053->regmap, PCF85053_REG_CTRL, &ctrl);
> +	if (err)
> +		return err;
> +
> +	err = regmap_read(pcf85053->regmap, PCF85053_REG_ST, &st);
> +	if (err)
> +		return err;
> +
> +	if (ctrl & PCF85053_BIT_ST)
> +		return -EINVAL;

[Severity: Medium]
Is it intentional that the st variable is read but never evaluated here?

Because st is ignored, the driver doesn't check the PCF85053_BIT_OF flag.
Would this allow the system to boot and blindly use a corrupted time from a
failed oscillator instead of returning -EINVAL?

[ ... ]
> +static int pcf85053_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +
> +{
> +	struct pcf85053 *pcf85053 = dev_get_drvdata(dev);
> +	unsigned int ctrl;
> +	int err, ret;
> +	u8 buf[10];
> +
> +	/*
> +	 * By default, secondary have write access to time registers as TWO
> +	 * bit is 0 by default, if we set nxp,interface = "primary" and the
> +	 * nxp,write-access in device tree, then TWO bits gets set and primary
> +	 * gets write access to time registers.
> +	 */
> +	if (!pcf85053_time_write_access(pcf85053))
> +		return -EACCES;
> +
> +	err = regmap_read(pcf85053->regmap, PCF85053_REG_CTRL, &ctrl);
> +	if (err)
> +		return err;
> +
> +	/*
> +	 * HF = 1, 24-hour mode
> +	 * DM = 1, binary (not BCD)
> +	 */
> +	if (!(ctrl & PCF85053_BIT_HF) ||
> +	    !(ctrl & PCF85053_BIT_DM)) {
> +		err = regmap_update_bits(pcf85053->regmap, PCF85053_REG_CTRL,
> +					 PCF85053_BIT_HF | PCF85053_BIT_DM,
> +					 PCF85053_BIT_HF | PCF85053_BIT_DM);
> +		if (err)
> +			return err;
> +	}

[Severity: High]
Does forcing 24-hour and binary mode here corrupt existing timekeeping?

If the hardware is currently configured in BCD mode, updating the control
register without converting the current time registers would instantly cause
the hardware to interpret the existing BCD values as binary.

Additionally, since the secondary interface has read-only access to control
registers, this regmap_update_bits() call will be ignored by the hardware when
invoked from a secondary interface. However, the driver will still proceed to
write binary-formatted time values. Could this break the clock by writing
binary values that the hardware will interpret as BCD?

> +
> +	buf[0] = tm->tm_sec & 0x7F;
> +	buf[1] = 0;
> +	buf[2] = tm->tm_min & 0x7F;
> +	buf[3] = 0;
> +	buf[4] = tm->tm_hour & PCF85053_HR_24H_MASK;
> +	buf[5] = 0;
> +	buf[6] = tm->tm_wday & 0x07;
> +	buf[7] = tm->tm_mday & 0x3F;
> +	buf[8] = (tm->tm_mon + 1) & 0x1F;
> +	buf[9] = (tm->tm_year - 100) & 0xFF;
> +
> +	if (pcf85053->is_primary) {
> +		err = regmap_update_bits(pcf85053->regmap, PCF85053_REG_CTRL,
> +					 PCF85053_BIT_ST, PCF85053_BIT_ST);
> +		if (err)
> +			return err;
> +
> +		ret = regmap_bulk_write(pcf85053->regmap, PCF85053_REG_SC, buf, sizeof(buf));
> +		err = regmap_update_bits(pcf85053->regmap, PCF85053_REG_CTRL,
> +					 PCF85053_BIT_ST, 0);
> +		return ret ? ret : err;
> +	}
> +
> +	return regmap_bulk_write(pcf85053->regmap, PCF85053_REG_SC, buf, sizeof(buf));
> +}

[Severity: High]
Does this bulk write unconditionally destroy any configured alarms?

The hardware interleaves time and alarm registers. By hardcoding 0 at the
alarm register indices (buf[1], buf[3], buf[5]) and doing a 10-byte bulk
write, calling set_time will clobber any pending alarm previously configured.

[ ... ]
> +static int pcf85053_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *tm)
> +{
> +	struct pcf85053 *pcf85053 = dev_get_drvdata(dev);
> +	unsigned int ctrl;
> +	u8 sec, min, hr;
> +	int err;
> +
> +	/*
> +	 * Only primary can set alarm, as secondary have read only access
> +	 * to alarm, control and status registers
> +	 */
> +	if (!pcf85053->is_primary)
> +		return -EACCES;
> +
> +	err = regmap_read(pcf85053->regmap, PCF85053_REG_CTRL, &ctrl);
> +	if (err)
> +		return err;
> +
> +	/*
> +	 * HF = 1, 24-hour mode
> +	 * DM = 1, binary (not BCD)
> +	 */
> +	if (!(ctrl & PCF85053_BIT_HF) ||
> +	    !(ctrl & PCF85053_BIT_DM)) {
> +		err = regmap_update_bits(pcf85053->regmap, PCF85053_REG_CTRL,
> +					 PCF85053_BIT_HF | PCF85053_BIT_DM,
> +					 PCF85053_BIT_HF | PCF85053_BIT_DM);
> +		if (err)
> +			return err;
> +	}

[Severity: High]
Just like in pcf85053_rtc_set_time(), does flipping the mode bits here without
updating the time registers cause the hardware to incorrectly interpret existing
time data?

[ ... ]
> +static int pcf85053_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
> +{
> +	struct pcf85053 *pcf85053 = dev_get_drvdata(dev);
> +	unsigned int val = 0, vl_status = 0;
> +	unsigned int bvl;
> +	int status;
> +
> +	switch (cmd) {
> +	case RTC_VL_READ:
> +		status = regmap_read(pcf85053->regmap, PCF85053_REG_ST, &val);
> +		if (status)
> +			return status;
> +
> +		if (val & PCF85053_BIT_OF)
> +			vl_status |= RTC_VL_DATA_INVALID;

[Severity: Medium]
Is there a missing mechanism to clear the Oscillator Fail (OF) bit?

The OF bit is reported here, but neither pcf85053_rtc_set_time() nor the ioctl
handler clears it. Since the hardware does not auto-clear this bit, will the
RTC permanently report invalid data to userspace after a power loss?

[ ... ]
> +static struct clk *pcf85053_clkout_register_clk(struct pcf85053 *pcf85053)
> +{
> +	struct device *dev = pcf85053->rtc->dev.parent;
> +	struct device_node *node = dev->of_node;
> +	struct clk *clk;
> +	struct clk_init_data init;
> +
> +	init.name = "pcf85053-clkout";
> +	init.ops = &pcf85053_clkout_ops;
> +	init.flags = 0;
> +	init.parent_names = NULL;
> +	init.num_parents = 0;
> +	pcf85053->clkout_hw.init = &init;
> +
> +	/* optional override of the clockname */
> +	of_property_read_string(node, "clock-output-names", &init.name);
> +
> +	/* register the clock */
> +	clk = devm_clk_register(dev, &pcf85053->clkout_hw);
> +
> +	if (!IS_ERR(clk))
> +		of_clk_add_provider(node, of_clk_src_simple_get, clk);
> +
> +	return clk;
> +}

[Severity: High]
Does this leak the OF clock provider?

The of_clk_add_provider() is called to register the clock, but there is no
corresponding teardown mechanism (like a devm action or a remove callback
calling of_clk_del_provider()).

If the driver is unbound or if devm_rtc_register_device() fails in
pcf85053_probe(), the clock hardware structure is freed by devres, leaving a
dangling pointer in the global of_clk_providers list. Any subsequent device
tree clock resolution could access freed memory.

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