Re: [PATCH] ACPI: add "auto" to acpi_enforce_resources

[Luca Tettamanti <kronos.it@gmail.com>]

On Mon, Feb 2, 2009 at 10:11 AM, Jean Delvare <khali@linux-fr.org> wrote:
> On Sun, 1 Feb 2009 22:22:43 +0100, Luca Tettamanti wrote:
>> --- a/drivers/acpi/osl.c
>> +++ b/drivers/acpi/osl.c
>> @@ -1063,7 +1063,10 @@ __setup("acpi_wake_gpes_always_on", acpi_wake_gpes_always_on_setup);
>>   * in arbitrary AML code and can interfere with legacy drivers.
>>   * acpi_enforce_resources= can be set to:
>>   *
>> - *   - strict           (2)
>> + *   - auto             (2)
>> + *     -> detect possible conflicts with ACPI drivers and switch to
>> + *     strict if needed, otherwise act like lax
>> + *   - strict           (3)
>>   *     -> further driver trying to access the resources will not load
>>   *   - lax (default)    (1)
>>   *     -> further driver trying to access the resources will load, but you
>> @@ -1073,11 +1076,12 @@ __setup("acpi_wake_gpes_always_on", acpi_wake_gpes_always_on_setup);
>>   *     -> ACPI Operation Region resources will not be registered
>>   *
>>   */
>> -#define ENFORCE_RESOURCES_STRICT 2
>> +#define ENFORCE_RESOURCES_STRICT 3
>> +#define ENFORCE_RESOURCES_AUTO   2
>>  #define ENFORCE_RESOURCES_LAX    1
>>  #define ENFORCE_RESOURCES_NO     0
>
> I don't see any reason to change ENFORCE_RESOURCES_STRICT from 2 to 3.
> Just add ENFORCE_RESOURCES_AUTO as 3 and that's it, makes your patch
> smaller.

There's an unspoken reason ;-) The options are ordered by
"strictness", I was experimenting with an API to export the parameter,
in order to move the code to a separate quirk file. Since it's not
relevant in this patch I'll back out the change.

>> +static int acpi_res_quick_check_hid(const struct dmi_system_id *d)
>> +{
>> +     acpi_status ret;
>> +     bool found = false;
>> +     char *hid;
>> +
>> +     if (!d->driver_data)
>> +             goto strict;
>> +
>> +     hid = d->driver_data;
>> +     ret = acpi_get_devices(hid, acpi_res_quick_check_hid_cb,
>> +                             NULL, (void **)&found);
>> +
>> +     if (ret == AE_OK && found)
>> +             goto strict;
>> +
>> +     return 0;
>> +strict:
>> +     printk(KERN_DEBUG "ACPI: detected %s system: "
>> +                     "enforcing strict resource checking\n", d->ident);
>
> Should be pr_debug().

Actually I was considering raising the level to info, it might make
easier to explain to the users why they cannot load their favorite
driver anymore.

>> +     return 1;
>> +}
>> +
>> +/* The following systems have ACPI drivers that might overlap the
>> + * functionality of native drivers (mostly in hwmon subsys). If
>> + * acpi_enforce_resources is set to auto the following table is
>> + * used to enforce strict checking in matching systems, in order
>> + * to avoid conflicts.
>> + * Note that driver_data and the acpi_res_quick_check_hid callback
>> + * are used to further refine the match, checking for the presence
>> + * of the given HID (driver_data) in the DSDT. Both fields are
>> + * optional for DMI-only rules.
>> + */
>> +static const struct dmi_system_id resource_quirks[] = {
>
> You must include <linux/dmi.h> for this.

Right, it's included indirectly from somewhere...

> I think it's better to tag this structure as __initdata than const, so
> that it is cleaned up after initialization. Then I guess you can also
> tag acpi_res_quick_check_hid_cb() and acpi_res_quick_check_hid() as
> __init so the runtime memory overhead is zero.

Ok, will do.

Luca