Re: [PATCH v2 5/5] rtc: s35390a: Add synology quirk

[sashiko-bot@kernel.org]

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

New issues:
- [High] The return value of s35390a_update_mode() is ignored when disabling INT2 for Synology devices.

Pre-existing issues:
- [High] Software mode cache is eagerly updated before hardware is successfully modified in s35390a_update_mode().
--

--- Patch [5]: [PATCH v2 5/5] rtc: s35390a: Add synology quirk ---
commit 707121acedc466ad23fc787561fac16b4efa8e1f
Author: Markus Probst <markus.probst@posteo.de>

rtc: s35390a: Add synology quirk
    
Synology NAS devices use interrupt signal 1 for wakeup alarms. On ACPI
it is not possible to configure pinctrl. Use interrupt signal 1 for
wakeup if no pinctrl state has been configured in devicetree and dmi
sys vendor matches "Synology Inc.".

> diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
> index fc0dbce0e8f4a..f20b8a5a72dea 100644
> --- a/drivers/rtc/rtc-s35390a.c
> +++ b/drivers/rtc/rtc-s35390a.c

[ ... ]

> @@ -806,7 +807,16 @@ static int s35390a_probe(struct i2c_client *client)
>  
>  	/* If no pinctrl config is defined in DT, fallback to previous behaviour */
>  	if (!s35390a->mode_init) {
> -		err = s35390a_update_mode(s35390a, 1, S35390A_MODE_WAKEUP, 0);
> +		i = 1;
> +		/*
> +		 * Synology uses interrupt signal 1 for wakeup. Since pinctrl cannot be
> +		 * configured on ACPI, we check against the dmi sys vendor.
> +		 */
> +		if (dmi_match(DMI_SYS_VENDOR, "Synology Inc.")) {
> +			i = 0;
> +			s35390a_update_mode(s35390a, 1, S35390A_MODE_DISABLE, 0);

[Severity: High]
Does this code ignore the return value of s35390a_update_mode() when
disabling INT2 for Synology devices?

If the I2C transaction fails during driver probe, such as due to a bus
error, the hardware configuration failure is silently ignored. This could
leave INT2 active when it should be disabled.

This is a pre-existing issue, but updating the software mode cache before
the hardware is successfully modified in s35390a_update_mode() makes
ignoring the return value more problematic:

drivers/rtc/rtc-s35390a.c:s35390a_update_mode() {
    ...
	if (mode == s35390a->mode[pin] && freq == s35390a->freq[pin])
		return 0;

	s35390a->mode[pin] = mode;
	s35390a->freq[pin] = freq;

	if (mode == S35390A_MODE_IGNORE)
		return 0;

	err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS2, &buf, 1);
    ...
}

Because the internal software cache retains the new target mode even if the
I2C communication fails later in the function, subsequent requests to set
the same mode will silently return success without communicating with the
hardware.

Since the error is ignored in s35390a_probe(), if the initial disable
fails, the software cache becomes permanently out of sync with the
hardware, making any subsequent attempt to disable INT2 silently fail.

> +		}
> +		err = s35390a_update_mode(s35390a, i, S35390A_MODE_WAKEUP, 0);
>  		if (err)
>  			return err;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260801-rtc_s35390a_int1-v2-0-f10c99ad1d6c@posteo.de?part=5