From: Alexandre Belloni [mailto:alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org]
Sent: Mittwoch, 6. Dezember 2017 09:58
On 06/12/2017 at 09:36:18 +0100, Sascha Hauer wrote:

quoted

quoted

+/*
+ * This function updates the RTC alarm registers and then clears all the
+ * interrupt status bits.
+ * The caller should hold the pdata->lock
+ *
+ * @param  alrm         the new alarm value to be updated in the RTC
+ *
+ * @return  0 if successful; non-zero otherwise.
+ */
+static int mxc_rtc_write_alarm_locked(struct mxc_rtc_data *const pdata,
+                                struct rtc_time *alarm_tm)
+{
+  void __iomem *const ioaddr = pdata->ioaddr;
+  unsigned long time;
+
+  rtc_tm_to_time(alarm_tm, &time);
+
+  if (time > U32_MAX) {
+          pr_err("Hopefully I am out of service by then :-(\n");
+          return -EINVAL;
+  }

This will never happen as on your target hardware unsigned long is a
32bit type. Not sure what is best to do here. Maybe you should test
the return value of rtc_tm_to_time. ATM it returns 0 unconditionally,
but rtc_tm_to_time could detect when the input time doesn't fit into
its return type and return an error in this case.
Also I just realized that it's unsigned and only overflows in the year
2106. I'm most likely dead then so I don't care that much ;)

One solution is to use the 64bit version instead so it doesn't overflow.
This makes the time > U32_MAX work.
Also, I'll send (hopefully soon) a series adding proper range checking
for the whole RTC subsystem. And yes, it not urgent as I don't think I
will care so much in 2106 too ;)

I just noticed that in mxc_rtc_set_time() I am using the 64bit version.
After thinking a while about this issue, I think the 64bit version is better
suited for my use case. It makes explicitly clear that I need to push the
time into a 32bit hw register and "unsigned long" is just by accident the
correct size for me.

quoted

quoted

+/*
+ * This function reads the current RTC time into tm in Gregorian date.
+ *
+ * @param  tm           contains the RTC time value upon return
+ *
+ * @return  0 if successful; non-zero otherwise.
+ */
+static int mxc_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+  struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
+  time_t now;
+  int ret = mxc_rtc_lock(pdata);
+
+  if (ret)
+          return ret;
+
+  now = readl(pdata->ioaddr + SRTC_LPSCMR);
+  rtc_time_to_tm(now, tm);
+  ret = rtc_valid_tm(tm);

This check is useless for two reasons: you know that rtc_time_to_tm will
generate a valid tm and the core always checks the tm anyway.

I will remove this with the next version

Thanks for your time and help,
Patrick

Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff
Registered office: Verl, Germany | Register court: Guetersloh HRA 7075



--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html