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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] rtc: dt binding violation by reading property from parent node
- [Low] firmware: global device pointer race and leak on multiple probes
--

commit f13bdcb7308fe3b9ecc514f418f2d57fa4e0247f
Author: Sander Speetjens <sander.speetjens@gmail.com>
Subject: rtc: Add Raspberry Pi 5 RTC driver

This upstreaming patch introduces support for the downstream Raspberry Pi 5
RTC driver. The driver enables support for the custom DA9091 RTC, which is
accessed through the firmware mailbox.

> diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
> index 0aa322e9a2e73..a35f00c6fb2c1 100644
> --- a/drivers/firmware/raspberrypi.c
> +++ b/drivers/firmware/raspberrypi.c
[ ... ]
> @@ -231,6 +232,27 @@ static void rpi_register_clk_driver(struct device *dev)
[ ... ]
> +static void rpi_register_rtc_driver(struct device *dev)
> +{
[ ... ]
> +	rpi_rtc = platform_device_register_data(dev, "raspberrypi-rtc",
> +						-1, NULL, 0);
> +}

[Severity: Low]
Does locklessly overwriting the global rpi_rtc pointer here risk a resource
leak? If multiple probes occur (for example, via a custom Device Tree
overlay), it appears the initially registered platform device could be
permanently leaked since the pointer is overwritten without synchronization.

[ ... ]
> @@ -327,6 +350,8 @@ static void rpi_firmware_remove(struct platform_device *pdev)
[ ... ]
>  	platform_device_unregister(rpi_clk);
>  	rpi_clk = NULL;
> +	platform_device_unregister(rpi_rtc);
> +	rpi_rtc = NULL;
>  
>  	rpi_firmware_put(fw);
>  }

[Severity: Low]
If a secondary probe occurred and overwrote the global pointer as noted above,
could this cross-unregister the wrong device during removal in
rpi_firmware_remove(), leading to a double-unregister of NULL and a leaked
device?

> diff --git a/drivers/rtc/rtc-raspberrypi.c b/drivers/rtc/rtc-raspberrypi.c
> new file mode 100644
> index 0000000000000..494b949374064
> --- /dev/null
> +++ b/drivers/rtc/rtc-raspberrypi.c
[ ... ]
> +static int rpi_rtc_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	rpi_rtc_alarm_clear_pending(dev);
> +
> +	vrtc->bbat_vchg_millivolts = 0;
> +	of_property_read_u32(dev->parent->of_node, "trickle-voltage-millivolt",
> +			     &vrtc->bbat_vchg_millivolts);

[Severity: Medium]
Is it intentional to unconditionally read the trickle-voltage-millivolt
property from the parent MFD firmware node instead of the driver's own RTC
subnode?

If this property is placed correctly in the raspberrypi,firmware-rtc subnode,
it seems it would be silently ignored, causing trickle charging to be disabled
and breaking standard Device Tree encapsulation.

> +
> +	rpi_rtc_set_limits(dev);
> +	rpi_rtc_set_charge_voltage(dev);
> +
> +	return devm_rtc_register_device(vrtc->rtc);
> +}

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