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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] rtc: raspberrypi: Inconsistent and incorrect endianness handling in mailbox payloads
--

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

This commit upstreams the downstream Raspberry Pi 5 RTC driver. It binds a
new rtc-raspberrypi driver to the device via the raspberrypi firmware
driver and accesses the custom DA9091 RTC using the firmware mailbox.

> diff --git a/drivers/rtc/rtc-raspberrypi.c b/drivers/rtc/rtc-raspberrypi.c
> new file mode 100644
> index 0000000000000..53178d1eaf48c
> --- /dev/null
> +++ b/drivers/rtc/rtc-raspberrypi.c
[ ... ]
> +static int rpi_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
> +	u32 data[2] = { RTC_TIME };
> +	int err;
> +
> +	err = rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_GET_RTC_REG,
> +				    &data, sizeof(data));
> +	rtc_time64_to_tm(le32_to_cpu(data[1]), tm);

[Severity: High]
Does passing a u32 into an le32_to_cpu() cast here cause a Sparse warning?
Should data be strictly typed to handle endianness without triggering warnings?

> +	return err;
> +}
> +
> +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, cpu_to_le32(rtc_tm_to_time64(tm)) };

[Severity: High]
Does assigning the __le32 return value of cpu_to_le32() to a u32 array
trigger a Sparse warning? 

Also, is it intentional that the time value is swapped while the register
index (RTC_TIME) is sent in host endian format? This could lead to
inconsistent byte swapping on big-endian architectures.

> +
> +	return rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_SET_RTC_REG,
> +				     &data, sizeof(data));
> +}
> +
> +static int rpi_rtc_alarm_irq_is_enabled(struct device *dev, unsigned char *enabled)
> +{
> +	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
> +	u32 data[2] = { RTC_ALARM_ENABLE };
> +	s32 err = 0;
> +
> +	err = rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_GET_RTC_REG,
> +				    &data, sizeof(data));
> +	*enabled = data[1] & 0x1;

[Severity: High]
Can applying a bitmask directly to the unswapped firmware value evaluate the
wrong byte on big-endian systems?

> +	return err;
> +}
[ ... ]
> +static int rpi_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
> +{
> +	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
> +	u32 data[2] = { RTC_ALARM };
> +	s32 err = 0;
> +
> +	err = rpi_rtc_alarm_irq_is_enabled(dev, &alarm->enabled);
> +	if (!err)
> +		err = rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_GET_RTC_REG,
> +					    &data, sizeof(data));
> +	rtc_time64_to_tm(data[1], &alarm->time);

[Severity: High]
Unlike in rpi_rtc_read_time(), the alarm time read from firmware here is not
byte-swapped. Could this lead to corrupted time parsing on big-endian systems?

> +
> +	return err;
> +}
> +
> +static int rpi_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
> +{
> +	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
> +	u32 data[2] = { RTC_ALARM, rtc_tm_to_time64(&alarm->time) };

[Severity: High]
Neither the register index nor the alarm time payload appears to be
byte-swapped here. Does this need a cpu_to_le32() conversion similar to
rpi_rtc_set_time() to ensure correct behavior on big-endian hardware?

> +	int err;
> +
> +	err = rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_SET_RTC_REG,
> +				    &data, sizeof(data));
[ ... ]
> +static int rpi_rtc_set_charge_voltage(struct device *dev)
> +{
> +	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
> +	u32 data[2] = { RTC_BBAT_CHG_VOLTS, vrtc->bbat_vchg_millivolts * 1000U };

[Severity: High]
The voltage configuration payload is transmitted to the firmware in host-endian
format without swapping. Does this cause the RTC firmware to receive an invalid
charge voltage on big-endian systems?

> +	int err;
> +
> +	if (vrtc->bbat_vchg_millivolts != 0 &&
> +	    (vrtc->bbat_vchg_millivolts < vrtc->bbat_vchg_min_millivolts ||
> +	    vrtc->bbat_vchg_millivolts > vrtc->bbat_vchg_max_millivolts)) {

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