Re: [PATCH] ACPI: Add _PDC object evaluate for VIA.

[louisqi <louisqi@viatech.com.cn>]

Dear Brown & Pallipadi:

I want to know whether there are some problems about this patch.

If it does, please kindly help me to point out, I will revise it.

Waiting for your reply!

Thanks a lot!


Louis

2008.10.08

///////////////////////////////////////////////////////
louisqi have wrote:
> 
> Thanks a lot, Ingo! Thanks for your kindly help.
> 
> I have revise my patch as following:
> 
> 
> //////////////////////////////////////////////////////////////////////////////////// 
> 
> [PATCH] ACPI: Add _PDC object evaluate for VIA.
> 
> Frome: Louis Qi <louisqi@viatech.com.cn>
> 
> The current routine "arch_acpi_processor_init_pdc" only supports INTEL 
> CPUs.
> This patch add support for VIA CPUs.
> 
> Signed-off-by: Louis Qi <louisqi@viatech.com.cn>
> ---
> 
> --- 2.6.26.5/arch/x86/kernel/acpi/processor.c.orig    2008-09-28 
> 10:18:11.000000000 +0800
> +++ 2.6.26.5/arch/x86/kernel/acpi/processor.c    2008-09-28 
> 10:18:11.000000000 +0800
> @@ -66,6 +66,53 @@ static void init_intel_pdc(struct acpi_p
>      return;
>  }
> 
> +static void init_via_pdc(struct acpi_processor *pr, struct cpuinfo_x86 *c)
> +{
> +    struct acpi_object_list *obj_list;
> +    union acpi_object *obj;
> +    u32 *buf;
> +
> +    /* allocate and initialize pdc. It will be used later. */
> +    obj_list = kmalloc(sizeof(struct acpi_object_list), GFP_KERNEL);
> +    if (!obj_list)
> +        goto err_obj_list;
> +
> +    obj = kmalloc(sizeof(union acpi_object), GFP_KERNEL);
> +    if (!obj)
> +        goto err_obj;
> +
> +    buf = kmalloc(12, GFP_KERNEL);
> +    if (!buf)
> +        goto err_buf;
> +
> +    buf[0] = ACPI_PDC_REVISION_ID;
> +    buf[1] = 1;
> +    buf[2] = 0;
> +
> +    if (cpu_has(c, X86_FEATURE_EST))
> +        buf[2] |= ACPI_PDC_P_FFH;
> +
> +    if (cpu_has(c, X86_FEATURE_ACPI))
> +        buf[2] |= ACPI_PDC_T_FFH;
> +
> +    obj->type = ACPI_TYPE_BUFFER;
> +    obj->buffer.length = 12;
> +    obj->buffer.pointer = (u8 *)buf;
> +    obj_list->count = 1;
> +    obj_list->pointer = obj;
> +    pr->pdc = obj_list;
> +
> +    return;
> +
> +err_buf:
> +    kfree(obj);
> +err_obj:
> +    kfree(obj_list);
> +err_obj_list:
> +    printk(KERN_ERR "init_via_pdc: memory allocation failed\n");
> +}
> +
> +
>  /* Initialize _PDC data based on the CPU vendor */
>  void arch_acpi_processor_init_pdc(struct acpi_processor *pr)
>  {
> @@ -74,6 +121,8 @@ void arch_acpi_processor_init_pdc(struct
>      pr->pdc = NULL;
>      if (c->x86_vendor == X86_VENDOR_INTEL)
>          init_intel_pdc(pr, c);
> +    else if (c->x86_vendor == X86_VENDOR_CENTAUR)
> +        init_via_pdc(pr, c);
> 
>      return;
>  }
> //////////////////////////////////////////////////////////////////////////////////// 
> 
> 
> Ingo Molnar :
>> just a few minor comments:
>>
>> * louisqi <louisqi@viatech.com.cn> wrote:
>>
>>> +static void init_via_pdc(struct acpi_processor *pr, struct 
>>> cpuinfo_x86 *c)
>>> +{
>>> +    struct acpi_object_list *obj_list;
>>> +    union acpi_object *obj;
>>> +    u32 *buf;
>>> +
>>> +    /* allocate and initialize pdc. It will be used later. */
>>> +    obj_list = kmalloc(sizeof(struct acpi_object_list), GFP_KERNEL);
>>> +    if (!obj_list) {
>>> +        printk(KERN_ERR "Memory allocation error\n");
>>> +        return;
>>> +    }
>>> +
>>> +    obj = kmalloc(sizeof(union acpi_object), GFP_KERNEL);
>>> +    if (!obj) {
>>> +        printk(KERN_ERR "Memory allocation error\n");
>>> +        kfree(obj_list);
>>> +        return;
>>> +    }
>>> +
>>> +    buf = kmalloc(12, GFP_KERNEL);
>>> +    if (!buf) {
>>> +        printk(KERN_ERR "Memory allocation error\n");
>>> +        kfree(obj);
>>> +        kfree(obj_list);
>>> +        return;
>>> +    }
>>
>> error condition cleanliness: it's cleaner to create a single off-line 
>> instance of freeing logic, at the end of the function:
>>
>> ...
>>     [ ... normal path ... ]
>>     return;
>>
>> err_obj:
>>     kfree(obj);
>> err_obj_list:
>>     kfree(obj_list);
>> err:
>>     printk(KERN_ERR "init_via_pdc: memory allocation error\n");
>> }
>>
>> and then after the allocation do:
>>
>>     if (!obj_list)
>>         goto err;
>>
>> etc.
>>
>> (also note the different style of the printk)
>>
>>> +
>>> +    buf[0] = ACPI_PDC_REVISION_ID;
>>> +    buf[1] = 1;
>>> +    buf[2] = 0;
>>> +
>>> +    if (cpu_has(c, X86_FEATURE_EST))
>>> +        buf[2] |= ACPI_PDC_P_FFH;
>>> +
>>> +    if (cpu_has(c, X86_FEATURE_ACPI))
>>> +        buf[2] |= ACPI_PDC_T_FFH;
>>> +
>>> +    obj->type = ACPI_TYPE_BUFFER;
>>> +    obj->buffer.length = 12;
>>> +    obj->buffer.pointer = (u8 *) buf;
>>
>> style: no need to put a space between the '(u8 *)' and 'buf'.
>>
>>> +    return;
>>> +}
>>
>> style: no need to put 'return' at the end of a void function.
>>
>>     Ingo
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
>