[patch] x86/efi: use GFP_ATOMIC under spin_lock

[patch] x86/efi: use GFP_ATOMIC under spin_lock

[Dan Carpenter]

In phys_efi_get_time() we call efi_call_phys_prelog() with a spin_lock
so this allocation should be atomic.

Fixes: b8f2c21db390 ('efi, x86: Pass a proper identity mapping in efi_call_phys_prelog')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 0c2a234fef1e..f5adcadb381b 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -90,7 +90,7 @@ void __init efi_call_phys_prelog(void)
 	local_irq_save(efi_flags);
 
 	n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT), PGDIR_SIZE);
-	save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_KERNEL);
+	save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_ATOMIC);
 
 	for (pgd = 0; pgd < n_pgds; pgd++) {
 		save_pgd[pgd] = *pgd_offset_k(pgd * PGDIR_SIZE);

Re: [patch] x86/efi: use GFP_ATOMIC under spin_lock

[Ingo Molnar]

* Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> wrote:

> In phys_efi_get_time() we call efi_call_phys_prelog() with a spin_lock
> so this allocation should be atomic.
> 
> Fixes: b8f2c21db390 ('efi, x86: Pass a proper identity mapping in efi_call_phys_prelog')
> Signed-off-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> 
> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index 0c2a234fef1e..f5adcadb381b 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -90,7 +90,7 @@ void __init efi_call_phys_prelog(void)
>  	local_irq_save(efi_flags);
>  
>  	n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT), PGDIR_SIZE);
> -	save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_KERNEL);
> +	save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_ATOMIC);
>  
>  	for (pgd = 0; pgd < n_pgds; pgd++) {
>  		save_pgd[pgd] = *pgd_offset_k(pgd * PGDIR_SIZE);

The allocation there, if it happens within a spinlocked path, is 
probably a layering violation - and GFP_ATOMIC is at best a 
workaround.

Thanks,

	Ingo

Re: [patch] x86/efi: use GFP_ATOMIC under spin_lock

[Dan Carpenter]

On Fri, Mar 07, 2014 at 01:10:22PM +0100, Ingo Molnar wrote:
> 
> * Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> wrote:
> 
> > In phys_efi_get_time() we call efi_call_phys_prelog() with a spin_lock
> > so this allocation should be atomic.
> > 
> > Fixes: b8f2c21db390 ('efi, x86: Pass a proper identity mapping in efi_call_phys_prelog')
> > Signed-off-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > 
> > diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> > index 0c2a234fef1e..f5adcadb381b 100644
> > --- a/arch/x86/platform/efi/efi_64.c
> > +++ b/arch/x86/platform/efi/efi_64.c
> > @@ -90,7 +90,7 @@ void __init efi_call_phys_prelog(void)
> >  	local_irq_save(efi_flags);
> >  
> >  	n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT), PGDIR_SIZE);
> > -	save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_KERNEL);
> > +	save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_ATOMIC);
> >  
> >  	for (pgd = 0; pgd < n_pgds; pgd++) {
> >  		save_pgd[pgd] = *pgd_offset_k(pgd * PGDIR_SIZE);
> 
> The allocation there, if it happens within a spinlocked path, is 
> probably a layering violation - and GFP_ATOMIC is at best a 
> workaround.
> 

You're on your own for fixing the complicated stuff like layering
violations. I just do static checker stuff and sed fixes.  ;)

regards,
dan carpenter

Re: [patch] x86/efi: use GFP_ATOMIC under spin_lock

[Ingo Molnar]

* Dan Carpenter <dan.carpenter@oracle.com> wrote:

> On Fri, Mar 07, 2014 at 01:10:22PM +0100, Ingo Molnar wrote:
> > 
> > * Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > 
> > > In phys_efi_get_time() we call efi_call_phys_prelog() with a spin_lock
> > > so this allocation should be atomic.
> > > 
> > > Fixes: b8f2c21db390 ('efi, x86: Pass a proper identity mapping in efi_call_phys_prelog')
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > 
> > > diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> > > index 0c2a234fef1e..f5adcadb381b 100644
> > > --- a/arch/x86/platform/efi/efi_64.c
> > > +++ b/arch/x86/platform/efi/efi_64.c
> > > @@ -90,7 +90,7 @@ void __init efi_call_phys_prelog(void)
> > >  	local_irq_save(efi_flags);
> > >  
> > >  	n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT), PGDIR_SIZE);
> > > -	save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_KERNEL);
> > > +	save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_ATOMIC);
> > >  
> > >  	for (pgd = 0; pgd < n_pgds; pgd++) {
> > >  		save_pgd[pgd] = *pgd_offset_k(pgd * PGDIR_SIZE);
> > 
> > The allocation there, if it happens within a spinlocked path, is 
> > probably a layering violation - and GFP_ATOMIC is at best a 
> > workaround.
> > 
> 
> You're on your own for fixing the complicated stuff like layering 
> violations. I just do static checker stuff and sed fixes.  ;)

It's a generic pattern: GFP_KERNEL -> GFP_ATOMIC fixes are almost 
always wrong.

Thanks,

	Ingo

Re: [patch] x86/efi: use GFP_ATOMIC under spin_lock

[Dan Carpenter]

On Sun, Mar 09, 2014 at 07:48:19AM +0100, Ingo Molnar wrote:
> 
> * Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> > On Fri, Mar 07, 2014 at 01:10:22PM +0100, Ingo Molnar wrote:
> > > 
> > > * Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > 
> > > > In phys_efi_get_time() we call efi_call_phys_prelog() with a spin_lock
> > > > so this allocation should be atomic.
> > > > 
> > > > Fixes: b8f2c21db390 ('efi, x86: Pass a proper identity mapping in efi_call_phys_prelog')
> > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > 
> > > > diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> > > > index 0c2a234fef1e..f5adcadb381b 100644
> > > > --- a/arch/x86/platform/efi/efi_64.c
> > > > +++ b/arch/x86/platform/efi/efi_64.c
> > > > @@ -90,7 +90,7 @@ void __init efi_call_phys_prelog(void)
> > > >  	local_irq_save(efi_flags);
> > > >  
> > > >  	n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT), PGDIR_SIZE);
> > > > -	save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_KERNEL);
> > > > +	save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_ATOMIC);
> > > >  
> > > >  	for (pgd = 0; pgd < n_pgds; pgd++) {
> > > >  		save_pgd[pgd] = *pgd_offset_k(pgd * PGDIR_SIZE);
> > > 
> > > The allocation there, if it happens within a spinlocked path, is 
> > > probably a layering violation - and GFP_ATOMIC is at best a 
> > > workaround.
> > > 
> > 
> > You're on your own for fixing the complicated stuff like layering 
> > violations. I just do static checker stuff and sed fixes.  ;)
> 
> It's a generic pattern: GFP_KERNEL -> GFP_ATOMIC fixes are almost 
> always wrong.
> 

I always feel that way too.  :/  There are too many places where
GFP_ATOMIC is used.  I think it's great that you're going to fix this
properly.

regards,
dan carpenter

Re: [patch] x86/efi: use GFP_ATOMIC under spin_lock

[Matt Fleming]

On Fri, 07 Mar, at 03:25:37PM, Dan Carpenter wrote:
> 
> You're on your own for fixing the complicated stuff like layering
> violations. I just do static checker stuff and sed fixes.  ;)

Thanks for the patch Dan. Nice catch.

But I'm wondering if we can simply delete phys_efi_get_time() to avoid
the whole problem of doing GFP_KERNEL allocations under a spinlock.

In fact, the whole EFI time stuff is looking a bit crusty.

I only see two direct users of efi.get_time() outside of arch,

  drivers/char/efirtc.c
  drivers/rtc/rtc-efi.c

which are both "depends on IA64". For x86, all other callers are inside
arch/x86/platform/efi/efi.c,

  efi_set_rtc_mmss()
  efi_get_time()

neither of which have any callers - it all appears to be dead code. The
diff stat of deleting all this dead code isn't too bad either,

 arch/x86/platform/efi/efi.c    | 151 ++---------------------------------------
 arch/x86/platform/efi/efi_64.c |  90 ------------------------
 2 files changed, 5 insertions(+), 236 deletions(-)

Thoughts?

---

>From c17c2275d1969e99f4a6427293af1a533f2e5242 Mon Sep 17 00:00:00 2001
From: Matt Fleming <matt.fleming@intel.com>
Date: Sun, 9 Mar 2014 14:18:10 +0000
Subject: [PATCH] x86/efi: Remove EFI time functions

Dan reported that phys_efi_get_time() is doing kmalloc(..., GFP_KERNEL)
under a spinlock which is very clearly a bug. However, the EFI time
functions have been dead code for x86 since commit 04bf9ba720fc ("x86,
efi: Don't use (U)EFI time services on 32 bit").

We have tried to use the time functions before, with little success
because of various bugs in the runtime implementations, e.g. see commit
bacef661acdb ("x86-64/efi: Use EFI to deal with platform wall clock")
and commit bd52276fa1d4 ("x86-64/efi: Use EFI to deal with platform wall
clock (again)").

Let's rip them out. If we find a need for them in the future, and if the
runtime implementations of the time services improve considerably, the
code can be put back.

Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Nathan Zimmer <nzimmer@sgi.com>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>
Cc: Jan Beulich <JBeulich@suse.com>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
 arch/x86/platform/efi/efi.c    | 151 ++---------------------------------------
 arch/x86/platform/efi/efi_64.c |  90 ------------------------
 2 files changed, 5 insertions(+), 236 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 43e7cf6c6111..fc1a8f639f7b 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -40,14 +40,12 @@
 #include <linux/memblock.h>
 #include <linux/spinlock.h>
 #include <linux/uaccess.h>
-#include <linux/time.h>
 #include <linux/io.h>
 #include <linux/reboot.h>
 #include <linux/bcd.h>
 
 #include <asm/setup.h>
 #include <asm/efi.h>
-#include <asm/time.h>
 #include <asm/cacheflush.h>
 #include <asm/tlbflush.h>
 #include <asm/x86_init.h>
@@ -104,54 +102,6 @@ static int __init setup_storage_paranoia(char *arg)
 }
 early_param("efi_no_storage_paranoia", setup_storage_paranoia);
 
-static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
-{
-	unsigned long flags;
-	efi_status_t status;
-
-	spin_lock_irqsave(&rtc_lock, flags);
-	status = efi_call_virt2(get_time, tm, tc);
-	spin_unlock_irqrestore(&rtc_lock, flags);
-	return status;
-}
-
-static efi_status_t virt_efi_set_time(efi_time_t *tm)
-{
-	unsigned long flags;
-	efi_status_t status;
-
-	spin_lock_irqsave(&rtc_lock, flags);
-	status = efi_call_virt1(set_time, tm);
-	spin_unlock_irqrestore(&rtc_lock, flags);
-	return status;
-}
-
-static efi_status_t virt_efi_get_wakeup_time(efi_bool_t *enabled,
-					     efi_bool_t *pending,
-					     efi_time_t *tm)
-{
-	unsigned long flags;
-	efi_status_t status;
-
-	spin_lock_irqsave(&rtc_lock, flags);
-	status = efi_call_virt3(get_wakeup_time,
-				enabled, pending, tm);
-	spin_unlock_irqrestore(&rtc_lock, flags);
-	return status;
-}
-
-static efi_status_t virt_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm)
-{
-	unsigned long flags;
-	efi_status_t status;
-
-	spin_lock_irqsave(&rtc_lock, flags);
-	status = efi_call_virt2(set_wakeup_time,
-				enabled, tm);
-	spin_unlock_irqrestore(&rtc_lock, flags);
-	return status;
-}
-
 static efi_status_t virt_efi_get_variable(efi_char16_t *name,
 					  efi_guid_t *vendor,
 					  u32 *attr,
@@ -194,11 +144,6 @@ static efi_status_t virt_efi_query_variable_info(u32 attr,
 			      remaining_space, max_variable_size);
 }
 
-static efi_status_t virt_efi_get_next_high_mono_count(u32 *count)
-{
-	return efi_call_virt1(get_next_high_mono_count, count);
-}
-
 static void virt_efi_reset_system(int reset_type,
 				  efi_status_t status,
 				  unsigned long data_size,
@@ -246,72 +191,6 @@ static efi_status_t __init phys_efi_set_virtual_address_map(
 	return status;
 }
 
-static efi_status_t __init phys_efi_get_time(efi_time_t *tm,
-					     efi_time_cap_t *tc)
-{
-	unsigned long flags;
-	efi_status_t status;
-
-	spin_lock_irqsave(&rtc_lock, flags);
-	efi_call_phys_prelog();
-	status = efi_call_phys2(efi_phys.get_time, virt_to_phys(tm),
-				virt_to_phys(tc));
-	efi_call_phys_epilog();
-	spin_unlock_irqrestore(&rtc_lock, flags);
-	return status;
-}
-
-int efi_set_rtc_mmss(const struct timespec *now)
-{
-	unsigned long nowtime = now->tv_sec;
-	efi_status_t	status;
-	efi_time_t	eft;
-	efi_time_cap_t	cap;
-	struct rtc_time	tm;
-
-	status = efi.get_time(&eft, &cap);
-	if (status != EFI_SUCCESS) {
-		pr_err("Oops: efitime: can't read time!\n");
-		return -1;
-	}
-
-	rtc_time_to_tm(nowtime, &tm);
-	if (!rtc_valid_tm(&tm)) {
-		eft.year = tm.tm_year + 1900;
-		eft.month = tm.tm_mon + 1;
-		eft.day = tm.tm_mday;
-		eft.minute = tm.tm_min;
-		eft.second = tm.tm_sec;
-		eft.nanosecond = 0;
-	} else {
-		pr_err("%s: Invalid EFI RTC value: write of %lx to EFI RTC failed\n",
-		       __func__, nowtime);
-		return -1;
-	}
-
-	status = efi.set_time(&eft);
-	if (status != EFI_SUCCESS) {
-		pr_err("Oops: efitime: can't write time!\n");
-		return -1;
-	}
-	return 0;
-}
-
-void efi_get_time(struct timespec *now)
-{
-	efi_status_t status;
-	efi_time_t eft;
-	efi_time_cap_t cap;
-
-	status = efi.get_time(&eft, &cap);
-	if (status != EFI_SUCCESS)
-		pr_err("Oops: efitime: can't read time!\n");
-
-	now->tv_sec = mktime(eft.year, eft.month, eft.day, eft.hour,
-			     eft.minute, eft.second);
-	now->tv_nsec = 0;
-}
-
 /*
  * Tell the kernel about the EFI memory map.  This might include
  * more than the max 128 entries that can fit in the e820 legacy
@@ -588,20 +467,12 @@ static int __init efi_runtime_init32(void)
 	}
 
 	/*
-	 * We will only need *early* access to the following two
-	 * EFI runtime services before set_virtual_address_map
-	 * is invoked.
+	 * We will only need *early* access to the following EFI runtime
+	 * service before set_virtual_address_map is invoked.
 	 */
-	efi_phys.get_time = (efi_get_time_t *)
-			(unsigned long)runtime->get_time;
 	efi_phys.set_virtual_address_map =
 			(efi_set_virtual_address_map_t *)
 			(unsigned long)runtime->set_virtual_address_map;
-	/*
-	 * Make efi_get_time can be called before entering
-	 * virtual mode.
-	 */
-	efi.get_time = phys_efi_get_time;
 	early_iounmap(runtime, sizeof(efi_runtime_services_32_t));
 
 	return 0;
@@ -619,20 +490,13 @@ static int __init efi_runtime_init64(void)
 	}
 
 	/*
-	 * We will only need *early* access to the following two
-	 * EFI runtime services before set_virtual_address_map
-	 * is invoked.
+	 * We will only need *early* access to the following EFI runtime
+	 * service before set_virtual_address_map is invoked.
 	 */
-	efi_phys.get_time = (efi_get_time_t *)
-			(unsigned long)runtime->get_time;
 	efi_phys.set_virtual_address_map =
 			(efi_set_virtual_address_map_t *)
 			(unsigned long)runtime->set_virtual_address_map;
-	/*
-	 * Make efi_get_time can be called before entering
-	 * virtual mode.
-	 */
-	efi.get_time = phys_efi_get_time;
+
 	early_iounmap(runtime, sizeof(efi_runtime_services_64_t));
 
 	return 0;
@@ -880,14 +744,9 @@ void __init old_map_region(efi_memory_desc_t *md)
 
 static void native_runtime_setup(void)
 {
-	efi.get_time = virt_efi_get_time;
-	efi.set_time = virt_efi_set_time;
-	efi.get_wakeup_time = virt_efi_get_wakeup_time;
-	efi.set_wakeup_time = virt_efi_set_wakeup_time;
 	efi.get_variable = virt_efi_get_variable;
 	efi.get_next_variable = virt_efi_get_next_variable;
 	efi.set_variable = virt_efi_set_variable;
-	efi.get_next_high_mono_count = virt_efi_get_next_high_mono_count;
 	efi.reset_system = virt_efi_reset_system;
 	efi.query_variable_info = virt_efi_query_variable_info;
 	efi.update_capsule = virt_efi_update_capsule;
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 7e7f195aa5cf..468444157e3c 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -39,7 +39,6 @@
 #include <asm/cacheflush.h>
 #include <asm/fixmap.h>
 #include <asm/realmode.h>
-#include <asm/time.h>
 
 static pgd_t *save_pgd __initdata;
 static unsigned long efi_flags __initdata;
@@ -389,78 +388,6 @@ efi_status_t efi_thunk_set_virtual_address_map(
 	return status;
 }
 
-static efi_status_t efi_thunk_get_time(efi_time_t *tm, efi_time_cap_t *tc)
-{
-	efi_status_t status;
-	u32 phys_tm, phys_tc;
-
-	spin_lock(&rtc_lock);
-
-	phys_tm = virt_to_phys(tm);
-	phys_tc = virt_to_phys(tc);
-
-	status = efi_thunk(get_time, phys_tm, phys_tc);
-
-	spin_unlock(&rtc_lock);
-
-	return status;
-}
-
-static efi_status_t efi_thunk_set_time(efi_time_t *tm)
-{
-	efi_status_t status;
-	u32 phys_tm;
-
-	spin_lock(&rtc_lock);
-
-	phys_tm = virt_to_phys(tm);
-
-	status = efi_thunk(set_time, phys_tm);
-
-	spin_unlock(&rtc_lock);
-
-	return status;
-}
-
-static efi_status_t
-efi_thunk_get_wakeup_time(efi_bool_t *enabled, efi_bool_t *pending,
-			  efi_time_t *tm)
-{
-	efi_status_t status;
-	u32 phys_enabled, phys_pending, phys_tm;
-
-	spin_lock(&rtc_lock);
-
-	phys_enabled = virt_to_phys(enabled);
-	phys_pending = virt_to_phys(pending);
-	phys_tm = virt_to_phys(tm);
-
-	status = efi_thunk(get_wakeup_time, phys_enabled,
-			     phys_pending, phys_tm);
-
-	spin_unlock(&rtc_lock);
-
-	return status;
-}
-
-static efi_status_t
-efi_thunk_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm)
-{
-	efi_status_t status;
-	u32 phys_tm;
-
-	spin_lock(&rtc_lock);
-
-	phys_tm = virt_to_phys(tm);
-
-	status = efi_thunk(set_wakeup_time, enabled, phys_tm);
-
-	spin_unlock(&rtc_lock);
-
-	return status;
-}
-
-
 static efi_status_t
 efi_thunk_get_variable(efi_char16_t *name, efi_guid_t *vendor,
 		       u32 *attr, unsigned long *data_size, void *data)
@@ -517,18 +444,6 @@ efi_thunk_get_next_variable(unsigned long *name_size,
 	return status;
 }
 
-static efi_status_t
-efi_thunk_get_next_high_mono_count(u32 *count)
-{
-	efi_status_t status;
-	u32 phys_count;
-
-	phys_count = virt_to_phys(count);
-	status = efi_thunk(get_next_high_mono_count, phys_count);
-
-	return status;
-}
-
 static void
 efi_thunk_reset_system(int reset_type, efi_status_t status,
 		       unsigned long data_size, efi_char16_t *data)
@@ -588,14 +503,9 @@ efi_thunk_query_capsule_caps(efi_capsule_header_t **capsules,
 
 void efi_thunk_runtime_setup(void)
 {
-	efi.get_time = efi_thunk_get_time;
-	efi.set_time = efi_thunk_set_time;
-	efi.get_wakeup_time = efi_thunk_get_wakeup_time;
-	efi.set_wakeup_time = efi_thunk_set_wakeup_time;
 	efi.get_variable = efi_thunk_get_variable;
 	efi.get_next_variable = efi_thunk_get_next_variable;
 	efi.set_variable = efi_thunk_set_variable;
-	efi.get_next_high_mono_count = efi_thunk_get_next_high_mono_count;
 	efi.reset_system = efi_thunk_reset_system;
 	efi.query_variable_info = efi_thunk_query_variable_info;
 	efi.update_capsule = efi_thunk_update_capsule;
-- 
1.8.5.3

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [patch] x86/efi: use GFP_ATOMIC under spin_lock

[Matthew Garrett]

On Sun, Mar 09, 2014 at 04:20:20PM +0000, Matt Fleming wrote:

> We have tried to use the time functions before, with little success
> because of various bugs in the runtime implementations, e.g. see commit
> bacef661acdb ("x86-64/efi: Use EFI to deal with platform wall clock")
> and commit bd52276fa1d4 ("x86-64/efi: Use EFI to deal with platform wall
> clock (again)").

I'd naively expected that these would be more reliable after the 1:1 
mapping patches, so it might actually be time to give them another go.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [patch] x86/efi: use GFP_ATOMIC under spin_lock

[Matt Fleming]

On Sun, 09 Mar, at 04:31:41PM, Matthew Garrett wrote:
> On Sun, Mar 09, 2014 at 04:20:20PM +0000, Matt Fleming wrote:
> 
> > We have tried to use the time functions before, with little success
> > because of various bugs in the runtime implementations, e.g. see commit
> > bacef661acdb ("x86-64/efi: Use EFI to deal with platform wall clock")
> > and commit bd52276fa1d4 ("x86-64/efi: Use EFI to deal with platform wall
> > clock (again)").
> 
> I'd naively expected that these would be more reliable after the 1:1 
> mapping patches, so it might actually be time to give them another go.

Is there any value in that? Do machines exist where we absolutely must
have access to the EFI time services? Either because there's no other
method or no other working one?

I can check again, but I'm pretty sure this ASUS machine under my desk
always returns a vendor-specific error code when invoked, even with
Borislav's 1:1 patches. Enabling EFI services just because they exist
hasn't worked out well for us in the past.

So enabling them is fine, but we certainly need some kind of priority
mechanism so they're used as a last resort and some ironclad use case of
why we absolutely must have this.

And of course, the GPF_KERNEL alloc under spinlock bug that started this
thread needs to be fixed.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [patch] x86/efi: use GFP_ATOMIC under spin_lock

[Matthew Garrett]

On Sun, Mar 09, 2014 at 06:50:28PM +0000, Matt Fleming wrote:

> Is there any value in that? Do machines exist where we absolutely must
> have access to the EFI time services? Either because there's no other
> method or no other working one?

In theory we get timezones, which we otherwise only get if we have an 
ACPI TAD device.

> I can check again, but I'm pretty sure this ASUS machine under my desk
> always returns a vendor-specific error code when invoked, even with
> Borislav's 1:1 patches. Enabling EFI services just because they exist
> hasn't worked out well for us in the past.

Returning an error doesn't seem like a problem, as long as it's not 
actually killing the machine in the process...

> And of course, the GPF_KERNEL alloc under spinlock bug that started this
> thread needs to be fixed.

Losing phys_efi_get_time() doesn't seem like it ought to be much of a 
problem.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [patch] x86/efi: use GFP_ATOMIC under spin_lock

[Matt Fleming]

On Sun, 09 Mar, at 07:00:53PM, Matthew Garrett wrote:
> On Sun, Mar 09, 2014 at 06:50:28PM +0000, Matt Fleming wrote:
> 
> > Is there any value in that? Do machines exist where we absolutely must
> > have access to the EFI time services? Either because there's no other
> > method or no other working one?
> 
> In theory we get timezones, which we otherwise only get if we have an 
> ACPI TAD device.
 
We'd previously discussed grabbing this in the EFI stub, before
ExitBootServices(), assuming that we only want to get the timezone as
opposed to setting it. We don't need phys_* and virt_* functions in the
kernel proper for that.

> > I can check again, but I'm pretty sure this ASUS machine under my desk
> > always returns a vendor-specific error code when invoked, even with
> > Borislav's 1:1 patches. Enabling EFI services just because they exist
> > hasn't worked out well for us in the past.
> 
> Returning an error doesn't seem like a problem, as long as it's not 
> actually killing the machine in the process...
 
True, but I don't think that's a good enough justificaton for letting
those functions live.

If someone can show me a platform where they're needed/advantageous,
then that's a different story.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [patch] x86/efi: use GFP_ATOMIC under spin_lock

[Jan Beulich]

>>> On 09.03.14 at 19:50, Matt Fleming <matt-HNK1S37rvNbeXh+fF434Mdi2O/JbrIOy@public.gmane.org> wrote:
> On Sun, 09 Mar, at 04:31:41PM, Matthew Garrett wrote:
>> On Sun, Mar 09, 2014 at 04:20:20PM +0000, Matt Fleming wrote:
>> 
>> > We have tried to use the time functions before, with little success
>> > because of various bugs in the runtime implementations, e.g. see commit
>> > bacef661acdb ("x86-64/efi: Use EFI to deal with platform wall clock")
>> > and commit bd52276fa1d4 ("x86-64/efi: Use EFI to deal with platform wall
>> > clock (again)").
>> 
>> I'd naively expected that these would be more reliable after the 1:1 
>> mapping patches, so it might actually be time to give them another go.
> 
> Is there any value in that? Do machines exist where we absolutely must
> have access to the EFI time services? Either because there's no other
> method or no other working one?

Is it such a bad thing to be prepared for this sort of machine to
arrive even if likely there are none so far?

Jan

Re: [patch] x86/efi: use GFP_ATOMIC under spin_lock

[Ingo Molnar]

* Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 09.03.14 at 19:50, Matt Fleming <matt@console-pimps.org> wrote:
> > On Sun, 09 Mar, at 04:31:41PM, Matthew Garrett wrote:
> >> On Sun, Mar 09, 2014 at 04:20:20PM +0000, Matt Fleming wrote:
> >> 
> >> > We have tried to use the time functions before, with little success
> >> > because of various bugs in the runtime implementations, e.g. see commit
> >> > bacef661acdb ("x86-64/efi: Use EFI to deal with platform wall clock")
> >> > and commit bd52276fa1d4 ("x86-64/efi: Use EFI to deal with platform wall
> >> > clock (again)").
> >> 
> >> I'd naively expected that these would be more reliable after the 
> >> 1:1 mapping patches, so it might actually be time to give them 
> >> another go.
> > 
> > Is there any value in that? Do machines exist where we absolutely 
> > must have access to the EFI time services? Either because there's 
> > no other method or no other working one?
> 
> Is it such a bad thing to be prepared for this sort of machine to 
> arrive even if likely there are none so far?

"Be prepared for a not yet existing machine" != "time to give them 
another go on existing machines", right?

Thanks,

	Ingo

Re: [patch] x86/efi: use GFP_ATOMIC under spin_lock

[Jan Beulich]

>>> On 10.03.14 at 08:37, Ingo Molnar <mingo@kernel.org> wrote:

> * Jan Beulich <JBeulich@suse.com> wrote:
> 
>> >>> On 09.03.14 at 19:50, Matt Fleming <matt@console-pimps.org> wrote:
>> > On Sun, 09 Mar, at 04:31:41PM, Matthew Garrett wrote:
>> >> On Sun, Mar 09, 2014 at 04:20:20PM +0000, Matt Fleming wrote:
>> >> 
>> >> > We have tried to use the time functions before, with little success
>> >> > because of various bugs in the runtime implementations, e.g. see commit
>> >> > bacef661acdb ("x86-64/efi: Use EFI to deal with platform wall clock")
>> >> > and commit bd52276fa1d4 ("x86-64/efi: Use EFI to deal with platform wall
>> >> > clock (again)").
>> >> 
>> >> I'd naively expected that these would be more reliable after the 
>> >> 1:1 mapping patches, so it might actually be time to give them 
>> >> another go.
>> > 
>> > Is there any value in that? Do machines exist where we absolutely 
>> > must have access to the EFI time services? Either because there's 
>> > no other method or no other working one?
>> 
>> Is it such a bad thing to be prepared for this sort of machine to 
>> arrive even if likely there are none so far?
> 
> "Be prepared for a not yet existing machine" != "time to give them 
> another go on existing machines", right?

That heavily depends on the perspective you take, and I know
we tend to have disagreeing perspectives here: I think things
should be done the intended way on all systems where this is
possible. Remember the far from insignificant number of issues
with the ACPI interrupt source overrides for particularly the
timer interrupt years ago? Which wasn't a reason to stop trying
to use ACPI wherever possible, even on affected systems (by
introducing command line overrides).

Jan

Re: [patch] x86/efi: use GFP_ATOMIC under spin_lock

[Ingo Molnar]

* Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 10.03.14 at 08:37, Ingo Molnar <mingo@kernel.org> wrote:
> 
> > * Jan Beulich <JBeulich@suse.com> wrote:
> > 
> >> >>> On 09.03.14 at 19:50, Matt Fleming <matt@console-pimps.org> wrote:
> >> > On Sun, 09 Mar, at 04:31:41PM, Matthew Garrett wrote:
> >> >> On Sun, Mar 09, 2014 at 04:20:20PM +0000, Matt Fleming wrote:
> >> >> 
> >> >> > We have tried to use the time functions before, with little success
> >> >> > because of various bugs in the runtime implementations, e.g. see commit
> >> >> > bacef661acdb ("x86-64/efi: Use EFI to deal with platform wall clock")
> >> >> > and commit bd52276fa1d4 ("x86-64/efi: Use EFI to deal with platform wall
> >> >> > clock (again)").
> >> >> 
> >> >> I'd naively expected that these would be more reliable after the 
> >> >> 1:1 mapping patches, so it might actually be time to give them 
> >> >> another go.
> >> > 
> >> > Is there any value in that? Do machines exist where we absolutely 
> >> > must have access to the EFI time services? Either because there's 
> >> > no other method or no other working one?
> >> 
> >> Is it such a bad thing to be prepared for this sort of machine to 
> >> arrive even if likely there are none so far?
> > 
> > "Be prepared for a not yet existing machine" != "time to give them 
> > another go on existing machines", right?
> 
> That heavily depends on the perspective you take, [...]

The inequality I mention above is purely factual, so it does not 
depend on perspective.

> [...] and I know we tend to have disagreeing perspectives here: I 
> think things should be done the intended way on all systems where 
> this is possible. [...]

The problem is that on 99% of these systems the "intended way" at 
hardware design time was to run Windows and only Windows.

Firmware interfacing problems are a lot less severe on ARM, where 99% 
of the systems are intended to run Linux.

So it's all inflicted on us artificially, and when it comes to Windows 
hardware we have no choice but to act defensively, not in an 
optimistic, anticipatory fashion.

> [...] Remember the far from insignificant number of issues with the 
> ACPI interrupt source overrides for particularly the timer interrupt 
> years ago? Which wasn't a reason to stop trying to use ACPI wherever 
> possible, even on affected systems (by introducing command line 
> overrides).

That's an apples to oranges comparison IMO, the motivation for that 
was real on real systems: to get high(er) resolution time and 
interrupt sources.

What's the upside here, for existing systems? Please don't mistake me: 
if an upside exists I'm not against it, at all.

Thanks,

	Ingo

Re: [patch] x86/efi: use GFP_ATOMIC under spin_lock

[Jan Beulich]

>>> On 10.03.14 at 08:53, Ingo Molnar <mingo@kernel.org> wrote:
>> [...] Remember the far from insignificant number of issues with the 
>> ACPI interrupt source overrides for particularly the timer interrupt 
>> years ago? Which wasn't a reason to stop trying to use ACPI wherever 
>> possible, even on affected systems (by introducing command line 
>> overrides).
> 
> That's an apples to oranges comparison IMO, the motivation for that 
> was real on real systems: to get high(er) resolution time and 
> interrupt sources.

What has that got to do with getting higher resolution? IRQ0 is IRQ0,
irrespective whether driven by an 8254 or a HPET. The workaround
was needed to get IRQ0 to work properly _at all_.

> What's the upside here, for existing systems? Please don't mistake me: 
> if an upside exists I'm not against it, at all.

The upside isn't so much on existing systems that work with the
intended model, but on those that don't: Increasing awareness
and pressure on the firmware vendors' side to get their bugs
fixed. Just like - afaict - the ratio of systems with broken ACPI
tables has gone down over time.

Just to give an example from the Xen side: Xen uses the time
interface in UEFI, as you would expect not without problems. Apart
from issues with memory areas needed by those runtime calls not
being properly marked for runtime use (which the respective
vendors accepted they need to fix), we are also facing problems
with runtime calls using XMM registers. Rather than blindly saying
"the specification isn't precise on this, and e.g. Linux has a half
baked mechanism to deal with this, so let's just do so too" we're
pushing for the specification to get updated, such that it becomes
clear whether the firmware may do so and we need to change Xen,
or whether it's the firmware people needing to fix their
implementation. Doing it that way is cumbersome, yes, but I think
it is in the best interest of everybody involved (and better than
papering over bugs by avoiding certain functionality). And yes,
should such a machine reach customer hands, and should we get
reports that there's no way for them to boot it with Xen, we can
always add a command line override telling the hypervisor to
avoid the broken runtime service and use direct CMOS accesses
instead.

The approach you lobby - avoid the runtime service by default -
will just result in further delaying the fixing of the firmware bugs.

(The one real upside - getting time zone information - was
already mentioned by someone else on this thread.)

Jan

Re: [patch] x86/efi: use GFP_ATOMIC under spin_lock

[Matt Fleming]

On Mon, 10 Mar, at 08:22:47AM, Jan Beulich wrote:
> 
> The upside isn't so much on existing systems that work with the
> intended model, but on those that don't: Increasing awareness
> and pressure on the firmware vendors' side to get their bugs
> fixed. Just like - afaict - the ratio of systems with broken ACPI
> tables has gone down over time.
 
I'm not saying that we can never use the time functions. All I'm saying
is that until there are actaul in-kernel users let's delete the code.
Feel free to bring them back when someone actually needs them.

We're evaluating the tradeoff between having the code around for future
use, and the resultant maintenance burden whenever the EFI code gets
refactored. Clearly the code is bitrotting, which is the issue that
started this thread. No, it's not bitrotting very fast, and yes things
still build, but code can't bitrot at all if it doesn't exist.

I think we're all pretty much agreed that at the very least
phys_efi_get_time() can go in the bin. Whatever the outcome of this
thread that'll happen. The question is whether it makes sense to rip out
all of the time stuff in one go.

> Just to give an example from the Xen side: Xen uses the time
> interface in UEFI, as you would expect not without problems. Apart
> from issues with memory areas needed by those runtime calls not
> being properly marked for runtime use (which the respective
> vendors accepted they need to fix), we are also facing problems
> with runtime calls using XMM registers.

Presumably you've got some kind of quirk mechanism to get things
working? That doesn't exist in the kernel so the time functions as-is
are useless, right? What's the benefit of using the EFI time services
for Xen? Does Xen use the current kernel functions directly?

But this is good, here is a concrete use case.

Regarding the XMM updates...

> Rather than blindly saying "the specification isn't precise on this,
> and e.g. Linux has a half baked mechanism to deal with this, so let's
> just do so too" we're pushing for the specification to get updated,
> such that it becomes clear whether the firmware may do so and we need
> to change Xen, or whether it's the firmware people needing to fix
> their implementation.  Doing it that way is cumbersome, yes, but I
> think it is in the best interest of everybody involved (and better
> than papering over bugs by avoiding certain functionality).

Pushing for clarification in the spec is definitely a good idea, and I
commend you for doing that. But you're talking about fundamental calling
conventions here, you're not discussing peripheral services that have a
track record of bearly being tested at runtime and of arguably limited
value when things do work.

But we're getting off topic here.

> (The one real upside - getting time zone information - was
> already mentioned by someone else on this thread.)
 
We can easily do that from the EFI boot stub, we don't need runtime
services to do that. And even if we did need runtime services, someone
needs to show me the patches that use them.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [patch] x86/efi: use GFP_ATOMIC under spin_lock

[Jan Beulich]

>>> On 10.03.14 at 11:43, Matt Fleming <matt-HNK1S37rvNbeXh+fF434Mdi2O/JbrIOy@public.gmane.org> wrote:
> On Mon, 10 Mar, at 08:22:47AM, Jan Beulich wrote:
>> Just to give an example from the Xen side: Xen uses the time
>> interface in UEFI, as you would expect not without problems. Apart
>> from issues with memory areas needed by those runtime calls not
>> being properly marked for runtime use (which the respective
>> vendors accepted they need to fix), we are also facing problems
>> with runtime calls using XMM registers.
> 
> Presumably you've got some kind of quirk mechanism to get things
> working?

Not yet, since so far we haven't been shown issues with the code
on other than non-production systems.

> That doesn't exist in the kernel so the time functions as-is
> are useless, right?

Not sure why you call them useless. If wired up properly, they
would be useful on systems where they work.

> What's the benefit of using the EFI time services for Xen?

As said before: Don't suffer from there not being any wallclock
if there's no CMOS clock. With there even being a FADT flag to
indicate its absence, there clearly must be plans to have such
machines.

> Does Xen use the current kernel functions directly?

Which kernel functions? Perhaps just a misunderstanding - I
gave that example with the actual hypervisor in mind, not the
kernel side code sitting on top. The kernel, after all, can't
directly call EFI runtime code when running on top of Xen.

Jan

Re: [patch] x86/efi: use GFP_ATOMIC under spin_lock

[Matt Fleming]

On Mon, 10 Mar, at 11:05:10AM, Jan Beulich wrote:
> 
> Not sure why you call them useless. If wired up properly, they
> would be useful on systems where they work.
 
They're useless in the sense that they have no users in the kernel
currently.

> Which kernel functions? Perhaps just a misunderstanding - I
> gave that example with the actual hypervisor in mind, not the
> kernel side code sitting on top. The kernel, after all, can't
> directly call EFI runtime code when running on top of Xen.

OK, that's my bad, I misunderstood what you were saying.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [patch] x86/efi: use GFP_ATOMIC under spin_lock

[Matt Fleming]

On Mon, 10 Mar, at 10:43:28AM, Matt Fleming wrote:
> 
> I think we're all pretty much agreed that at the very least
> phys_efi_get_time() can go in the bin. Whatever the outcome of this
> thread that'll happen. The question is whether it makes sense to rip out
> all of the time stuff in one go.

OK, I'm chickening out of doing the full delete for v3.15. How about
something like this instead?

---

>From 697c512d44415794396773533715dff45785c585 Mon Sep 17 00:00:00 2001
From: Matt Fleming <matt.fleming@intel.com>
Date: Thu, 13 Mar 2014 19:41:08 +0000
Subject: [PATCH] x86/efi: Rip out phys_efi_get_time()

Dan reported that phys_efi_get_time() is doing kmalloc(..., GFP_KERNEL)
under a spinlock which is very clearly a bug. Since phys_efi_get_time()
has no users let's just delete it instead of trying to fix it.

Note that since there are no users of phys_efi_get_time(), it is not
possible to actually trigger a GFP_KERNEL alloc under the spinlock.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Nathan Zimmer <nzimmer@sgi.com>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>
Cc: Jan Beulich <JBeulich@suse.com>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
 arch/x86/platform/efi/efi.c | 29 -----------------------------
 1 file changed, 29 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 395decedfa2b..872214435e9d 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -256,21 +256,6 @@ static efi_status_t __init phys_efi_set_virtual_address_map(
 	return status;
 }
 
-static efi_status_t __init phys_efi_get_time(efi_time_t *tm,
-					     efi_time_cap_t *tc)
-{
-	unsigned long flags;
-	efi_status_t status;
-
-	spin_lock_irqsave(&rtc_lock, flags);
-	efi_call_phys_prelog();
-	status = efi_call_phys2(efi_phys.get_time, virt_to_phys(tm),
-				virt_to_phys(tc));
-	efi_call_phys_epilog();
-	spin_unlock_irqrestore(&rtc_lock, flags);
-	return status;
-}
-
 int efi_set_rtc_mmss(const struct timespec *now)
 {
 	unsigned long nowtime = now->tv_sec;
@@ -604,16 +589,9 @@ static int __init efi_runtime_init32(void)
 	 * EFI runtime services before set_virtual_address_map
 	 * is invoked.
 	 */
-	efi_phys.get_time = (efi_get_time_t *)
-			(unsigned long)runtime->get_time;
 	efi_phys.set_virtual_address_map =
 			(efi_set_virtual_address_map_t *)
 			(unsigned long)runtime->set_virtual_address_map;
-	/*
-	 * Make efi_get_time can be called before entering
-	 * virtual mode.
-	 */
-	efi.get_time = phys_efi_get_time;
 	early_iounmap(runtime, sizeof(efi_runtime_services_32_t));
 
 	return 0;
@@ -635,16 +613,9 @@ static int __init efi_runtime_init64(void)
 	 * EFI runtime services before set_virtual_address_map
 	 * is invoked.
 	 */
-	efi_phys.get_time = (efi_get_time_t *)
-			(unsigned long)runtime->get_time;
 	efi_phys.set_virtual_address_map =
 			(efi_set_virtual_address_map_t *)
 			(unsigned long)runtime->set_virtual_address_map;
-	/*
-	 * Make efi_get_time can be called before entering
-	 * virtual mode.
-	 */
-	efi.get_time = phys_efi_get_time;
 	early_iounmap(runtime, sizeof(efi_runtime_services_64_t));
 
 	return 0;
-- 
1.8.5.3

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [patch] x86/efi: use GFP_ATOMIC under spin_lock

[Matt Fleming]

On Mon, 10 Mar, at 07:27:50AM, Jan Beulich wrote:
> 
> Is it such a bad thing to be prepared for this sort of machine to
> arrive even if likely there are none so far?

I think it's a bad thing for us to carry code in the kernel that has no
users, yes.

If someone wants it in the future, they can pull it out of git.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [patch] x86/efi: use GFP_ATOMIC under spin_lock

[Jan Beulich]

>>> On 09.03.14 at 17:20, Matt Fleming <matt@console-pimps.org> wrote:
> On Fri, 07 Mar, at 03:25:37PM, Dan Carpenter wrote:
>> 
>> You're on your own for fixing the complicated stuff like layering
>> violations. I just do static checker stuff and sed fixes.  ;)
> 
> Thanks for the patch Dan. Nice catch.
> 
> But I'm wondering if we can simply delete phys_efi_get_time() to avoid
> the whole problem of doing GFP_KERNEL allocations under a spinlock.
> 
> In fact, the whole EFI time stuff is looking a bit crusty.
> 
> I only see two direct users of efi.get_time() outside of arch,
> 
>   drivers/char/efirtc.c
>   drivers/rtc/rtc-efi.c
> 
> which are both "depends on IA64".

And wrongly so. I continue to think that the fact that various
firmware implementations are broken should not make us by
default avoid using proper abstraction functionality whenever
possible. There's a reason behind that abstraction after all...

> For x86, all other callers are inside
> arch/x86/platform/efi/efi.c,
> 
>   efi_set_rtc_mmss()
>   efi_get_time()
> 
> neither of which have any callers - it all appears to be dead code. The
> diff stat of deleting all this dead code isn't too bad either,
> 
>  arch/x86/platform/efi/efi.c    | 151 ++---------------------------------------
>  arch/x86/platform/efi/efi_64.c |  90 ------------------------
>  2 files changed, 5 insertions(+), 236 deletions(-)
> 
> Thoughts?

Please don't, except perhaps for the phys_efi_get_time() removal.

Jan

Re: [patch] x86/efi: use GFP_ATOMIC under spin_lock

[Dan Carpenter]

On Sun, Mar 09, 2014 at 04:20:20PM +0000, Matt Fleming wrote:
> On Fri, 07 Mar, at 03:25:37PM, Dan Carpenter wrote:
> > 
> > You're on your own for fixing the complicated stuff like layering
> > violations. I just do static checker stuff and sed fixes.  ;)
> 
> Thanks for the patch Dan. Nice catch.
> 
> But I'm wondering if we can simply delete phys_efi_get_time() to avoid
> the whole problem of doing GFP_KERNEL allocations under a spinlock.
> 
> In fact, the whole EFI time stuff is looking a bit crusty.
> 
> I only see two direct users of efi.get_time() outside of arch,
> 
>   drivers/char/efirtc.c
>   drivers/rtc/rtc-efi.c
> 
> which are both "depends on IA64". For x86, all other callers are inside
> arch/x86/platform/efi/efi.c,
> 
>   efi_set_rtc_mmss()
>   efi_get_time()
> 
> neither of which have any callers - it all appears to be dead code. The
> diff stat of deleting all this dead code isn't too bad either,
> 
>  arch/x86/platform/efi/efi.c    | 151 ++---------------------------------------
>  arch/x86/platform/efi/efi_64.c |  90 ------------------------
>  2 files changed, 5 insertions(+), 236 deletions(-)
> 
> Thoughts?

I don't have strong opinions on this, this was just a static checker
thing, and I don't know the code.

regards,
dan carpenter

Re: [patch] x86/efi: use GFP_ATOMIC under spin_lock

[Dan Carpenter]

On Fri, Mar 07, 2014 at 02:20:56PM +0300, Dan Carpenter wrote:
> In phys_efi_get_time() we call efi_call_phys_prelog() with a spin_lock
> so this allocation should be atomic.
> 
> Fixes: b8f2c21db390 ('efi, x86: Pass a proper identity mapping in efi_call_phys_prelog')
> Signed-off-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> 
> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index 0c2a234fef1e..f5adcadb381b 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -90,7 +90,7 @@ void __init efi_call_phys_prelog(void)
>  	local_irq_save(efi_flags);
>  
>  	n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT), PGDIR_SIZE);
> -	save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_KERNEL);
> +	save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_ATOMIC);

"save_pgd" is a file scope variable.  It wouldn't be that hard to move
the allocation to a different function.  Meanwhile I am on the way to
the airport for vacation.

See you guys in couple weeks.

regards,
dan carpenter

Re: [patch] x86/efi: use GFP_ATOMIC under spin_lock

[H. Peter Anvin]

On 03/07/2014 03:20 AM, Dan Carpenter wrote:
> In phys_efi_get_time() we call efi_call_phys_prelog() with a spin_lock
> so this allocation should be atomic.
> 
> Fixes: b8f2c21db390 ('efi, x86: Pass a proper identity mapping in efi_call_phys_prelog')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index 0c2a234fef1e..f5adcadb381b 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -90,7 +90,7 @@ void __init efi_call_phys_prelog(void)
>  	local_irq_save(efi_flags);
>  
>  	n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT), PGDIR_SIZE);
> -	save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_KERNEL);
> +	save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_ATOMIC);
>  
>  	for (pgd = 0; pgd < n_pgds; pgd++) {
>  		save_pgd[pgd] = *pgd_offset_k(pgd * PGDIR_SIZE);
> 

And what happens if the allocation fails, as GFP_ATOMIC allocations are
wont to do?  I see an inbound NULL pointer reference from miles away here...

	-hpa