| Message ID | 20250109-enable-rtc-v3-3-f003e8144419@baylibre.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enable RTC for the MT6357 | expand |
On 11/04/2025 14:35:56+0200, Alexandre Mergnat wrote: > The RTC subsystem was experiencing comparison issues between signed and > unsigned time values. When comparing time64_t variables (signed) with > potentially unsigned range values, incorrect results could occur leading > to runtime errors. > > Adds explicit type casts to time64_t for critical RTC time comparisons > in both class.c and interface.c files. The changes ensure proper > handling of negative time values during range validation and offset > calculations, particularly when dealing with timestamps before 1970. > > The previous implementation might incorrectly interpret negative values > as extremely large positive values, causing unexpected behavior in the > RTC hardware abstraction logic. > range_max is explicitly unsigned, casting it to a signed value will break drivers. > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> > --- > drivers/rtc/class.c | 6 +++--- > drivers/rtc/interface.c | 8 ++++---- > 2 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c > index e31fa0ad127e9..1ee3f609f92ea 100644 > --- a/drivers/rtc/class.c > +++ b/drivers/rtc/class.c > @@ -282,7 +282,7 @@ static void rtc_device_get_offset(struct rtc_device *rtc) > * then we can not expand the RTC range by adding or subtracting one > * offset. > */ > - if (rtc->range_min == rtc->range_max) > + if (rtc->range_min == (time64_t)rtc->range_max) > return; > > ret = device_property_read_u32(rtc->dev.parent, "start-year", > @@ -299,7 +299,7 @@ static void rtc_device_get_offset(struct rtc_device *rtc) > if (!rtc->set_start_time) > return; > > - range_secs = rtc->range_max - rtc->range_min + 1; > + range_secs = (time64_t)rtc->range_max - rtc->range_min + 1; > > /* > * If the start_secs is larger than the maximum seconds (rtc->range_max) > @@ -327,7 +327,7 @@ static void rtc_device_get_offset(struct rtc_device *rtc) > * > * Otherwise the offset seconds should be 0. > */ > - if (rtc->start_secs > rtc->range_max || > + if (rtc->start_secs > (time64_t)rtc->range_max || > rtc->start_secs + range_secs - 1 < rtc->range_min) > rtc->offset_secs = rtc->start_secs - rtc->range_min; > else if (rtc->start_secs > rtc->range_min) > diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c > index aaf76406cd7d7..93bdf06807f23 100644 > --- a/drivers/rtc/interface.c > +++ b/drivers/rtc/interface.c > @@ -37,7 +37,7 @@ static void rtc_add_offset(struct rtc_device *rtc, struct rtc_time *tm) > */ > if ((rtc->start_secs > rtc->range_min && secs >= rtc->start_secs) || > (rtc->start_secs < rtc->range_min && > - secs <= (rtc->start_secs + rtc->range_max - rtc->range_min))) > + secs <= (time64_t)(rtc->start_secs + rtc->range_max - rtc->range_min))) > return; > > rtc_time64_to_tm(secs + rtc->offset_secs, tm); > @@ -58,7 +58,7 @@ static void rtc_subtract_offset(struct rtc_device *rtc, struct rtc_time *tm) > * device. Otherwise we need to subtract the offset to make the time > * values are valid for RTC hardware device. > */ > - if (secs >= rtc->range_min && secs <= rtc->range_max) > + if (secs >= rtc->range_min && secs <= (time64_t)rtc->range_max) > return; > > rtc_time64_to_tm(secs - rtc->offset_secs, tm); > @@ -66,7 +66,7 @@ static void rtc_subtract_offset(struct rtc_device *rtc, struct rtc_time *tm) > > static int rtc_valid_range(struct rtc_device *rtc, struct rtc_time *tm) > { > - if (rtc->range_min != rtc->range_max) { > + if (rtc->range_min != (time64_t)rtc->range_max) { > time64_t time = rtc_tm_to_time64(tm); > time64_t range_min = rtc->set_start_time ? rtc->start_secs : > rtc->range_min; > @@ -74,7 +74,7 @@ static int rtc_valid_range(struct rtc_device *rtc, struct rtc_time *tm) > (rtc->start_secs + rtc->range_max - rtc->range_min) : > rtc->range_max; > > - if (time < range_min || time > range_max) > + if (time < range_min || time > (time64_t)range_max) > return -ERANGE; > } > > > -- > 2.25.1 >
On 11/04/2025 15:38, Alexandre Belloni wrote: > On 11/04/2025 14:35:56+0200, Alexandre Mergnat wrote: >> The RTC subsystem was experiencing comparison issues between signed and >> unsigned time values. When comparing time64_t variables (signed) with >> potentially unsigned range values, incorrect results could occur leading >> to runtime errors. >> >> Adds explicit type casts to time64_t for critical RTC time comparisons >> in both class.c and interface.c files. The changes ensure proper >> handling of negative time values during range validation and offset >> calculations, particularly when dealing with timestamps before 1970. >> >> The previous implementation might incorrectly interpret negative values >> as extremely large positive values, causing unexpected behavior in the >> RTC hardware abstraction logic. >> > range_max is explicitly unsigned, casting it to a signed value will > break drivers. Ok, It should be fine for all drivers using range_max = U32_MAX RTC_TIMESTAMP_END_2099 RTC_TIMESTAMP_END_9999 (1 << 14) * 86400ULL - 1 Whereas drivers using range_max = U64_MAX going in trouble: rtc-goldfish.c rtc-ps3.c rtc-st-lpc.c rtc-sun4v.c Is it ok for you if I fix the drivers to avoid issue with signed range_max ? Because, at the end, you can't keep comparison operations between signed and unsigned variable, it lead to future issues. Otherwise, I've another working implementation which remove all comparison operation and drivers doesn't require to be modify.
Hello Alex, On Fri, Apr 11, 2025 at 02:35:56PM +0200, Alexandre Mergnat wrote: > The RTC subsystem was experiencing comparison issues between signed and > unsigned time values. When comparing time64_t variables (signed) with > potentially unsigned range values, incorrect results could occur leading > to runtime errors. > > Adds explicit type casts to time64_t for critical RTC time comparisons > in both class.c and interface.c files. The changes ensure proper > handling of negative time values during range validation and offset > calculations, particularly when dealing with timestamps before 1970. > > The previous implementation might incorrectly interpret negative values > as extremely large positive values, causing unexpected behavior in the > RTC hardware abstraction logic. > > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> > --- > drivers/rtc/class.c | 6 +++--- > drivers/rtc/interface.c | 8 ++++---- > 2 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c > index e31fa0ad127e9..1ee3f609f92ea 100644 > --- a/drivers/rtc/class.c > +++ b/drivers/rtc/class.c > @@ -282,7 +282,7 @@ static void rtc_device_get_offset(struct rtc_device *rtc) > * then we can not expand the RTC range by adding or subtracting one > * offset. > */ > - if (rtc->range_min == rtc->range_max) > + if (rtc->range_min == (time64_t)rtc->range_max) > return; For which values of range_min and range_max does this change result in a different semantic? Trying to answer that question myself I wrote two functions: #include <stdint.h> int compare_unsigned(uint64_t a, int64_t b) { return a == b; } int compare_signed(uint64_t a, int64_t b) { return (int64_t)a == b; } When I compile this (with gcc -Os) the assembly for both functions is the same (tested for x86_64 and arm32). > ret = device_property_read_u32(rtc->dev.parent, "start-year", > @@ -299,7 +299,7 @@ static void rtc_device_get_offset(struct rtc_device *rtc) > if (!rtc->set_start_time) > return; > > - range_secs = rtc->range_max - rtc->range_min + 1; > + range_secs = (time64_t)rtc->range_max - rtc->range_min + 1; In the case where no overflow (or underflow) happens, the result is the same, isn't it? If there is an overflow, the unsigned variant is probably the better choice because overflow for signed variables is undefined behaviour (UB). Respective demo program looks as follows: #include <stdint.h> int test_unsigned(uint64_t a) { return a + 3 > a; } int test_signed(int64_t a) { return a + 3 > a; } Using again `gcc -Os`, the signed variant is compiled to a function that returns true unconditionally while the unsigned one implements the expected semantic. > /* > * If the start_secs is larger than the maximum seconds (rtc->range_max) > @@ -327,7 +327,7 @@ static void rtc_device_get_offset(struct rtc_device *rtc) > * > * Otherwise the offset seconds should be 0. > */ > - if (rtc->start_secs > rtc->range_max || The original comparison uses unsigned semantics. With start_secs signed and range_max unsigned, this might become true if start_secs is less than 0. > + if (rtc->start_secs > (time64_t)rtc->range_max || This new comparison has a similar problem: If range_max is bigger than INT64_MAX, its value interpreted as signed 64bit integer might be a negative number and so this comparison might become true unexpectedly. So even if UB doesn't play a role here (I'm not sure), it's not clear to me why you consider the issue of the unsigned comparison worse than the signed one. If this is indeed beneficial, it needs a better explanation than "When comparing time64_t variables (signed) with potentially unsigned range values, incorrect results could occur leading to runtime errors.". Maybe you have to replace rtc->start_secs > rtc->range_max by: rtc->start_secs >= 0 && rtc->start_secs > rtc->range_max instead? > rtc->start_secs + range_secs - 1 < rtc->range_min) > rtc->offset_secs = rtc->start_secs - rtc->range_min; > else if (rtc->start_secs > rtc->range_min) I didn't check the other hunks. All in all I would suggest to split this series in two: - Adding support for mt6357 in the rtc-mt6359 driver - Fixing overflow issues in the rtc core Given that I don't understand the intend of this patch, I cannot say if it should be included in the 2nd series, or if this is yet another standalone topic. Best regards Uwe
diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c index e31fa0ad127e9..1ee3f609f92ea 100644 --- a/drivers/rtc/class.c +++ b/drivers/rtc/class.c @@ -282,7 +282,7 @@ static void rtc_device_get_offset(struct rtc_device *rtc) * then we can not expand the RTC range by adding or subtracting one * offset. */ - if (rtc->range_min == rtc->range_max) + if (rtc->range_min == (time64_t)rtc->range_max) return; ret = device_property_read_u32(rtc->dev.parent, "start-year", @@ -299,7 +299,7 @@ static void rtc_device_get_offset(struct rtc_device *rtc) if (!rtc->set_start_time) return; - range_secs = rtc->range_max - rtc->range_min + 1; + range_secs = (time64_t)rtc->range_max - rtc->range_min + 1; /* * If the start_secs is larger than the maximum seconds (rtc->range_max) @@ -327,7 +327,7 @@ static void rtc_device_get_offset(struct rtc_device *rtc) * * Otherwise the offset seconds should be 0. */ - if (rtc->start_secs > rtc->range_max || + if (rtc->start_secs > (time64_t)rtc->range_max || rtc->start_secs + range_secs - 1 < rtc->range_min) rtc->offset_secs = rtc->start_secs - rtc->range_min; else if (rtc->start_secs > rtc->range_min) diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c index aaf76406cd7d7..93bdf06807f23 100644 --- a/drivers/rtc/interface.c +++ b/drivers/rtc/interface.c @@ -37,7 +37,7 @@ static void rtc_add_offset(struct rtc_device *rtc, struct rtc_time *tm) */ if ((rtc->start_secs > rtc->range_min && secs >= rtc->start_secs) || (rtc->start_secs < rtc->range_min && - secs <= (rtc->start_secs + rtc->range_max - rtc->range_min))) + secs <= (time64_t)(rtc->start_secs + rtc->range_max - rtc->range_min))) return; rtc_time64_to_tm(secs + rtc->offset_secs, tm); @@ -58,7 +58,7 @@ static void rtc_subtract_offset(struct rtc_device *rtc, struct rtc_time *tm) * device. Otherwise we need to subtract the offset to make the time * values are valid for RTC hardware device. */ - if (secs >= rtc->range_min && secs <= rtc->range_max) + if (secs >= rtc->range_min && secs <= (time64_t)rtc->range_max) return; rtc_time64_to_tm(secs - rtc->offset_secs, tm); @@ -66,7 +66,7 @@ static void rtc_subtract_offset(struct rtc_device *rtc, struct rtc_time *tm) static int rtc_valid_range(struct rtc_device *rtc, struct rtc_time *tm) { - if (rtc->range_min != rtc->range_max) { + if (rtc->range_min != (time64_t)rtc->range_max) { time64_t time = rtc_tm_to_time64(tm); time64_t range_min = rtc->set_start_time ? rtc->start_secs : rtc->range_min; @@ -74,7 +74,7 @@ static int rtc_valid_range(struct rtc_device *rtc, struct rtc_time *tm) (rtc->start_secs + rtc->range_max - rtc->range_min) : rtc->range_max; - if (time < range_min || time > range_max) + if (time < range_min || time > (time64_t)range_max) return -ERANGE; }
The RTC subsystem was experiencing comparison issues between signed and unsigned time values. When comparing time64_t variables (signed) with potentially unsigned range values, incorrect results could occur leading to runtime errors. Adds explicit type casts to time64_t for critical RTC time comparisons in both class.c and interface.c files. The changes ensure proper handling of negative time values during range validation and offset calculations, particularly when dealing with timestamps before 1970. The previous implementation might incorrectly interpret negative values as extremely large positive values, causing unexpected behavior in the RTC hardware abstraction logic. Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> --- drivers/rtc/class.c | 6 +++--- drivers/rtc/interface.c | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-)