Re: [PATCH] rtc: rv3028: configure backup switch mode from device tree

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

New issues:
- [Critical] Removal of the `RTC_PARAM_BACKUP_SWITCH_MODE` userspace parameter breaks the userspace ABI.
- [Medium] Introduction of an undocumented, un-prefixed, and hardware-specific DT property `backup-switch-mode`.

Pre-existing issues:
- [High] The clock provider registered during probe is never removed, leaving a dangling pointer in the OF clock provider list when the driver is unbound.
--

--- Patch [1]: [PATCH] rtc: rv3028: configure backup switch mode from device tree ---
commit 55d9a6511b8e26eb9e90d9e9a4a3d1d04b2c0f1b
Author: Dhaval Shiroya <[email protected]>

rtc: rv3028: configure backup switch mode from device tree
    
Enable and set the backup switch mode directly from the device tree at
probe, instead of requiring a userspace RTC_PARAM_BACKUP_SWITCH_MODE
ioctl on every unit.

> diff --git a/drivers/rtc/rtc-rv3028.c b/drivers/rtc/rtc-rv3028.c
> index d96f6bb68850d..5dde1aed76dd7 100644
> --- a/drivers/rtc/rtc-rv3028.c
> +++ b/drivers/rtc/rtc-rv3028.c
[ ... ]
> @@ -852,8 +789,6 @@ static const struct rtc_class_ops rv3028_rtc_ops = {
>  	.read_offset = rv3028_read_offset,
>  	.set_offset = rv3028_set_offset,
>  	.ioctl = rv3028_ioctl,
> -	.param_get = rv3028_param_get,
> -	.param_set = rv3028_param_set,
>  };

[Severity: Critical]
Does removing the param_get and param_set callbacks break existing userspace
ABI?

Existing applications or scripts (like hwclock) that rely on querying or
modifying the backup switchover mode via the RTC_PARAM_GET and RTC_PARAM_SET
ioctls will encounter failures, as the API has been entirely removed from the
driver.

[ ... ]
> @@ -920,6 +855,65 @@ static u8 rv3028_set_trickle_charger(struct rv3028_data *rv3028,
[ ... ]
> +static int rv3028_set_bsm_from_dt(struct rv3028_data *rv3028,
> +				  struct i2c_client *client)
> +{
> +	u32 val_old, bsm_dt, bsm_bits;
> +	int ret;
> +
> +	if (device_property_read_u32(&client->dev, "backup-switch-mode",
> +				     &bsm_dt))
> +		return 0;

[Severity: Medium]
Is this new device tree property documented in the binding schema?

Adding an undocumented, un-prefixed property that requires raw
hardware-specific bit values (0, 1, or 3) instead of a standard
vendor-prefixed property (e.g., "microcrystal,backup-switch-mode") will
trigger warnings during dtbs_check. 

The device tree schema likely enforces unevaluatedProperties: false, which
will flag this missing schema definition as an invalid configuration.

[ ... ]
> @@ -1005,11 +999,14 @@ static int rv3028_probe(struct i2c_client *client)
[ ... ]
> -	set_bit(RTC_FEATURE_BACKUP_SWITCH_MODE, rv3028->rtc->features);

[Severity: Critical]
Does removing this capability flag cause the kernel to reject userspace
attempts to configure the parameter?

This reinforces the ABI break mentioned above, preventing any userspace
process with access to the RTC device node from interacting with the backup
switchover mode.


While reviewing this patch, I noticed a pre-existing issue in the surrounding
clock registration code:

drivers/rtc/rtc-rv3028.c:rv3028_clkout_register_clk() {
    ...
    of_clk_add_provider(node, of_clk_src_simple_get, clk);
    ...
}

[Severity: High]
This is a pre-existing issue, but does this leave a dangling pointer in the
OF clock provider list when the driver is unbound?

The clock object is allocated and registered using devm_clk_register(), meaning
devres automatically frees the clk structure when the device is unbound.
However, of_clk_add_provider() is a non-managed API that registers the clock
provider globally. 

Because this driver lacks a .remove callback to unregister the provider, a
subsequent clock lookup after unbind could attempt to use the freed structure,
leading to a use-after-free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1