Re: [PATCH 06/10] ACPI: call acpi_debug_init() explicitly rather than as initcall

[Yinghai Lu <yinghai@kernel.org>]

Bjorn Helgaas wrote:
> On Tuesday 24 March 2009 05:20:44 pm Yinghai Lu wrote:
>> Bjorn Helgaas wrote:
>>> On Tuesday 24 March 2009 05:08:12 pm Yinghai Lu wrote:
>>>> On Tue, Mar 24, 2009 at 3:50 PM, Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
>>>>> This patch makes acpi_init() call acpi_debug_init() directly.
>>>>> Previously, both were subsys_initcalls.  acpi_debug_init()
>>>>> must happen after acpi_init(), and it's better to call it
>>>>> explicitly rather than rely on link ordering.
>>>>>
>>>>> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
>>>>> ---
>>>>>  drivers/acpi/bus.c      |    1 +
>>>>>  drivers/acpi/debug.c    |   14 ++++++--------
>>>>>  drivers/acpi/internal.h |    6 ++++++
>>>>>  3 files changed, 13 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>>>>> index c133072..f32cfd6 100644
>>>>> --- a/drivers/acpi/bus.c
>>>>> +++ b/drivers/acpi/bus.c
>>>>> @@ -883,6 +883,7 @@ static int __init acpi_init(void)
>>>>>        acpi_ec_init();
>>>>>        acpi_power_init();
>>>>>        acpi_system_init();
>>>>> +       acpi_debug_init();
>>>>>        return result;
>>>>>  }
>>>>>
>>>>> diff --git a/drivers/acpi/debug.c b/drivers/acpi/debug.c
>>>>> index 20223cb..9cb189f 100644
>>>>> --- a/drivers/acpi/debug.c
>>>>> +++ b/drivers/acpi/debug.c
>>>>> @@ -297,17 +297,15 @@ acpi_system_write_debug(struct file *file,
>>>>>
>>>>>        return count;
>>>>>  }
>>>>> +#endif
>>>>>
>>>>> -static int __init acpi_debug_init(void)
>>>>> +int __init acpi_debug_init(void)
>>>>>  {
>>>>> +#ifdef CONFIG_ACPI_PROCFS
>>>>>        struct proc_dir_entry *entry;
>>>>>        int error = 0;
>>>>>        char *name;
>>>>>
>>>>> -
>>>>> -       if (acpi_disabled)
>>>>> -               return 0;
>>>>> -
>>>>>        /* 'debug_layer' [R/W] */
>>>>>        name = ACPI_SYSTEM_FILE_DEBUG_LAYER;
>>>>>        entry =
>>>>> @@ -338,7 +336,7 @@ static int __init acpi_debug_init(void)
>>>>>        remove_proc_entry(ACPI_SYSTEM_FILE_DEBUG_LAYER, acpi_root_dir);
>>>>>        error = -ENODEV;
>>>>>        goto Done;
>>>>> -}
>>>>> -
>>>>> -subsys_initcall(acpi_debug_init);
>>>>> +#else
>>>>> +       return 0;
>>>>>  #endif
>>>>> +}
>>>>> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
>>>>> index 4a35f6e..44b8402 100644
>>>>> --- a/drivers/acpi/internal.h
>>>>> +++ b/drivers/acpi/internal.h
>>>>> @@ -3,6 +3,12 @@
>>>>>  int acpi_scan_init(void);
>>>>>  int acpi_system_init(void);
>>>>>
>>>>> +#ifdef CONFIG_ACPI_DEBUG
>>>> ==> #if defined(CONFIG_ACPI_DEBUG) && defined(CONFIG_ACPI_PROCFS)
>>> I could do that, and leave the #ifdefs in debug.c as they were,
>>> but I thought it was cleaner to make it so that if we compile debug.c
>>> (i.e., CONFIG_ACPI_DEBUG=y), it always provides acpi_debug_init().
>>>
>>> I moved the #ifdefs in debug.c so that acpi_debug_init() is a no-op
>>> if CONFIG_ACPI_PROCFS=n.
>>>
>>> So I think my patch already addressed your concern, but let me
>>> know if not.
>> you had two copy
>> +#else
>>>> +       return 0;
>>>>  #endif
>> ...
>>
>> with
>> #if defined(CONFIG_ACPI_DEBUG) && defined(CONFIG_ACPI_PROCFS) 
>> in .h
>>
>> you only need to do
>>>>> -
>>>>> -       if (acpi_disabled)
>>>>> -               return 0;
>>>>> -
>>>>>        /* 'debug_layer' [R/W] */
>>>>>        name = ACPI_SYSTEM_FILE_DEBUG_LAYER;
>>>>>        entry =
>>>>> @@ -338,7 +336,7 @@ static int __init acpi_debug_init(void)
>>>>>        remove_proc_entry(ACPI_SYSTEM_FILE_DEBUG_LAYER, acpi_root_dir);
>>>>>        error = -ENODEV;
>>>>>        goto Done;
>>>>> -}
>>>>> -
>>>>> -subsys_initcall(acpi_debug_init);
>> in debug.c
>>
>> totally you will have less one #ifdef
> 
> You're right that I have two "#ifdef CONFIG_ACPI_PROCFS" in debug.c,
> and I could get away with only one if I used
>     #if defined(CONFIG_ACPI_DEBUG) && defined(CONFIG_ACPI_PROCFS)
> in internal.h.  I actually did that in my first version of the patch.
> 
> However, I thought it was a bit ugly to put the CONFIG_ACPI_PROCFS
> stuff in internal.h.  That would mean a reader of internal.h has
> to know about the details of how debug.c is implemented.  It is
> completely non-obvious why a definition of acpi_debug_init() should
> depend on CONFIG_ACPI_PROCFS, so the reader would have to go dig
> through debug.c to figure it out.  With my patch, the reader only
> has to know "CONFIG_ACPI_DEBUG enables the build of debug.c."
> 
> If I understand you correctly, you're raising a style issue, and
> there's no functional problem either way.  Right?

besides that, some last_calls are merged to direct call.
wonder if those calling could depend on pci_acpi_init etc.

YH