Re: [PATCH v6 2/6] rtc: s35390a: Add missing newline to dev_err

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Probe clears power-on clear (POC) and battery low (BLD) flags without acting on them, causing silent data corruption.
- [High] RTC_VL_CLR ioctl performs a full hardware reset, destructively erasing the RTC time.
--

--- Patch [2]: [PATCH v6 2/6] rtc: s35390a: Add missing newline to dev_err ---
commit f317b10535ad5028ddd5cbe785ad735c87f43819
Author: Markus Probst <markus.probst@posteo.de>

rtc: s35390a: Add missing newline to dev_err

This commit adds a missing newline character to a dev_err() message in the
probe function. It fixes the log output format when disabling the alarm fails.

> diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
> index b72eef4fb0997..4cfe7034c5165 100644
> --- a/drivers/rtc/rtc-s35390a.c
> +++ b/drivers/rtc/rtc-s35390a.c
> @@ -494,7 +494,7 @@ static int s35390a_probe(struct i2c_client *client)
>  		buf = 0;
>  		err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &buf, 1);
>  		if (err < 0) {
> -			dev_err(dev, "error disabling alarm");
> +			dev_err(dev, "error disabling alarm\n");
>  			return err;
>  		}
>  	} else {

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential problem
in how the probe function handles power-loss flags:

drivers/rtc/rtc-s35390a.c:s35390a_probe() {
[ ... ]
	err_read = s35390a_read_status(s35390a, &status1);
	if (err_read < 0) {
		dev_err(dev, "error resetting chip\n");
		return err_read;
	}
[ ... ]
}

Does the driver silently discard the power-loss or battery-low indicators?

When the RTC loses power, reading STATUS1 clears the hardware flags, but
s35390a_read_status() returns 1 to indicate the invalid state. Since this code
doesn't check if err_read == 1, won't it just continue and serve garbage time
data to userspace as if it were valid?

[Severity: High]
This is a pre-existing issue, but there is also a destructive behavior in the
ioctl handler:

drivers/rtc/rtc-s35390a.c:s35390a_rtc_ioctl() {
[ ... ]
	case RTC_VL_CLR:
		/* update flag and clear register */
		err = s35390a_init(s35390a);
		if (err < 0)
			return err;
		break;
[ ... ]
}

Could this ioctl destructively erase the configured RTC time?

Calling s35390a_init() writes S35390A_FLAG_RESET, which resets the chip and
erases the currently programmed time. Since RTC_VL_CLR is only meant to clear
low-voltage warnings, is it safe to let this UAPI call wipe the main time data?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824-rtc_s35390a_int1-v6-0-df96569e3412@posteo.de?part=2