lkml.org 
[lkml]   [2008]   [Dec]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: 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?


    \
     
     \ /
      Last update: 2008-12-16 06:45    [W:0.033 / U:29.344 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site