Date: Wed, 13 Apr 2005 20:12:11 +0200 From: "Poul-Henning Kamp" <phk@phk.freebsd.dk> To: Adam Gregoire <ebola@psychoholics.org> Cc: "Matthew N. Dodd" <mdodd@FreeBSD.org> Subject: Re: cvs commit: src/sys/i386/isa clock.c Message-ID: <21177.1113415931@critter.freebsd.dk> In-Reply-To: Your message of "Wed, 13 Apr 2005 10:18:33 EDT." <1113401913.838.0.camel@S010600deadc0de00.su.shawcable.net>
next in thread | previous in thread | raw e-mail | index | archive | help
Could you do me the favour and run the diff again but with -b so all
the white-space only changes get ignored ?
In message <1113401913.838.0.camel@S010600deadc0de00.su.shawcable.net>, Adam Gregoire writes:
>
>--=-D2UC1HCBeWdBxtbsjtTR
>Content-Type: text/plain
>Content-Transfer-Encoding: 7bit
>
>> Modified files:
>> sys/i386/isa clock.c
>> Log:
>> Replace spl protection in rtcin() and writertc() with spinlocks
>> using the existing clock_lock mutex.
>>
>> Revision Changes Path
>> 1.219 +6 -6 src/sys/i386/isa/clock.c
>
>Hello, I have made a patch that brings these changes into amd64's
>isa/clock.c, and it also contains changes for style consistancy,
>whitespace, and use of prototypes on both amd64 and i386. It should even
>reduce diffs a bit. I have tested this on amd64 kernel as of an hour
>ago. All seems to be fine here.
>
>Hopefully this is deemed useful. If not are you able to just get the
>amd64 changes for locking?
>
>Thank you,
>
>--
>Adam Gregoire <ebola@psychoholics.org>
>
>--=-D2UC1HCBeWdBxtbsjtTR
>Content-Disposition: attachment; filename=clock.diff
>Content-Type: text/x-patch; name=clock.diff; charset=UTF-8
>Content-Transfer-Encoding: 7bit
>
>--- sys/amd64/isa/clock.c.orig Wed Apr 13 01:16:47 2005
>+++ sys/amd64/isa/clock.c Wed Apr 13 01:55:42 2005
>@@ -84,10 +84,10 @@
> * 32-bit time_t's can't reach leap years before 1904 or after 2036, so we
> * can use a simple formula for leap years.
> */
>-#define LEAPYEAR(y) (((u_int)(y) % 4 == 0) ? 1 : 0)
>-#define DAYSPERYEAR (31+28+31+30+31+30+31+31+30+31+30+31)
>+#define LEAPYEAR(y) (((u_int)(y) % 4 == 0) ? 1 : 0)
>+#define DAYSPERYEAR (31+28+31+30+31+30+31+31+30+31+30+31)
>
>-#define TIMER_DIV(x) ((timer_freq + (x) / 2) / (x))
>+#define TIMER_DIV(x) ((timer_freq + (x) / 2) / (x))
>
> int adjkerntz; /* local offset from GMT in seconds */
> int clkintr_pending;
>@@ -96,12 +96,14 @@
> int psdiv = 1;
> int statclock_disable;
> #ifndef TIMER_FREQ
>-#define TIMER_FREQ 1193182
>+#define TIMER_FREQ 1193182
> #endif
> u_int timer_freq = TIMER_FREQ;
> int timer0_max_count;
> int wall_cmos_clock; /* wall CMOS clock assumed if != 0 */
> struct mtx clock_lock;
>+#define RTC_LOCK mtx_lock_spin(&clock_lock)
>+#define RTC_UNLOCK mtx_unlock_spin(&clock_lock)
>
> static int beeping = 0;
> static const u_char daysinmonth[] = {31,28,31,30,31,30,31,31,30,31,30,31};
>@@ -175,7 +177,7 @@
> }
>
> int
>-release_timer2()
>+release_timer2(void)
> {
>
> if (timer2_state != ACQUIRED)
>@@ -228,9 +230,9 @@
> DB_SHOW_COMMAND(rtc, rtc)
> {
> printf("%02x/%02x/%02x %02x:%02x:%02x, A = %02x, B = %02x, C = %02x\n",
>- rtcin(RTC_YEAR), rtcin(RTC_MONTH), rtcin(RTC_DAY),
>- rtcin(RTC_HRS), rtcin(RTC_MIN), rtcin(RTC_SEC),
>- rtcin(RTC_STATUSA), rtcin(RTC_STATUSB), rtcin(RTC_INTR));
>+ rtcin(RTC_YEAR), rtcin(RTC_MONTH), rtcin(RTC_DAY),
>+ rtcin(RTC_HRS), rtcin(RTC_MIN), rtcin(RTC_SEC),
>+ rtcin(RTC_STATUSA), rtcin(RTC_STATUSB), rtcin(RTC_INTR));
> }
> #endif /* DDB */
>
>@@ -323,7 +325,7 @@
> * before the delay is up (unless we're interrupted).
> */
> ticks_left = ((u_int)n * (long long)timer_freq + 999999)
>- / 1000000;
>+ / 1000000;
>
> while (ticks_left > 0) {
> #ifdef KDB
>@@ -356,7 +358,7 @@
> #ifdef DELAYDEBUG
> if (state == 1)
> printf(" %d calls to getit() at %d usec each\n",
>- getit_calls, (n + 5) / getit_calls);
>+ getit_calls, (n + 5) / getit_calls);
> #endif
> }
>
>@@ -398,33 +400,30 @@
> */
>
> int
>-rtcin(reg)
>- int reg;
>+rtcin(int reg)
> {
>- int s;
> u_char val;
>
>- s = splhigh();
>+ RTC_LOCK;
> outb(IO_RTC, reg);
> inb(0x84);
> val = inb(IO_RTC + 1);
> inb(0x84);
>- splx(s);
>+ RTC_UNLOCK;
> return (val);
> }
>
> static __inline void
> writertc(u_char reg, u_char val)
> {
>- int s;
>
>- s = splhigh();
>+ RTC_LOCK;
> inb(0x84);
> outb(IO_RTC, reg);
> inb(0x84);
> outb(IO_RTC + 1, val);
> inb(0x84); /* XXX work around wrong order in rtcin() */
>- splx(s);
>+ RTC_UNLOCK;
> }
>
> static __inline int
>@@ -501,15 +500,14 @@
> goto fail;
> }
>
>- if (bootverbose) {
>+ if (bootverbose)
> printf("i8254 clock: %u Hz\n", tot_count);
>- }
> return (tot_count);
>
> fail:
> if (bootverbose)
> printf("failed, using default i8254 clock of %u Hz\n",
>- timer_freq);
>+ timer_freq);
> return (timer_freq);
> }
>
>@@ -535,7 +533,7 @@
> * XXX initialization of other timers is unintentionally left blank.
> */
> void
>-startrtclock()
>+startrtclock(void)
> {
> u_int delta, freq;
>
>@@ -547,7 +545,7 @@
> #ifdef CLK_CALIBRATION_LOOP
> if (bootverbose) {
> printf(
>- "Press a key on the console to abort clock calibration\n");
>+ "Press a key on the console to abort clock calibration\n");
> while (cncheckc() == -1)
> calibrate_clocks();
> }
>@@ -562,18 +560,16 @@
> if (delta < timer_freq / 100) {
> #ifndef CLK_USE_I8254_CALIBRATION
> if (bootverbose)
>- printf(
>-"CLK_USE_I8254_CALIBRATION not specified - using default frequency\n");
>+ printf("CLK_USE_I8254_CALIBRATION not specified - "
>+ "using default frequency\n");
> freq = timer_freq;
> #endif
> timer_freq = freq;
>- } else {
>+ } else
> if (bootverbose)
>- printf(
>- "%d Hz differs from default of %d Hz by more than 1%%\n",
>- freq, timer_freq);
>- }
>-
>+ printf("%d Hz differs from default of %d Hz by more "
>+ "than 1%%\n", freq, timer_freq);
>+
> set_timer_freq(timer_freq, hz);
> i8254_timecounter.tc_frequency = timer_freq;
> tc_init(&i8254_timecounter);
>@@ -633,10 +629,8 @@
> days += readrtc(RTC_DAY) - 1;
> for (y = 1970; y < year; y++)
> days += DAYSPERYEAR + LEAPYEAR(y);
>- sec = ((( days * 24 +
>- readrtc(RTC_HRS)) * 60 +
>- readrtc(RTC_MIN)) * 60 +
>- readrtc(RTC_SEC));
>+ sec = (((days * 24 +
>+ readrtc(RTC_HRS)) * 60 + readrtc(RTC_MIN)) * 60 + readrtc(RTC_SEC));
> /* sec now contains the number of seconds, since Jan 1 1970,
> in the local time zone */
>
>@@ -653,18 +647,19 @@
> return;
>
> wrong_time:
>- printf("Invalid time in real time clock.\n");
>- printf("Check and reset the date immediately!\n");
>+ printf("Invalid time in real time clock.\n"
>+ "Check and reset the date immediately!\n");
> }
>
> /*
> * Write system time back to RTC
> */
> void
>-resettodr()
>+resettodr(void)
> {
> unsigned long tm;
> int y, m, s;
>+ int ml;
>
> if (disable_rtc_set)
> return;
>@@ -697,8 +692,6 @@
> writertc(RTC_CENTURY, bin2bcd(y/100)); /* ... and Century */
> #endif
> for (m = 0; ; m++) {
>- int ml;
>-
> ml = daysinmonth[m];
> if (m == 1 && LEAPYEAR(y))
> ml++;
>@@ -720,7 +713,7 @@
> * Start both clocks running.
> */
> void
>-cpu_initclocks()
>+cpu_initclocks(void)
> {
> int diag;
>
>@@ -752,9 +745,9 @@
>
> /* Don't bother enabling the statistics clock. */
> if (!statclock_disable && !using_lapic_timer) {
>- diag = rtcin(RTC_DIAG);
>- if (diag != 0)
>- printf("RTC BIOS diagnostic error %b\n", diag, RTCDG_BITS);
>+ if ((diag = rtcin(RTC_DIAG)) != 0)
>+ printf("RTC BIOS diagnostic error %b\n",
>+ diag, RTCDG_BITS);
>
> intr_add_handler("rtc", 8, (driver_intr_t *)rtcintr, NULL,
> INTR_TYPE_CLK | INTR_FAST, NULL);
>@@ -772,8 +765,7 @@
>
> if (using_lapic_timer)
> return;
>- rtc_statusa = RTCSA_DIVIDER | RTCSA_PROF;
>- writertc(RTC_STATUSA, rtc_statusa);
>+ writertc(RTC_STATUSA, RTCSA_DIVIDER | RTCSA_PROF);
> psdiv = pscnt = psratio;
> }
>
>@@ -783,8 +775,7 @@
>
> if (using_lapic_timer)
> return;
>- rtc_statusa = RTCSA_DIVIDER | RTCSA_NOPROF;
>- writertc(RTC_STATUSA, rtc_statusa);
>+ writertc(RTC_STATUSA, RTCSA_DIVIDER | RTCSA_NOPROF);
> psdiv = pscnt = 1;
> }
>
>@@ -799,25 +790,23 @@
> * is is too generic. Should use it everywhere.
> */
> freq = timer_freq;
>- error = sysctl_handle_int(oidp, &freq, sizeof(freq), req);
>- if (error == 0 && req->newptr != NULL) {
>+ if ((error = sysctl_handle_int(oidp, &freq, sizeof(freq), req)) == 0 &&
>+ req->newptr != NULL) {
> set_timer_freq(freq, hz);
> i8254_timecounter.tc_frequency = freq;
> }
> return (error);
> }
>
>-SYSCTL_PROC(_machdep, OID_AUTO, i8254_freq, CTLTYPE_INT | CTLFLAG_RW,
>- 0, sizeof(u_int), sysctl_machdep_i8254_freq, "IU", "");
>+SYSCTL_PROC(_machdep, OID_AUTO, i8254_freq, CTLTYPE_INT | CTLFLAG_RW, 0,
>+ sizeof(u_int), sysctl_machdep_i8254_freq, "IU", "");
>
> static unsigned
> i8254_get_timecount(struct timecounter *tc)
> {
> u_int count;
> u_int high, low;
>- u_long rflags;
>
>- rflags = read_rflags();
> mtx_lock_spin(&clock_lock);
>
> /* Select timer0 and latch counter value. */
>@@ -826,10 +815,10 @@
> low = inb(TIMER_CNTR0);
> high = inb(TIMER_CNTR0);
> count = timer0_max_count - ((high << 8) | low);
>- if (count < i8254_lastcount ||
>- (!i8254_ticked && (clkintr_pending ||
>- ((count < 20 || (!(rflags & PSL_I) && count < timer0_max_count / 2u)) &&
>- i8254_pending != NULL && i8254_pending(i8254_intsrc))))) {
>+ if (count < i8254_lastcount || (!i8254_ticked && (clkintr_pending ||
>+ ((count < 20 || (!(read_rflags() & PSL_I) &&
>+ count < timer0_max_count / 2u)) && i8254_pending != NULL &&
>+ i8254_pending(i8254_intsrc))))) {
> i8254_ticked = 1;
> i8254_offset += timer0_max_count;
> }
>@@ -852,11 +841,12 @@
> static int
> attimer_probe(device_t dev)
> {
>- int result;
>+ int res;
>
>- if ((result = ISA_PNP_PROBE(device_get_parent(dev), dev, attimer_ids)) <= 0)
>+ if ((res =
>+ ISA_PNP_PROBE(device_get_parent(dev), dev, attimer_ids) <= 0))
> device_quiet(dev);
>- return(result);
>+ return(res);
> }
>
> static int
>--- sys/i386/isa/clock.c.orig Wed Apr 13 01:16:51 2005
>+++ sys/i386/isa/clock.c Wed Apr 13 01:55:54 2005
>@@ -93,10 +93,10 @@
> * 32-bit time_t's can't reach leap years before 1904 or after 2036, so we
> * can use a simple formula for leap years.
> */
>-#define LEAPYEAR(y) (((u_int)(y) % 4 == 0) ? 1 : 0)
>-#define DAYSPERYEAR (31+28+31+30+31+30+31+31+30+31+30+31)
>+#define LEAPYEAR(y) (((u_int)(y) % 4 == 0) ? 1 : 0)
>+#define DAYSPERYEAR (31+28+31+30+31+30+31+31+30+31+30+31)
>
>-#define TIMER_DIV(x) ((timer_freq + (x) / 2) / (x))
>+#define TIMER_DIV(x) ((timer_freq + (x) / 2) / (x))
>
> int adjkerntz; /* local offset from GMT in seconds */
> int clkintr_pending;
>@@ -105,7 +105,7 @@
> int psdiv = 1;
> int statclock_disable;
> #ifndef TIMER_FREQ
>-#define TIMER_FREQ 1193182
>+#define TIMER_FREQ 1193182
> #endif
> u_int timer_freq = TIMER_FREQ;
> int timer0_max_count;
>@@ -191,7 +191,7 @@
> }
>
> int
>-release_timer2()
>+release_timer2(void)
> {
>
> if (timer2_state != ACQUIRED)
>@@ -244,9 +244,9 @@
> DB_SHOW_COMMAND(rtc, rtc)
> {
> printf("%02x/%02x/%02x %02x:%02x:%02x, A = %02x, B = %02x, C = %02x\n",
>- rtcin(RTC_YEAR), rtcin(RTC_MONTH), rtcin(RTC_DAY),
>- rtcin(RTC_HRS), rtcin(RTC_MIN), rtcin(RTC_SEC),
>- rtcin(RTC_STATUSA), rtcin(RTC_STATUSB), rtcin(RTC_INTR));
>+ rtcin(RTC_YEAR), rtcin(RTC_MONTH), rtcin(RTC_DAY),
>+ rtcin(RTC_HRS), rtcin(RTC_MIN), rtcin(RTC_SEC),
>+ rtcin(RTC_STATUSA), rtcin(RTC_STATUSB), rtcin(RTC_INTR));
> }
> #endif /* DDB */
>
>@@ -339,7 +339,7 @@
> * before the delay is up (unless we're interrupted).
> */
> ticks_left = ((u_int)n * (long long)timer_freq + 999999)
>- / 1000000;
>+ / 1000000;
>
> while (ticks_left > 0) {
> #ifdef KDB
>@@ -372,7 +372,7 @@
> #ifdef DELAYDEBUG
> if (state == 1)
> printf(" %d calls to getit() at %d usec each\n",
>- getit_calls, (n + 5) / getit_calls);
>+ getit_calls, (n + 5) / getit_calls);
> #endif
> }
>
>@@ -414,8 +414,7 @@
> */
>
> int
>-rtcin(reg)
>- int reg;
>+rtcin(int reg)
> {
> u_char val;
>
>@@ -515,15 +514,14 @@
> goto fail;
> }
>
>- if (bootverbose) {
>+ if (bootverbose)
> printf("i8254 clock: %u Hz\n", tot_count);
>- }
> return (tot_count);
>
> fail:
> if (bootverbose)
> printf("failed, using default i8254 clock of %u Hz\n",
>- timer_freq);
>+ timer_freq);
> return (timer_freq);
> }
>
>@@ -587,7 +585,7 @@
> * XXX initialization of other timers is unintentionally left blank.
> */
> void
>-startrtclock()
>+startrtclock(void)
> {
> u_int delta, freq;
>
>@@ -599,7 +597,7 @@
> #ifdef CLK_CALIBRATION_LOOP
> if (bootverbose) {
> printf(
>- "Press a key on the console to abort clock calibration\n");
>+ "Press a key on the console to abort clock calibration\n");
> while (cncheckc() == -1)
> calibrate_clocks();
> }
>@@ -614,17 +612,15 @@
> if (delta < timer_freq / 100) {
> #ifndef CLK_USE_I8254_CALIBRATION
> if (bootverbose)
>- printf(
>-"CLK_USE_I8254_CALIBRATION not specified - using default frequency\n");
>+ printf("CLK_USE_I8254_CALIBRATION not specified - "
>+ "using default frequency\n");
> freq = timer_freq;
> #endif
> timer_freq = freq;
>- } else {
>+ } else
> if (bootverbose)
>- printf(
>- "%d Hz differs from default of %d Hz by more than 1%%\n",
>- freq, timer_freq);
>- }
>+ printf("%d Hz differs from default of %d Hz by more "
>+ "than 1%%\n", freq, timer_freq);
>
> set_timer_freq(timer_freq, hz);
> i8254_timecounter.tc_frequency = timer_freq;
>@@ -705,18 +701,19 @@
> return;
>
> wrong_time:
>- printf("Invalid time in real time clock.\n");
>- printf("Check and reset the date immediately!\n");
>+ printf("Invalid time in real time clock.\n"
>+ "Check and reset the date immediately!\n");
> }
>
> /*
> * Write system time back to RTC
> */
> void
>-resettodr()
>+resettodr(void)
> {
> unsigned long tm;
> int y, m, s;
>+ int ml;
>
> if (disable_rtc_set)
> return;
>@@ -749,8 +746,6 @@
> writertc(RTC_CENTURY, bin2bcd(y/100)); /* ... and Century */
> #endif
> for (m = 0; ; m++) {
>- int ml;
>-
> ml = daysinmonth[m];
> if (m == 1 && LEAPYEAR(y))
> ml++;
>@@ -772,7 +767,7 @@
> * Start both clocks running.
> */
> void
>-cpu_initclocks()
>+cpu_initclocks(void)
> {
> int diag;
>
>@@ -804,9 +799,9 @@
> * drive statclock() and profclock().
> */
> if (!statclock_disable && !using_lapic_timer) {
>- diag = rtcin(RTC_DIAG);
>- if (diag != 0)
>- printf("RTC BIOS diagnostic error %b\n", diag, RTCDG_BITS);
>+ if ((diag = rtcin(RTC_DIAG)) != 0)
>+ printf("RTC BIOS diagnostic error %b\n",
>+ diag, RTCDG_BITS);
>
> /* Setting stathz to nonzero early helps avoid races. */
> stathz = RTC_NOPROFRATE;
>@@ -830,8 +825,7 @@
>
> if (using_lapic_timer)
> return;
>- rtc_statusa = RTCSA_DIVIDER | RTCSA_PROF;
>- writertc(RTC_STATUSA, rtc_statusa);
>+ writertc(RTC_STATUSA, RTCSA_DIVIDER | RTCSA_PROF);
> psdiv = pscnt = psratio;
> }
>
>@@ -841,8 +835,7 @@
>
> if (using_lapic_timer)
> return;
>- rtc_statusa = RTCSA_DIVIDER | RTCSA_NOPROF;
>- writertc(RTC_STATUSA, rtc_statusa);
>+ writertc(RTC_STATUSA, RTCSA_DIVIDER | RTCSA_NOPROF);
> psdiv = pscnt = 1;
> }
>
>@@ -857,25 +850,23 @@
> * is is too generic. Should use it everywhere.
> */
> freq = timer_freq;
>- error = sysctl_handle_int(oidp, &freq, sizeof(freq), req);
>- if (error == 0 && req->newptr != NULL) {
>+ if ((error = sysctl_handle_int(oidp, &freq, sizeof(freq), req) == 0) &&
>+ req->newptr != NULL) {
> set_timer_freq(freq, hz);
> i8254_timecounter.tc_frequency = freq;
> }
> return (error);
> }
>
>-SYSCTL_PROC(_machdep, OID_AUTO, i8254_freq, CTLTYPE_INT | CTLFLAG_RW,
>- 0, sizeof(u_int), sysctl_machdep_i8254_freq, "IU", "");
>+SYSCTL_PROC(_machdep, OID_AUTO, i8254_freq, CTLTYPE_INT | CTLFLAG_RW, 0,
>+ sizeof(u_int), sysctl_machdep_i8254_freq, "IU", "");
>
> static unsigned
> i8254_get_timecount(struct timecounter *tc)
> {
> u_int count;
> u_int high, low;
>- u_int eflags;
>
>- eflags = read_eflags();
> mtx_lock_spin(&clock_lock);
>
> /* Select timer0 and latch counter value. */
>@@ -884,10 +875,10 @@
> low = inb(TIMER_CNTR0);
> high = inb(TIMER_CNTR0);
> count = timer0_max_count - ((high << 8) | low);
>- if (count < i8254_lastcount ||
>- (!i8254_ticked && (clkintr_pending ||
>- ((count < 20 || (!(eflags & PSL_I) && count < timer0_max_count / 2u)) &&
>- i8254_pending != NULL && i8254_pending(i8254_intsrc))))) {
>+ if (count < i8254_lastcount || (!i8254_ticked && (clkintr_pending ||
>+ ((count < 20 || (!(read_eflags() & PSL_I) &&
>+ count < timer0_max_count / 2u)) && i8254_pending != NULL &&
>+ i8254_pending(i8254_intsrc))))) {
> i8254_ticked = 1;
> i8254_offset += timer0_max_count;
> }
>@@ -910,11 +901,12 @@
> static int
> attimer_probe(device_t dev)
> {
>- int result;
>+ int res;
>
>- if ((result = ISA_PNP_PROBE(device_get_parent(dev), dev, attimer_ids)) <= 0)
>+ if ((res =
>+ ISA_PNP_PROBE(device_get_parent(dev), dev, attimer_ids)) <= 0)
> device_quiet(dev);
>- return(result);
>+ return(res);
> }
>
> static int
>
>--=-D2UC1HCBeWdBxtbsjtTR--
>
--
Poul-Henning Kamp | UNIX since Zilog Zeus 3.20
phk@FreeBSD.ORG | TCP/IP since RFC 956
FreeBSD committer | BSD since 4.3-tahoe
Never attribute to malice what can adequately be explained by incompetence.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?21177.1113415931>