[PATCH 0/3] ACPI autoloading

[PATCH 0/3] ACPI autoloading

[Thomas Renninger]

[-- Attachment #1: Type: text/plain, Size: 3477 bytes --]

This patch series enables ACPI modules to get loaded automatically via
module aliases.

Thanks to Kay I got help with the udev strings and this got already some
testing and review.

There are some minor problems left, but this should be mm ready and it
would be great if you can add these to your acpi-test tree, Len (patches
clean against 2.6.22-rc4, the last patch has some offset/fuzz against
acpi-test, but should patch correctly without modifications).

The known problems are:


A) Cannot pass acpi_device_id through ops.add (probe equivalent func)
---------------------------------------------------------------------

int acpi_match_device_ids(..)
should be:
const acpi_device_id *acpi_match_device_ids
and the matching device id should get passed to the acpi driver's .add
function.
This would ease up the code in the ACPI driver a lot (e.g. see button.c,
it is compared again which device (HID) has been found. Theoretically
the drivers also needs to check CIDs...).

The problem is that in scan.c ACPI driver is abstracted to use .match
and .probe "struct bus_type" functions and I have no idea how the
matching acpi_device_id should get passed or stored from
acpi_match_device_ids (.match) to ops.add (invoked in .probe).
Currently:
        kernel_ulong_t driver_data;
of acpi_device_id got added, but is not used at all atm.


B) Thermal module always gets loaded
------------------------------------

This is because _TZ_ (scope?!?) always gets added, but is declared as a
device or at least pops up as a thermal device.
This is in drivers/acpi/utilities/utglobal.c:

/*
 * Predefined ACPI Names (Built-in to the Interpreter)
 *
 * NOTES:
 * 1) _SB_ is defined to be a device to allow \_SB_._INI to be run
 *    during the initialization sequence.
 * 2) _TZ_ is defined to be a thermal zone in order to allow ASL code to
 *    perform a Notify() operation on it.
 */

Any idea how to get rid of that is very welcome.
Is this to be able to process buggy ASL code of this kind?:
"Notify ("_TZ_", 0x80)"
While _TZ_ is not a real device, but a scope operator and gets
statically declared as a device here to solve such things?


C) Renaming/Unifying of HID strings
-----------------------------------

This shouldn't be a problem. Consequence is that the new ACPI sysfs
structure will show other names, but as it got introduced recently this
shouldn't hurt anyone?

For better readability we could map all HIDs to a device name through a
HID <-> Device Name - enum and table. And then export "battery" again
through /sys instead of "PNP0C0A".
This was a bit mixed up. If, this should get implemented soon as the
sysfs structure should not get altered that often...
If wanted, I can send an on top patch later...

D) Why renaming struct acpi_device_id to struct __acpi_device_id

Not a problem, just for understanding why I have done this:
struct acpi_device_id already exists in ACPICA. The module aliasing
interface requires that struct "SUBSYSTEM"_device_id is used as name.
This is not that obvious (include/linux/module.h):
#define MODULE_GENERIC_TABLE(gtype,name) and
#define MODULE_DEVICE_TABLE(type,name)

-------------------------


For SUSE users who want to test this, you also need a minor change in
userspace (AFAIK this additionally abstraction vanishes soon...
hopefully), the change is attached. Theoretically ACPI modules should
now get autoloaded via udev out of the box...

Any comments, corrections, ideas, etc. very welcome.

Thanks,

     Thomas

[-- Attachment #2: acpi_autoload_userspace_suse.patch --]
[-- Type: text/x-patch, Size: 968 bytes --]

Index: /etc/udev/rules.d/80-sysconfig.rules
===================================================================
--- .orig/etc/udev/rules.d/80-sysconfig.rules
+++ /etc/udev/rules.d/80-sysconfig.rules
@@ -15,5 +15,6 @@ SUBSYSTEM=="pnp", ACTION=="add", RUN+="/
 SUBSYSTEM=="ide", ACTION=="add", RUN+="/sbin/hwup ide-devpath-%p -o hotplug"
 SUBSYSTEM=="input", KERNEL=="input[0-9]*", ACTION=="add", RUN+="/sbin/hwup input-devpath-%p -o hotplug"
 SUBSYSTEM=="platform", ACTION=="add", RUN+="/sbin/hwup platform-devpath-%p -o hotplug"
+SUBSYSTEM=="acpi", ACTION=="add", RUN+="/sbin/hwup acpi-devpath-%p -o hotplug"
 
 LABEL="sysconfig_end"
Index: /sbin/hwup
===================================================================
--- .orig/sbin/hwup
+++ /sbin/hwup
@@ -65,6 +65,7 @@ get_config_getcfg() {
 
 is_known_subsystem() {
 	case "$1" in
+		acpi)             return 0 ;;
 		ccw)              return 0 ;;
 		ccwgroup)         return 0 ;;
 		iucv)             return 0 ;;

Re: [PATCH 0/3] ACPI autoloading

[Len Brown]

> A) Cannot pass acpi_device_id through ops.add (probe equivalent func)
> ---------------------------------------------------------------------
> 
> int acpi_match_device_ids(..)
> should be:
> const acpi_device_id *acpi_match_device_ids
> and the matching device id should get passed to the acpi driver's .add
> function.
> This would ease up the code in the ACPI driver a lot (e.g. see button.c,
> it is compared again which device (HID) has been found. Theoretically
> the drivers also needs to check CIDs...).
> 
> The problem is that in scan.c ACPI driver is abstracted to use .match
> and .probe "struct bus_type" functions and I have no idea how the
> matching acpi_device_id should get passed or stored from
> acpi_match_device_ids (.match) to ops.add (invoked in .probe).
> Currently:
>         kernel_ulong_t driver_data;
> of acpi_device_id got added, but is not used at all atm.
>
> B) Thermal module always gets loaded
> ------------------------------------
> 
> This is because _TZ_ (scope?!?) always gets added, but is declared as a
> device or at least pops up as a thermal device.
> This is in drivers/acpi/utilities/utglobal.c:
> 
> /*
>  * Predefined ACPI Names (Built-in to the Interpreter)
>  *
>  * NOTES:
>  * 1) _SB_ is defined to be a device to allow \_SB_._INI to be run
>  *    during the initialization sequence.
>  * 2) _TZ_ is defined to be a thermal zone in order to allow ASL code to
>  *    perform a Notify() operation on it.
>  */
> 
> Any idea how to get rid of that is very welcome.
> Is this to be able to process buggy ASL code of this kind?:
> "Notify ("_TZ_", 0x80)"
> While _TZ_ is not a real device, but a scope operator and gets
> statically declared as a device here to solve such things?

ThermalZone objects hang under _TZ, whether TZ it be global, or under _SB
I would think that if there are no ThermalZone objects,
then we'd not need to bind the thermal driver.

However, this would be a custom binding method (like acpi video.c)
rather than a HID based one.

I'm really not sure about the _TZ_ syntax -- we'll have to ask Bob
where that came from when he returns -- but I don't think it matters
for the issue at hand.

> C) Renaming/Unifying of HID strings
> -----------------------------------
> 
> This shouldn't be a problem. Consequence is that the new ACPI sysfs
> structure will show other names, but as it got introduced recently this
> shouldn't hurt anyone?

I agree that it is still new and that it is unlikely that a re-name
now would be a significant compatibility issue.

> For better readability we could map all HIDs to a device name through a
> HID <-> Device Name - enum and table. And then export "battery" again
> through /sys instead of "PNP0C0A".
> This was a bit mixed up. If, this should get implemented soon as the
> sysfs structure should not get altered that often...
> If wanted, I can send an on top patch later...

We've had this discussion before.

I'm inclined to keep the kernel as simple as possible, and let
some user-space thingie look up standard strings in some user-space table.
I don't want to get into the game of trying to make sysfs
so user-friendly, that a kernel re-build is necessary to add/translate
a new string...

that said, if we have to _invent_ a new string, it might as well
be something meaningful to a human.

> D) Why renaming struct acpi_device_id to struct __acpi_device_id
> 
> Not a problem, just for understanding why I have done this:
> struct acpi_device_id already exists in ACPICA. The module aliasing
> interface requires that struct "SUBSYSTEM"_device_id is used as name.
> This is not that obvious (include/linux/module.h):
> #define MODULE_GENERIC_TABLE(gtype,name) and
> #define MODULE_DEVICE_TABLE(type,name)

it is really irritating to end up with two nearly identical definitions.
it is also irritating to diverge from ACPICA for syntax reasons.

I don't really have a better idea -- though I'd be inclined to call it
acpica_device_id instead of __acpi_device_id if we're stuck with that route.

-Len

Re: [PATCH 0/3] ACPI autoloading

[Thomas Renninger]

On Tue, 2007-07-03 at 03:40 -0400, Len Brown wrote:
> > A) Cannot pass acpi_device_id through ops.add (probe equivalent func)
> > ---------------------------------------------------------------------
> > 
> > int acpi_match_device_ids(..)
> > should be:
> > const acpi_device_id *acpi_match_device_ids
> > and the matching device id should get passed to the acpi driver's .add
> > function.
> > This would ease up the code in the ACPI driver a lot (e.g. see button.c,
> > it is compared again which device (HID) has been found. Theoretically
> > the drivers also needs to check CIDs...).
> > 
> > The problem is that in scan.c ACPI driver is abstracted to use .match
> > and .probe "struct bus_type" functions and I have no idea how the
> > matching acpi_device_id should get passed or stored from
> > acpi_match_device_ids (.match) to ops.add (invoked in .probe).
> > Currently:
> >         kernel_ulong_t driver_data;
> > of acpi_device_id got added, but is not used at all atm.
> >
> > B) Thermal module always gets loaded
> > ------------------------------------
> > 
> > This is because _TZ_ (scope?!?) always gets added, but is declared as a
> > device or at least pops up as a thermal device.
> > This is in drivers/acpi/utilities/utglobal.c:
> > 
> > /*
> >  * Predefined ACPI Names (Built-in to the Interpreter)
> >  *
> >  * NOTES:
> >  * 1) _SB_ is defined to be a device to allow \_SB_._INI to be run
> >  *    during the initialization sequence.
> >  * 2) _TZ_ is defined to be a thermal zone in order to allow ASL code to
> >  *    perform a Notify() operation on it.
> >  */
> > 
> > Any idea how to get rid of that is very welcome.
> > Is this to be able to process buggy ASL code of this kind?:
> > "Notify ("_TZ_", 0x80)"
> > While _TZ_ is not a real device, but a scope operator and gets
> > statically declared as a device here to solve such things?
> 
> ThermalZone objects hang under _TZ, whether TZ it be global, or under _SB
> I would think that if there are no ThermalZone objects,
> then we'd not need to bind the thermal driver.
> 
> However, this would be a custom binding method (like acpi video.c)
> rather than a HID based one.
> 
> I'm really not sure about the _TZ_ syntax -- we'll have to ask Bob
> where that came from when he returns -- but I don't think it matters
> for the issue at hand.
Yeah. It's the same behavior as before then, as everybody would try to
load the thermal module on all i386/x86_64 machines anyway.
Would be great if Bob could give a short comment, maybe I start digging
a bit deeper if I find the time to find out why this is and how this can
get workarounded (in another way, already looks like a workaround).
> 
> > C) Renaming/Unifying of HID strings
> > -----------------------------------
> > 
> > This shouldn't be a problem. Consequence is that the new ACPI sysfs
> > structure will show other names, but as it got introduced recently this
> > shouldn't hurt anyone?
> 
> I agree that it is still new and that it is unlikely that a re-name
> now would be a significant compatibility issue.
> 
> > For better readability we could map all HIDs to a device name through a
> > HID <-> Device Name - enum and table. And then export "battery" again
> > through /sys instead of "PNP0C0A".
> > This was a bit mixed up. If, this should get implemented soon as the
> > sysfs structure should not get altered that often...
> > If wanted, I can send an on top patch later...
> 
> We've had this discussion before.
> 
> I'm inclined to keep the kernel as simple as possible, and let
> some user-space thingie look up standard strings in some user-space table.
> I don't want to get into the game of trying to make sysfs
> so user-friendly, that a kernel re-build is necessary to add/translate
> a new string...
> 
> that said, if we have to _invent_ a new string, it might as well
> be something meaningful to a human.
I don't know. Maybe Kay could give a comment whether it's useful/common
to have the PNPwxyz:00 path in /sys/../acpi/... to be evaluated to e.g.
a battery:00 or ac_adapter:00 path name.

IMO it is useful. Any user who knows about linux, but has no idea about
ACPI will find battery info by simply "find /sys |grep battery".
Disadvantage would be the size of the strings mapping to the PNP ids, I
think it's not that bad.
OTH you stick to the spec name of the device.
No idea what is better/nicer, but we should decide before 2.6.23...
For the latter we could add another documentation file,
acpi_hids_mapping.txt listing all supported Hids?

Userspace (HAL) would need to know about the ids then..., maybe these
guys have any preferences?
> 
> > D) Why renaming struct acpi_device_id to struct __acpi_device_id
> > 
> > Not a problem, just for understanding why I have done this:
> > struct acpi_device_id already exists in ACPICA. The module aliasing
> > interface requires that struct "SUBSYSTEM"_device_id is used as name.
> > This is not that obvious (include/linux/module.h):
> > #define MODULE_GENERIC_TABLE(gtype,name) and
> > #define MODULE_DEVICE_TABLE(type,name)
> 
> it is really irritating to end up with two nearly identical definitions.
> it is also irritating to diverge from ACPICA for syntax reasons.
> 
> I don't really have a better idea -- though I'd be inclined to call it
> acpica_device_id instead of __acpi_device_id if we're stuck with that route.

As this is ACPICA code, can you do this, you have to touch it anyway?
In general, I try(ied) to separate ACPICA code in a separate patch. Does
it make sense to directly patch against the ACPICA package (maybe not at
the moment, but if the next update comes out -> nothing urgent, but I
expect the current version has diverged a lot)?

Thanks,

   Thomas