First of all, thanks for the code review.
On 11/17/24 9:04 PM, Sergey Bugaev wrote:
On Sun, Nov 17, 2024 at 3:34 PM Zhaoming Luo <zhming...@163.com> wrote:
diff --git a/hurd/rtc.h b/hurd/rtc.h
new file mode 100644
index 00000000..6516e86c
--- /dev/null
+++ b/hurd/rtc.h
+struct rtc_time {
+ int tm_sec;
+ int tm_min;
+ int tm_hour;
+ int tm_mday;
+ int tm_mon;
+ int tm_year;
+ int tm_wday;
+ int tm_yday;
+ int tm_isdst;
+};
Nitpick: please see how struct definitions should be formatted
according to the GNU code style [0].
OK
[0]: https://www.gnu.org/prep/standards/html_node/Formatting.html#Formatting
diff --git a/rtc/main.c b/rtc/main.c
new file mode 100644
index 00000000..daefb95d
--- /dev/null
+++ b/rtc/main.c
+static const struct argp rtc_argp =
+{ options, parse_opt, 0, "RTC device" };
This would probably be a good place to spell out that RTC stands for
real time clock?
Indeed, "Real-Time Clock device" is employed.
+int
+main (int argc, char **argv)
+{
+ error_t err;
+ mach_port_t bootstrap;
+
+ argp_parse (&rtc_argp, argc, argv, 0, 0, 0);
+
+ task_get_bootstrap_port (mach_task_self (), &bootstrap);
+ if (bootstrap == MACH_PORT_NULL)
+ error (1, 0, "Must be started as a translator");
+
+ /* Reply to our parent */
+ err = trivfs_startup (bootstrap, O_NORW, NULL, NULL, NULL, NULL, &rtccntl);
+ mach_port_deallocate (mach_task_self (), bootstrap);
+ if (err)
+ error (1, err, "trivfs_startup failed");
+
+ /* Request for permission to do i/o on port numbers 0x70 and 0x71 for
+ accessing RTC registers. */
+ err = ioperm (0x70, 2, true);
+ if (err)
+ error (1, err, "Request IO permission failed");
It's probably a good idea to do this before you reply to the parent,
so you don't end up saying "I'm ready!" and then immediately exit with
an error.
OK. I think it can explain why when I settrans rtc to /tmp/rtc without
sudo will cause `ls /tmp/` hanging. After I swap the position of "Replay
to our parent" and "Request for permission", `ls /tmp/` does not hang,
instead ls gives me "cannot access '/tmp/rtc': Translator died" error.
Interesting.
+error_t
+trivfs_goaway (struct trivfs_control *fsys, int flags)
+{
+ exit (0);
+}
Do you need to do anything to the RTC hardware before you exit to
leave it in a good/well-defined state? Especially if other threads are
accessing it at the same time? (Perhaps not, I don't know.)
I don't think I need to do anything before exit. However, maybe I need
to ensure the rtc operations is thread-safe; I haven't done it. It seems
util-linux tried to implement thread-safe functionality[0], but it seems
they didn't do it right[1].
[0]:https://github.com/util-linux/util-linux/blob/master/sys-utils/hwclock-cmos.c#L207-L215
[1]:https://github.com/util-linux/util-linux/blob/master/sys-utils/hwclock-cmos.c#L90-L94
I will do some experiments to see whether two threads setting rtc causes
data race before implement thread-safe functionality.
diff --git a/rtc/pioctl-ops.c b/rtc/pioctl-ops.c
new file mode 100644
index 00000000..c33403c8
--- /dev/null
+++ b/rtc/pioctl-ops.c
@@ -0,0 +1,223 @@
+/* Server side implementation for rtc server
+
+ Copyright (C) 2024 Free Software Foundation, Inc.
+
+ This file is part of the GNU Hurd.
+
+ The GNU Hurd is free software; you can redistribute it and/or
+ modify it under the terms of the GNU General Public License as
+ published by the Free Software Foundation; either version 2, or (at
+ your option) any later version.
+
+ The GNU Hurd is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program; if not, write to the Free Software
+ Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111, USA. */
+
+/* This implementation is largely based on sys-utils/hwclock-cmos.c from
+ util-linux */
+
+/* A struct tm has int fields (it is defined in POSIX)
+ tm_sec 0-59, 60 or 61 only for leap seconds
+ tm_min 0-59
+ tm_hour 0-23
+ tm_mday 1-31
+ tm_mon 0-11
+ tm_year number of years since 1900
+ tm_wday 0-6, 0=Sunday
+ tm_yday 0-365
+ tm_isdst >0: yes, 0: no, <0: unknown */
+
+#include "rtc_pioctl_S.h"
+#include <hurd/rtc.h>
+#include <hurd/hurd_types.h>
+#include <sys/io.h>
+#include <stdbool.h>
+
+/* Conversions to and from RTC internal format */
+#define BCD_TO_BIN(val) ((val)=((val)&15) + ((val)>>4)*10)
+#define BIN_TO_BCD(val) ((val)=(((val)/10)<<4) + (val)%10)
+
+/* POSIX uses 1900 as epoch for a struct tm, and 1970 for a time_t. */
+#define TM_EPOCH 1900
+
+static unsigned short clock_ctl_addr = 0x70;
+static unsigned short clock_data_addr = 0x71;
These should be #define's (or enum members if you prefer), rather than
mutable runtime memory in .data, no?
#define is employed now.
+static inline unsigned long cmos_read(unsigned long reg)
+{
+ outb_p(reg, clock_ctl_addr);
+ return inb_p(clock_data_addr);
+}
GNU style please:
OK.
static inline unsigned long
cmos_read (unsigned long reg)
{
outb_p (reg, clock_ctl_addr);
return inb_p (clock_data_addr);
}
also I believe outb_p's first argument is an 'unsigned char', so why
unsigned long here?
The unsigned long is used because the source code of util-linux uses it
and I just copied it.
The unsigned char is employed.
+
+static inline void cmos_write(unsigned long reg, unsigned long val)
+{
+ outb(reg, clock_ctl_addr);
+ outb(val, clock_data_addr);
+}
+
+
+static inline int cmos_clock_busy(void)
+{
+ return
+ /* poll bit 7 (UIP) of Control Register A */
+ (cmos_read(10) & 0x80);
You could perhaps write it as (1 << 7) instead of 0x80 then, not that
it matters.
Then 0x80 is kept for code style consistency.
+}
+
+
+/* 9 RTC_RD_TIME -- Read RTC time */
+kern_return_t
+rtc_S_pioctl_rtc_rd_time (struct trivfs_protid *cred, struct rtc_time *tm)
+{
+ if (!cred)
+ return EOPNOTSUPP;
+ if (!(cred->po->openmodes & O_READ))
+ return EBADF;
+
+ unsigned char status = 0;
+ unsigned char pmbit = 0;
+
+ /* When we wait for 100 ms (it takes too long), we exit with error */
+ int time_passed_in_milliseconds = 0;
+ bool read_rtc_successfully = false;
+ while (time_passed_in_milliseconds < 100) {
+ if (!cmos_clock_busy()) {
+ tm->tm_sec = cmos_read(0);
+ tm->tm_min = cmos_read(2);
+ tm->tm_hour = cmos_read(4);
+ tm->tm_wday = cmos_read(6);
+ tm->tm_mday = cmos_read(7);
+ tm->tm_mon = cmos_read(8);
+ tm->tm_year = cmos_read(9);
+ status = cmos_read(11);
+ /* Unless the clock changed while we were reading, consider this
+ a good clock read. */
+ if (tm->tm_sec == cmos_read(0)) {
+ read_rtc_successfully = true;
+ break;
+ }
+ }
GNU code style nitpicks again:
* all local variables should be declared before the actual statements
(like the cred checks),
* braces placement,
* single space between the function name and the opening paren.
Modified to follow GNU code style.
(and same for all of the code below)
+ usleep (1000);
+ time_passed_in_milliseconds++;
+ }
+
+ if (!read_rtc_successfully)
+ return EBUSY;
+
+ if (!(status & 0x04)) { /* BCD mode - the default */
+ BCD_TO_BIN(tm->tm_sec);
+ BCD_TO_BIN(tm->tm_min);
+ pmbit = (tm->tm_hour & 0x80);
+ tm->tm_hour &= 0x7f;
+ BCD_TO_BIN(tm->tm_hour);
+ BCD_TO_BIN(tm->tm_wday);
+ BCD_TO_BIN(tm->tm_mday);
+ BCD_TO_BIN(tm->tm_mon);
+ BCD_TO_BIN(tm->tm_year);
+ }
+
+ /* We don't use the century byte of the Hardware Clock since we
+ don't know its address (usually 50 or 55). Here, we follow the
+ advice of the X/Open Base Working Group: "if century is not
+ specified, then values in the range [69-99] refer to years in the
+ twentieth century (1969 to 1999 inclusive), and values in the
+ range [00-68] refer to years in the twenty-first century (2000 to
+ 2068 inclusive)." */
+ tm->tm_wday -= 1;
+ tm->tm_mon -= 1;
+ if (tm->tm_year < 69)
+ tm->tm_year += 100;
+ if (pmbit) {
+ tm->tm_hour += 12;
+ if (tm->tm_hour == 24)
+ tm->tm_hour = 0;
+ }
+
+ /* don't know whether it's daylight */
+ tm->tm_isdst = -1;
+
+ return KERN_SUCCESS;
+}
+
+/* 10 RTC_SET_TIME -- Set RTC time */
+kern_return_t
+rtc_S_pioctl_rtc_set_time (struct trivfs_protid *cred, struct rtc_time tm)
+{
+ if (!cred)
+ return EOPNOTSUPP;
+ if (!(cred->po->openmodes & O_WRITE))
+ return EBADF;
+
+ unsigned char save_control, save_freq_select, pmbit = 0;
+
+ /* CMOS byte 10 (clock status register A) has 3 bitfields:
+ bit 7: 1 if data invalid, update in progress (read-only bit)
+ (this is raised 224 us before the actual update starts)
+ 6-4 select base frequency
+ 010: 32768 Hz time base (default)
+ 111: reset
+ all other combinations are manufacturer-dependent
+ (e.g.: DS1287: 010 = start oscillator, anything else = stop)
+ 3-0 rate selection bits for interrupt
+ 0000 none (may stop RTC)
+ 0001, 0010 give same frequency as 1000, 1001
+ 0011 122 microseconds (minimum, 8192 Hz)
+ .... each increase by 1 halves the frequency, doubles the period
+ 1111 500 milliseconds (maximum, 2 Hz)
+ 0110 976.562 microseconds (default 1024 Hz) */
+
+ save_control = cmos_read(11); /* tell the clock it's being set */
+ cmos_write(11, (save_control | 0x80));
+ save_freq_select = cmos_read(10); /* stop and reset prescaler */
+ cmos_write(10, (save_freq_select | 0x70));
+
+ tm.tm_year %= 100;
+ tm.tm_mon += 1;
+ tm.tm_wday += 1;
+
+ if (!(save_control & 0x02)) { /* 12hr mode; the default is 24hr mode
*/
+ if (tm.tm_hour == 0)
+ tm.tm_hour = 24;
+ if (tm.tm_hour > 12) {
+ tm.tm_hour -= 12;
+ pmbit = 0x80;
+ }
+ }
+
+ if (!(save_control & 0x04)) { /* BCD mode - the default */
+ BIN_TO_BCD(tm.tm_sec);
+ BIN_TO_BCD(tm.tm_min);
+ BIN_TO_BCD(tm.tm_hour);
+ BIN_TO_BCD(tm.tm_wday);
+ BIN_TO_BCD(tm.tm_mday);
+ BIN_TO_BCD(tm.tm_mon);
+ BIN_TO_BCD(tm.tm_year);
+ }
+
+ cmos_write(0, tm.tm_sec);
+ cmos_write(2, tm.tm_min);
+ cmos_write(4, tm.tm_hour | pmbit);
+ cmos_write(6, tm.tm_wday);
+ cmos_write(7, tm.tm_mday);
+ cmos_write(8, tm.tm_mon);
+ cmos_write(9, tm.tm_year);
+
+ /* The Linux kernel sources, linux/arch/i386/kernel/time.c, have the
+ following comment:
+
+ The following flags have to be released exactly in this order,
+ otherwise the DS12887 (popular MC146818A clone with integrated
+ battery and quartz) will not reset the oscillator and will not
+ update precisely 500 ms later. You won't find this mentioned in
+ the Dallas Semiconductor data sheets, but who believes data
+ sheets anyway ... -- Markus Kuhn */
+ cmos_write(11, save_control);
+ cmos_write(10, save_freq_select);
Can these writes somehow fail, and if so, can you detect it and report
the error? (I've no idea.)
I think the only way to check is reading rtc between `cmos_write(9,
tm.tm_year);` and `cmos_write(11, save_control);`; then compare the
values we just read with the values for setting.
I've added implementing this functionality in my TODO list.
--
Zhaoming Luo
