Messages in this thread | | | Date | Mon, 15 Dec 2008 21:41:40 -0800 | From | Andrew Morton <> | Subject | Re: acpi_os_allocate(GFP_KERNEL) (was Re: acpi_evaluate_integer broken by design) |
| |
On Tue, 16 Dec 2008 00:24:43 -0500 (EST) Len Brown <lenb@kernel.org> wrote:
> > Obviously, it is even better to do neither. It is a basic and > > oft-reoccurring design mistake for a low-level piece of code to > > hardwire its GFP_foo decision. The gfp_t should be passed in from the > > callers, all the way down the chain from the top-level code which > > actually knows what state this thread of control is in. > > Here the caller does not know.
I bet it does it's just that the caller is thirty five levels of function call higher up...
> The crux of the problem it that the AML interpreter > may need to allocate memory and thus may sleep. > Under nominal conditions the interpreter only runs in > user-context and allocatges with GFP_KERNEL > so sleeping is just dandy. > > But AML also needs to run at boot time and at resume time > to do things like configure interrupts before interrupts are > enabled... > > boot-time is handled by _cond_resched() > not calling __cond_resched unless > system_state == SYSTEM_RUNNING > > I suppose I could do something like this, > though I'm not sure of the prettiest place > to check system_resume_irqsoff == true, > so that is left out for now... > > > ... > > diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c > index e52ad91..def91fa 100644 > --- a/drivers/acpi/pci_link.c > +++ b/drivers/acpi/pci_link.c > @@ -320,7 +320,7 @@ static int acpi_pci_link_set(struct acpi_pci_link *link, int irq) > if (!link || !irq) > return -EINVAL; > > - resource = kzalloc(sizeof(*resource) + 1, irqs_disabled() ? GFP_ATOMIC: GFP_KERNEL); > + resource = kzalloc(sizeof(*resource) + 1, GFP_KERNEL); > if (!resource) > return -ENOMEM; > > diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c > index bb7d50d..27243f9 100644 > --- a/drivers/acpi/power.c > +++ b/drivers/acpi/power.c > @@ -218,8 +218,7 @@ static int acpi_power_on(acpi_handle handle, struct acpi_device *dev) > } > > if (!found) { > - ref = kmalloc(sizeof (struct acpi_power_reference), > - irqs_disabled() ? GFP_ATOMIC : GFP_KERNEL); > + ref = kmalloc(sizeof (struct acpi_power_reference), GFP_KERNEL); > if (!ref) { > ACPI_DEBUG_PRINT((ACPI_DB_INFO, "kmalloc() failed\n")); > mutex_unlock(&resource->resource_lock); > diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h > index 029c8c0..9fbf73f 100644 > --- a/include/acpi/platform/aclinux.h > +++ b/include/acpi/platform/aclinux.h > @@ -121,17 +121,16 @@ static inline acpi_thread_id acpi_os_get_thread_id(void) > #include <acpi/actypes.h> > static inline void *acpi_os_allocate(acpi_size size) > { > - return kmalloc(size, irqs_disabled()? GFP_ATOMIC : GFP_KERNEL); > + return kmalloc(size, GFP_KERNEL); > } > static inline void *acpi_os_allocate_zeroed(acpi_size size) > { > - return kzalloc(size, irqs_disabled()? GFP_ATOMIC : GFP_KERNEL); > + return kzalloc(size, GFP_KERNEL); > } > > static inline void *acpi_os_acquire_object(acpi_cache_t * cache) > { > - return kmem_cache_zalloc(cache, > - irqs_disabled()? GFP_ATOMIC : GFP_KERNEL); > + return kmem_cache_zalloc(cache, GFP_KERNEL); > } > > #define ACPI_ALLOCATE(a) acpi_os_allocate(a) > diff --git a/include/linux/suspend.h b/include/linux/suspend.h > index 2ce8207..a716de8 100644 > --- a/include/linux/suspend.h > +++ b/include/linux/suspend.h > @@ -128,9 +128,16 @@ extern void arch_suspend_disable_irqs(void); > */ > extern void arch_suspend_enable_irqs(void); > > +/** > + * suspend_irqs_off > + * > + * flag to indicate that IRQs are off for the benefit of suspend > + */ > +extern bool system_suspend_irqs_off; > extern int pm_suspend(suspend_state_t state); > #else /* !CONFIG_SUSPEND */ > #define suspend_valid_only_mem NULL > +#define system_suspend_irqs_off false > > static inline void suspend_set_ops(struct platform_suspend_ops *ops) {} > static inline int pm_suspend(suspend_state_t state) { return -ENOSYS; } > diff --git a/kernel/power/main.c b/kernel/power/main.c > index b8f7ce9..2eadae7 100644 > --- a/kernel/power/main.c > +++ b/kernel/power/main.c > @@ -268,16 +268,20 @@ static int suspend_prepare(void) > return error; > } > > +bool system_resume_irqsoff; /* IRQs are off for resume */
I guess that was supposed to be "system_suspend_irqs_off"?
> /* default implementation */ > void __attribute__ ((weak)) arch_suspend_disable_irqs(void) > { > local_irq_disable(); > + system_resume_irqsoff = true; > } > > /* default implementation */ > void __attribute__ ((weak)) arch_suspend_enable_irqs(void) > { > local_irq_enable(); > + system_resume_irqsoff = false; > }
(please use __weak)
Whether it's "system_resume_irqsoff" or "system_suspend_irqs_off", neither of them actually got used ;)
If the code is really this simple and localised then perhaps:
gfp_t acpi_aml_gfp_flags = GFP_ATOMIC;
void __weak arch_suspend_disable_irqs(void) { local_irq_disable(); acpi_aml_gfp_flags = GFP_ATOMIC; }
/* default implementation */ void __weak arch_suspend_enable_irqs(void) { local_irq_enable(); acpi_aml_gfp_flags = GFP_KERNEL; }
would suffice. Dunno.
Why are we providing for an arch override of this?
| |