Re: [PATCH v2 2/3] rtc: rs5c372: add support for Ricoh R2223x

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver assigns the `s32` return value of `i2c_smbus_read_byte_data()` directly to an `unsigned char` without checking for negative error codes, which can silently corrupt the control register.
- [High] Stale cached register value is modified and written back in `rs5c_rtc_alarm_irq_enable()`, clobbering hardware state.
- [High] The probe function silently succeeds without registering the RTC device if the RTC type is unknown.
- [Medium] In `rs5c_set_alarm()`, a failed I2C write when enabling the alarm is ignored, resulting in an inconsistent cached state.
--

--- Patch [2]: [PATCH v2 2/3] rtc: rs5c372: add support for Ricoh R2223x ---
commit 8a1878e4309e6f90ca5c0f1dc3f2411879befd7b
Author: Heiko Schocher <hs@nabladev.com>
Subject: rtc: rs5c372: add support for Ricoh R2223x

This commit adds support for the Ricoh R2223x I2C RTC device to the rs5c372
driver. It updates the driver's type definitions, device ID tables, and match
tables, treating the R2223x similarly to the r2221tl and r2025sd models for
time setting, reading, and control register handling.

Closes: https://sashiko.dev/#/patchset/20260824110452.4038870-1-hs@nabladev.com?part=2
Closes: https://sashiko.dev/#/patchset/20260824110452.4038870-1-hs@nabladev.com?part=3

> diff --git a/drivers/rtc/rtc-rs5c372.c b/drivers/rtc/rtc-rs5c372.c
> index 24bd795d9d95d..c65b76cc0dc66 100644
> --- a/drivers/rtc/rtc-rs5c372.c
> +++ b/drivers/rtc/rtc-rs5c372.c
[ ... ]
> @@ -292,6 +300,7 @@ static int rs5c372_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  	switch (rs5c->type) {
>  	case rtc_r2025sd:
>  	case rtc_r2221tl:
> +	case rtc_r2223x:
>  		ctrl2 &= ~(R2x2x_CTRL2_VDET | R2x2x_CTRL2_PON);
>  		if (rs5c->type == rtc_r2025sd)
>  			ctrl2 |= R2x2x_CTRL2_XSTP;

[Severity: High]
This is a pre-existing issue, but does rs5c372_rtc_set_time() handle negative
error codes from i2c_smbus_read_byte_data() correctly here?

Just above this switch statement, the return value is assigned directly to an
unsigned char:

	ctrl2 = i2c_smbus_read_byte_data(client, addr);

If this read fails (e.g., returning -EIO), the negative error code is
truncated into an unsigned char. The driver then masks and modifies this
corrupted value before writing it back to the device, which could silently
corrupt the control register.

[ ... ]
> @@ -511,8 +520,10 @@ static int rs5c372_ioctl(struct device *dev, unsigned int cmd, unsigned long arg
>  		switch (rs5c->type) {
>  		case rtc_r2025sd:
>  		case rtc_r2221tl:
> +		case rtc_r2223x:
>  			if ((rs5c->type == rtc_r2025sd && !(ctrl2 & R2x2x_CTRL2_XSTP)) ||
> -				(rs5c->type == rtc_r2221tl &&  (ctrl2 & R2x2x_CTRL2_XSTP))) {
> +			    ((rs5c->type == rtc_r2221tl || rs5c->type == rtc_r2223x) &&
> +			     (ctrl2 & R2x2x_CTRL2_XSTP))) {
>  				flags |= RTC_VL_DATA_INVALID;

[Severity: High]
This is a pre-existing issue, but similar to the set_time function, does
rs5c372_ioctl() suffer from the same truncation problem?

On the RTC_VL_CLR path, the read value is assigned to an unsigned char:

	ctrl2 = i2c_smbus_read_byte_data(rs5c->client, addr);

If this read fails, the unhandled negative error code is truncated and
subsequently written back to the device, potentially clobbering the register.

[ ... ]
> @@ -847,6 +863,7 @@ static int rs5c372_probe(struct i2c_client *client)
>  		break;
>  	case rtc_r2025sd:
>  	case rtc_r2221tl:
> +	case rtc_r2223x:
>  	case rtc_rv5c386:
>  	case rtc_rv5c387a:
>  		if (rs5c372->regs[RS5C_REG_CTRL1] & RV5C387_CTRL1_24)

[Severity: High]
This is a pre-existing issue, but does rs5c372_probe() silently succeed if
the RTC type is unknown?

If the device type doesn't match the known cases, the switch falls through to
the default case:

	default:
		dev_err(&client->dev, "unknown RTC type\n");
		goto exit;

At this point, the 'err' variable is 0 because the preceding rs5c_get_regs()
call succeeded. This causes the driver probe to incorrectly return 0 (success)
instead of a negative error code like -ENODEV, leading to an incorrectly
bound but non-functional driver.

While reviewing the above changes, a few other pre-existing issues were
noticed in this file:

[Severity: High]
This is a pre-existing issue, but does rs5c_rtc_alarm_irq_enable() overwrite
hardware state changes with stale cached values?

The function initializes 'buf' from the old cache before fetching the new
state:

	buf = rs5c->regs[RS5C_REG_CTRL1];

	if (!rs5c->has_irq)
		return -EINVAL;

	status = rs5c_get_regs(rs5c);

The rs5c_get_regs() call updates the local cache (rs5c->regs) with the
latest hardware state. However, the function then modifies the stale 'buf'
variable and writes it back to the hardware. Any state changes that occurred
between the previous cache update and the rs5c_get_regs() call could be
silently overwritten with the old values.

[Severity: Medium]
This is a pre-existing issue, but does rs5c_set_alarm() incorrectly sync the
local register cache to an enabled state if the I2C write fails?

When enabling the alarm:

	if (i2c_smbus_write_byte_data(client, addr, buf[0]) < 0)
		dev_warn(dev, "can't enable alarm\n");
	rs5c->regs[RS5C_REG_CTRL1] = buf[0];

If the I2C write fails, the driver logs a warning but proceeds to update the
software cache (rs5c->regs) as if the operation succeeded, returning 0. This
leaves the software state reporting the alarm as active while the hardware
is not actually configured.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825071927.4090460-1-hs@nabladev.com?part=2