* [PATCH v2] x86: Don't recursively acquire rtc_lock
@ 2011-08-30 16:12 Matt Fleming
2011-08-30 16:17 ` Matt Fleming
0 siblings, 1 reply; 6+ messages in thread
From: Matt Fleming @ 2011-08-30 16:12 UTC (permalink / raw)
To: Ingo Molnar, Thomas Gleixner
Cc: Jeremy Fitzhardinge, H. Peter Anvin, Marcelo Tosatti, johnstultz,
Matthew Garrett, linux-kernel, Matt Fleming
From: Matt Fleming <matt.fleming@intel.com>
A deadlock was introduced on x86 in commit ef68c8f87ed1 ("x86:
Serialize EFI time accesses on rtc_lock") because efi_get_time() and
friends can be called with rtc_lock already held by
read_persistent_time(), e.g.
timekeeping_init()
read_persistent_clock() <-- acquire rtc_lock
efi_get_time()
phys_efi_get_time() <-- acquire rtc_lock <DEADLOCK>
To fix this let's push the locking down into the get_wallclock() and
set_wallclock() implementations. Only the clock implementations that
access the x86 RTC directly need to acquire rtc_lock, so it makes
sense to push the locking down into the rtc, vrtc and efi code.
The virtualization implementations don't require rtc_lock to be held
because they provide their own serialization.
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
Acked-by: Jan Beulich <jbeulich@novell.com>
Acked-by: Avi Kivity <avi@redhat.com> [for the virtualization aspect]
---
arch/x86/kernel/rtc.c | 23 ++++++++++++-----------
arch/x86/platform/mrst/vrtc.c | 9 +++++++++
2 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/arch/x86/kernel/rtc.c b/arch/x86/kernel/rtc.c
index 3f2ad26..ccdbc16 100644
--- a/arch/x86/kernel/rtc.c
+++ b/arch/x86/kernel/rtc.c
@@ -42,8 +42,11 @@ int mach_set_rtc_mmss(unsigned long nowtime)
{
int real_seconds, real_minutes, cmos_minutes;
unsigned char save_control, save_freq_select;
+ unsigned long flags;
int retval = 0;
+ spin_lock_irqsave(&rtc_lock, flags);
+
/* tell the clock it's being set */
save_control = CMOS_READ(RTC_CONTROL);
CMOS_WRITE((save_control|RTC_SET), RTC_CONTROL);
@@ -93,12 +96,17 @@ int mach_set_rtc_mmss(unsigned long nowtime)
CMOS_WRITE(save_control, RTC_CONTROL);
CMOS_WRITE(save_freq_select, RTC_FREQ_SELECT);
+ spin_unlock_irqrestore(&rtc_lock, flags);
+
return retval;
}
unsigned long mach_get_cmos_time(void)
{
unsigned int status, year, mon, day, hour, min, sec, century = 0;
+ unsigned long flags;
+
+ spin_lock_irqsave(&rtc_lock, flags);
/*
* If UIP is clear, then we have >= 244 microseconds before
@@ -125,6 +133,8 @@ unsigned long mach_get_cmos_time(void)
status = CMOS_READ(RTC_CONTROL);
WARN_ON_ONCE(RTC_ALWAYS_BCD && (status & RTC_DM_BINARY));
+ spin_unlock_irqrestore(&rtc_lock, flags);
+
if (RTC_ALWAYS_BCD || !(status & RTC_DM_BINARY)) {
sec = bcd2bin(sec);
min = bcd2bin(min);
@@ -169,24 +179,15 @@ EXPORT_SYMBOL(rtc_cmos_write);
int update_persistent_clock(struct timespec now)
{
- unsigned long flags;
- int retval;
-
- spin_lock_irqsave(&rtc_lock, flags);
- retval = x86_platform.set_wallclock(now.tv_sec);
- spin_unlock_irqrestore(&rtc_lock, flags);
-
- return retval;
+ return x86_platform.set_wallclock(now.tv_sec);
}
/* not static: needed by APM */
void read_persistent_clock(struct timespec *ts)
{
- unsigned long retval, flags;
+ unsigned long retval;
- spin_lock_irqsave(&rtc_lock, flags);
retval = x86_platform.get_wallclock();
- spin_unlock_irqrestore(&rtc_lock, flags);
ts->tv_sec = retval;
ts->tv_nsec = 0;
diff --git a/arch/x86/platform/mrst/vrtc.c b/arch/x86/platform/mrst/vrtc.c
index 73d70d6..6d5dbcd 100644
--- a/arch/x86/platform/mrst/vrtc.c
+++ b/arch/x86/platform/mrst/vrtc.c
@@ -58,8 +58,11 @@ EXPORT_SYMBOL_GPL(vrtc_cmos_write);
unsigned long vrtc_get_time(void)
{
u8 sec, min, hour, mday, mon;
+ unsigned long flags;
u32 year;
+ spin_lock_irqsave(&rtc_lock, flags);
+
while ((vrtc_cmos_read(RTC_FREQ_SELECT) & RTC_UIP))
cpu_relax();
@@ -70,6 +73,8 @@ unsigned long vrtc_get_time(void)
mon = vrtc_cmos_read(RTC_MONTH);
year = vrtc_cmos_read(RTC_YEAR);
+ spin_unlock_irqrestore(&rtc_lock, flags);
+
/* vRTC YEAR reg contains the offset to 1960 */
year += 1960;
@@ -83,8 +88,10 @@ unsigned long vrtc_get_time(void)
int vrtc_set_mmss(unsigned long nowtime)
{
int real_sec, real_min;
+ unsigned long flags;
int vrtc_min;
+ spin_lock_irqsave(&rtc_lock, flags);
vrtc_min = vrtc_cmos_read(RTC_MINUTES);
real_sec = nowtime % 60;
@@ -95,6 +102,8 @@ int vrtc_set_mmss(unsigned long nowtime)
vrtc_cmos_write(real_sec, RTC_SECONDS);
vrtc_cmos_write(real_min, RTC_MINUTES);
+ spin_unlock_irqrestore(&rtc_lock, flags);
+
return 0;
}
--
1.7.4.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2] x86: Don't recursively acquire rtc_lock
2011-08-30 16:12 [PATCH v2] x86: Don't recursively acquire rtc_lock Matt Fleming
@ 2011-08-30 16:17 ` Matt Fleming
2011-09-30 7:26 ` Matt Fleming
0 siblings, 1 reply; 6+ messages in thread
From: Matt Fleming @ 2011-08-30 16:17 UTC (permalink / raw)
To: Ingo Molnar
Cc: Thomas Gleixner, Jeremy Fitzhardinge, H. Peter Anvin,
Marcelo Tosatti, johnstultz, Matthew Garrett, linux-kernel,
Jan Beulich, Avi Kivity
(Adding Jan and Avi, apparently git send-email doesn't grok Acked-by's)
On Tue, 2011-08-30 at 17:12 +0100, Matt Fleming wrote:
> From: Matt Fleming <matt.fleming@intel.com>
>
> A deadlock was introduced on x86 in commit ef68c8f87ed1 ("x86:
> Serialize EFI time accesses on rtc_lock") because efi_get_time() and
> friends can be called with rtc_lock already held by
> read_persistent_time(), e.g.
>
> timekeeping_init()
> read_persistent_clock() <-- acquire rtc_lock
> efi_get_time()
> phys_efi_get_time() <-- acquire rtc_lock <DEADLOCK>
>
> To fix this let's push the locking down into the get_wallclock() and
> set_wallclock() implementations. Only the clock implementations that
> access the x86 RTC directly need to acquire rtc_lock, so it makes
> sense to push the locking down into the rtc, vrtc and efi code.
>
> The virtualization implementations don't require rtc_lock to be held
> because they provide their own serialization.
>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> Acked-by: Jan Beulich <jbeulich@novell.com>
> Acked-by: Avi Kivity <avi@redhat.com> [for the virtualization aspect]
> ---
> arch/x86/kernel/rtc.c | 23 ++++++++++++-----------
> arch/x86/platform/mrst/vrtc.c | 9 +++++++++
> 2 files changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/kernel/rtc.c b/arch/x86/kernel/rtc.c
> index 3f2ad26..ccdbc16 100644
> --- a/arch/x86/kernel/rtc.c
> +++ b/arch/x86/kernel/rtc.c
> @@ -42,8 +42,11 @@ int mach_set_rtc_mmss(unsigned long nowtime)
> {
> int real_seconds, real_minutes, cmos_minutes;
> unsigned char save_control, save_freq_select;
> + unsigned long flags;
> int retval = 0;
>
> + spin_lock_irqsave(&rtc_lock, flags);
> +
> /* tell the clock it's being set */
> save_control = CMOS_READ(RTC_CONTROL);
> CMOS_WRITE((save_control|RTC_SET), RTC_CONTROL);
> @@ -93,12 +96,17 @@ int mach_set_rtc_mmss(unsigned long nowtime)
> CMOS_WRITE(save_control, RTC_CONTROL);
> CMOS_WRITE(save_freq_select, RTC_FREQ_SELECT);
>
> + spin_unlock_irqrestore(&rtc_lock, flags);
> +
> return retval;
> }
>
> unsigned long mach_get_cmos_time(void)
> {
> unsigned int status, year, mon, day, hour, min, sec, century = 0;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&rtc_lock, flags);
>
> /*
> * If UIP is clear, then we have >= 244 microseconds before
> @@ -125,6 +133,8 @@ unsigned long mach_get_cmos_time(void)
> status = CMOS_READ(RTC_CONTROL);
> WARN_ON_ONCE(RTC_ALWAYS_BCD && (status & RTC_DM_BINARY));
>
> + spin_unlock_irqrestore(&rtc_lock, flags);
> +
> if (RTC_ALWAYS_BCD || !(status & RTC_DM_BINARY)) {
> sec = bcd2bin(sec);
> min = bcd2bin(min);
> @@ -169,24 +179,15 @@ EXPORT_SYMBOL(rtc_cmos_write);
>
> int update_persistent_clock(struct timespec now)
> {
> - unsigned long flags;
> - int retval;
> -
> - spin_lock_irqsave(&rtc_lock, flags);
> - retval = x86_platform.set_wallclock(now.tv_sec);
> - spin_unlock_irqrestore(&rtc_lock, flags);
> -
> - return retval;
> + return x86_platform.set_wallclock(now.tv_sec);
> }
>
> /* not static: needed by APM */
> void read_persistent_clock(struct timespec *ts)
> {
> - unsigned long retval, flags;
> + unsigned long retval;
>
> - spin_lock_irqsave(&rtc_lock, flags);
> retval = x86_platform.get_wallclock();
> - spin_unlock_irqrestore(&rtc_lock, flags);
>
> ts->tv_sec = retval;
> ts->tv_nsec = 0;
> diff --git a/arch/x86/platform/mrst/vrtc.c b/arch/x86/platform/mrst/vrtc.c
> index 73d70d6..6d5dbcd 100644
> --- a/arch/x86/platform/mrst/vrtc.c
> +++ b/arch/x86/platform/mrst/vrtc.c
> @@ -58,8 +58,11 @@ EXPORT_SYMBOL_GPL(vrtc_cmos_write);
> unsigned long vrtc_get_time(void)
> {
> u8 sec, min, hour, mday, mon;
> + unsigned long flags;
> u32 year;
>
> + spin_lock_irqsave(&rtc_lock, flags);
> +
> while ((vrtc_cmos_read(RTC_FREQ_SELECT) & RTC_UIP))
> cpu_relax();
>
> @@ -70,6 +73,8 @@ unsigned long vrtc_get_time(void)
> mon = vrtc_cmos_read(RTC_MONTH);
> year = vrtc_cmos_read(RTC_YEAR);
>
> + spin_unlock_irqrestore(&rtc_lock, flags);
> +
> /* vRTC YEAR reg contains the offset to 1960 */
> year += 1960;
>
> @@ -83,8 +88,10 @@ unsigned long vrtc_get_time(void)
> int vrtc_set_mmss(unsigned long nowtime)
> {
> int real_sec, real_min;
> + unsigned long flags;
> int vrtc_min;
>
> + spin_lock_irqsave(&rtc_lock, flags);
> vrtc_min = vrtc_cmos_read(RTC_MINUTES);
>
> real_sec = nowtime % 60;
> @@ -95,6 +102,8 @@ int vrtc_set_mmss(unsigned long nowtime)
>
> vrtc_cmos_write(real_sec, RTC_SECONDS);
> vrtc_cmos_write(real_min, RTC_MINUTES);
> + spin_unlock_irqrestore(&rtc_lock, flags);
> +
> return 0;
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] x86: Don't recursively acquire rtc_lock
2011-08-30 16:17 ` Matt Fleming
@ 2011-09-30 7:26 ` Matt Fleming
2011-10-10 6:45 ` Ingo Molnar
0 siblings, 1 reply; 6+ messages in thread
From: Matt Fleming @ 2011-09-30 7:26 UTC (permalink / raw)
To: Ingo Molnar, Thomas Gleixner
Cc: Jeremy Fitzhardinge, H. Peter Anvin, Marcelo Tosatti, johnstultz,
Matthew Garrett, linux-kernel, Jan Beulich, Avi Kivity,
Zhang, Rui
On Tue, 2011-08-30 at 17:17 +0100, Matt Fleming wrote:
> (Adding Jan and Avi, apparently git send-email doesn't grok Acked-by's)
>
> On Tue, 2011-08-30 at 17:12 +0100, Matt Fleming wrote:
> > From: Matt Fleming <matt.fleming@intel.com>
> >
> > A deadlock was introduced on x86 in commit ef68c8f87ed1 ("x86:
> > Serialize EFI time accesses on rtc_lock") because efi_get_time() and
> > friends can be called with rtc_lock already held by
> > read_persistent_time(), e.g.
> >
> > timekeeping_init()
> > read_persistent_clock() <-- acquire rtc_lock
> > efi_get_time()
> > phys_efi_get_time() <-- acquire rtc_lock <DEADLOCK>
> >
> > To fix this let's push the locking down into the get_wallclock() and
> > set_wallclock() implementations. Only the clock implementations that
> > access the x86 RTC directly need to acquire rtc_lock, so it makes
> > sense to push the locking down into the rtc, vrtc and efi code.
> >
> > The virtualization implementations don't require rtc_lock to be held
> > because they provide their own serialization.
> >
> > Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> > Acked-by: Jan Beulich <jbeulich@novell.com>
> > Acked-by: Avi Kivity <avi@redhat.com> [for the virtualization aspect]
Ping? It's -rc8 and 32-bit EFI machines still don't boot.
--
Matt Fleming, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] x86: Don't recursively acquire rtc_lock
2011-09-30 7:26 ` Matt Fleming
@ 2011-10-10 6:45 ` Ingo Molnar
2011-10-10 7:15 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Ingo Molnar @ 2011-10-10 6:45 UTC (permalink / raw)
To: Matt Fleming
Cc: Thomas Gleixner, Jeremy Fitzhardinge, H. Peter Anvin,
Marcelo Tosatti, johnstultz, Matthew Garrett, linux-kernel,
Jan Beulich, Avi Kivity, Zhang, Rui
* Matt Fleming <matt@console-pimps.org> wrote:
> On Tue, 2011-08-30 at 17:17 +0100, Matt Fleming wrote:
> > (Adding Jan and Avi, apparently git send-email doesn't grok Acked-by's)
> >
> > On Tue, 2011-08-30 at 17:12 +0100, Matt Fleming wrote:
> > > From: Matt Fleming <matt.fleming@intel.com>
> > >
> > > A deadlock was introduced on x86 in commit ef68c8f87ed1 ("x86:
> > > Serialize EFI time accesses on rtc_lock") because efi_get_time() and
> > > friends can be called with rtc_lock already held by
> > > read_persistent_time(), e.g.
> > >
> > > timekeeping_init()
> > > read_persistent_clock() <-- acquire rtc_lock
> > > efi_get_time()
> > > phys_efi_get_time() <-- acquire rtc_lock <DEADLOCK>
> > >
> > > To fix this let's push the locking down into the get_wallclock() and
> > > set_wallclock() implementations. Only the clock implementations that
> > > access the x86 RTC directly need to acquire rtc_lock, so it makes
> > > sense to push the locking down into the rtc, vrtc and efi code.
> > >
> > > The virtualization implementations don't require rtc_lock to be held
> > > because they provide their own serialization.
> > >
> > > Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> > > Acked-by: Jan Beulich <jbeulich@novell.com>
> > > Acked-by: Avi Kivity <avi@redhat.com> [for the virtualization aspect]
>
> Ping? It's -rc8 and 32-bit EFI machines still don't boot.
Don't know the status of this - Thomas?
Thanks,
Ingo
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] x86: Don't recursively acquire rtc_lock
2011-10-10 6:45 ` Ingo Molnar
@ 2011-10-10 7:15 ` Jan Beulich
2011-10-10 7:54 ` Ingo Molnar
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2011-10-10 7:15 UTC (permalink / raw)
To: Ingo Molnar
Cc: Jeremy Fitzhardinge, Matt Fleming, Rui Zhang, Thomas Gleixner,
H. Peter Anvin, Avi Kivity, Matthew Garrett, Marcelo Tosatti,
johnstultz, linux-kernel
>>> On 10.10.11 at 08:45, Ingo Molnar <mingo@elte.hu> wrote:
> * Matt Fleming <matt@console-pimps.org> wrote:
>> Ping? It's -rc8 and 32-bit EFI machines still don't boot.
>
> Don't know the status of this - Thomas?
This went into 3.1-rc9 (with your signed-off being last).
Jan
> Thanks,
>
> Ingo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] x86: Don't recursively acquire rtc_lock
2011-10-10 7:15 ` Jan Beulich
@ 2011-10-10 7:54 ` Ingo Molnar
0 siblings, 0 replies; 6+ messages in thread
From: Ingo Molnar @ 2011-10-10 7:54 UTC (permalink / raw)
To: Jan Beulich
Cc: Jeremy Fitzhardinge, Matt Fleming, Rui Zhang, Thomas Gleixner,
H. Peter Anvin, Avi Kivity, Matthew Garrett, Marcelo Tosatti,
johnstultz, linux-kernel
* Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 10.10.11 at 08:45, Ingo Molnar <mingo@elte.hu> wrote:
> > * Matt Fleming <matt@console-pimps.org> wrote:
> >> Ping? It's -rc8 and 32-bit EFI machines still don't boot.
> >
> > Don't know the status of this - Thomas?
>
> This went into 3.1-rc9 (with your signed-off being last).
Heh, I typoed the search for the commit and thought it's still
unmerged and there's still some problem left with this patch.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-10-10 7:56 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-08-30 16:12 [PATCH v2] x86: Don't recursively acquire rtc_lock Matt Fleming
2011-08-30 16:17 ` Matt Fleming
2011-09-30 7:26 ` Matt Fleming
2011-10-10 6:45 ` Ingo Molnar
2011-10-10 7:15 ` Jan Beulich
2011-10-10 7:54 ` Ingo Molnar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox