Re: [PATCH 2/3] rtc: Add Raspberry Pi 5 RTC driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The device tree node reference acquired via of_parse_phandle() is never released.
- [High] The firmware reference is leaked on probe errors and during driver unbind.
- [High] The driver leaks the wakeup source allocated by device_init_wakeup() on error paths and module unload.
- [Medium] The driver truncates 64-bit timestamps to 32 bits and fails to configure RTC range bounds.
--

commit 3148e2d83cccdbeec926774910d9505646955095
Author: Jonathan Bell <jonathan@raspberrypi.com>

rtc: Add Raspberry Pi 5 RTC driver

Upstreaming the downstream Raspberry Pi 5 RTC driver.
This driver supports the custom DA9091, which is accessed through the firmware
mailbox.

> diff --git a/drivers/rtc/rtc-rpi.c b/drivers/rtc/rtc-rpi.c
> new file mode 100644
> index 0000000000000..e455b4116957d
> --- /dev/null
> +++ b/drivers/rtc/rtc-rpi.c
[ ... ]
> +static int rpi_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
> +	u32 data[2] = {RTC_TIME, rtc_tm_to_time64(tm)};

[Severity: Medium]
Does this code silently truncate the 64-bit timestamp when it assigns the 
return value of rtc_tm_to_time64() to a u32 array element?

Since rtc->range_max is left unconfigured (defaulting to 0), the RTC core 
does not enforce bounds checks. Thus, if a user attempts to set a date beyond 
year 2106, the timestamp will be truncated here, leading to silent hardware 
time corruption.

[ ... ]
> +static int rpi_rtc_probe(struct platform_device *pdev)
> +{
> +	struct rpi_rtc_data *vrtc;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct device_node *fw_node;
> +	struct rpi_firmware *fw;
> +	int ret;
> +
> +	fw_node = of_parse_phandle(np, "firmware", 0);
> +	if (!fw_node) {
> +		dev_err(dev, "Missing firmware node\n");
> +		return -ENOENT;
> +	}

[Severity: High]
Can this of_parse_phandle() call leak the device tree node reference? It 
acquires a reference to fw_node unconditionally, but I do not see a matching 
of_node_put(fw_node) in this function to release it once it is no longer 
needed.

> +
> +	fw = rpi_firmware_get(fw_node);
> +	if (!fw)
> +		return -EPROBE_DEFER;

[Severity: High]
Does this leak the firmware reference if the driver fails probe later, or is 
subsequently unbound? 

The rpi_firmware_get() function increments a reference count on the firmware 
object. Since there is no cleanup on error paths, nor a devm_rpi_firmware_get() 
variant used here, the resource appears to be permanently leaked when probe 
fails.

> +
> +	vrtc = devm_kzalloc(&pdev->dev, sizeof(*vrtc), GFP_KERNEL);
> +	if (!vrtc)
> +		return -ENOMEM;
> +
> +	vrtc->fw = fw;
> +
> +	device_init_wakeup(&pdev->dev, 1);

[Severity: High]
Could this leak the wakeup source allocated by device_init_wakeup() on error 
paths and during module unload? 

Because device_init_wakeup(&pdev->dev, 1) allocates a wakeup source, failing to 
call device_init_wakeup(&pdev->dev, 0) during probe failures leaves the wakeup 
source dangling.

> +
> +	platform_set_drvdata(pdev, vrtc);
> +
> +	vrtc->rtc = devm_rtc_allocate_device(&pdev->dev);
> +	if (IS_ERR(vrtc->rtc))
> +		return PTR_ERR(vrtc->rtc);
[ ... ]
> +	vrtc->rtc->ops = &rpi_rtc_ops;

[Severity: Medium]
Should rtc->range_max be properly configured before device registration? 
Without configuring the bounds, the RTC subsystem permits out-of-range times 
that will trigger the truncation issue noted earlier in rpi_rtc_set_time().

[ ... ]
> +static struct platform_driver rpi_rtc_driver = {
> +	.probe = rpi_rtc_probe,
> +	.driver = {
> +		.name = "rpi-rtc",
> +		.of_match_table = rpi_rtc_dt_match,
> +	},
> +};

[Severity: High]
Is a .remove callback missing here to handle driver unbinds? Without it, the 
firmware reference acquired by rpi_firmware_get() and the wakeup source 
allocated by device_init_wakeup() will be leaked whenever the driver is unbound 
by a local administrator.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909141300.1368790-1-sander.speetjens@gmail.com?part=2