Re: [RFC][PATCH] ACPI: Ignore invalid _PSS entries, but use valid ones

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, May 04, 2012 at 10:46:01AM -0300, Marco Aurelio da Costa wrote:
> From: Marco Aurelio da Costa <costa@gamic.com>
> Signed-off-by: Marco Aurelio da Costa <costa@gamic.com>
> 
> The EliteBook 8560W has non-initialized entries in its _PSS ACPI
> table. Instead of bailing out when the first non-initialized entry is
> found, ignore it and use only  the valid entries. Only bail out if there
> is no valid entry at all.

Is that safe? Meaning re-use the other CPU's _PSS states? Perhaps the
warning at the end should say: "Trying to compensate by using the
other CPU's PSS state).

> 
> ---
> --- linux-3.3.3/drivers/acpi/processor_perflib.c.orig   2012-04-24
> 22:18:23.288041268 +0200
> +++ linux-3.3.3/drivers/acpi/processor_perflib.c        2012-04-24
> 22:19:25.912042603 +0200
> @@ -311,6 +311,7 @@ static int acpi_processor_get_performanc
>        struct acpi_buffer state = { 0, NULL };
>        union acpi_object *pss = NULL;
>        int i;
> +       int last_invalid = -1;
> 
> 
>        status = acpi_evaluate_object(pr->handle, "_PSS", NULL, &buffer);
> @@ -374,12 +375,30 @@ static int acpi_processor_get_performanc
>                        printk(KERN_ERR FW_BUG PREFIX
>                               "Invalid BIOS _PSS frequency: 0x%llx MHz\n",
>                               px->core_frequency);
> -                       result = -EFAULT;
> -                       kfree(pr->performance->states);
> -                       goto end;
> +                       if (-1 == last_invalid)

Swap it around or just do it this way:

if (last_invalid < 0)

> +                               last_invalid = i;
> +               } else {
> +                       if (last_invalid != -1) {

if (last_invalid >= 0)

> +                               /*
> +                                * Copy this valid entry over last_invalid entry
> +                                */
> +                               memcpy(&(pr->performance->states[last_invalid]),
> +                                      px, sizeof(struct acpi_processor_px));
> +                               ++last_invalid;
> +                       }
>                }
>        }
> 
> +       if (0 == last_invalid) {

So if _PSS that is missing is at CPU2, this own't print it.

I think you want 'if (last_invalid >= 0)'

> +               printk(KERN_ERR FW_BUG PREFIX
> +                      "No valid BIOS _PSS frequency found\n");

And you should mention which CPU has it busted - as there are
some that are working.


> +               result = -EFAULT;
> +               kfree(pr->performance->states);
> +       }
> +
> +       if (last_invalid > 0)

Don't you want 'last_invalid >= 0' ?

> +               pr->performance->state_count = last_invalid;
> +
>       end:
>        kfree(buffer.pointer);
> --
> 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