RE: [PATCH 1/4] properly create kobjects in acpi/scan.c

RE: [PATCH 1/4] properly create kobjects in acpi/scan.c

[Keshavamurthy, Anil S]

 
>> Btw, why don't we just convert ACPI to be standard driver core bus 
>> with standard drivers and devices? I could cook up something... The 
>> /sys/firmware/acpi hierarchy is not populated with any 
>useful data yet 
>> so there should be no concern of breakign userspace interface (I am 
>> mot talking about removing /proc/acpi part at the moment).
>
>Funny you should mention it.
>
>My current opinion is that /sys/firmware/acpi should probably 
>not exist at all, and that there should be a single 
>/sys/devices including both ACPI-enumerated and pci-enumerated 
>devices.  Adam and I have been ganging up on Patrick on this, 
>but we don't have him convinced yet...
>
For my hotplug work I am currently banking on
/sys/firmware/acpi/namespace/ACPI
 to find the all the children devices and from there to find the
corresponding logical
 devices. Once I find the all child devices then the scripts would turn
online or offline of all the child devices.

If we are proposing to eliminate /sys/firmware/acpi can anyone pass a
pointer to the above discussion.

Thanks,
Anil Keshavamurthy
	


-------------------------------------------------------
SF.Net email is sponsored by Shop4tech.com-Lowest price on Blank Media
100pk Sonic DVD-R 4x for only $29 -100pk Sonic DVD+R for only $33
Save 50% off Retail on Ink & Toner - Free Shipping and Free Gift.
http://www.shop4tech.com/z/Inkjet_Cartridges/9_108_r285

Re: [PATCH 1/4] properly create kobjects in acpi/scan.c

[Dmitry Torokhov]

On Tuesday 24 August 2004 04:45 pm, Keshavamurthy, Anil S wrote:
> 
> >> Btw, why don't we just convert ACPI to be standard driver core bus 
> >> with standard drivers and devices? I could cook up something... The 
> >> /sys/firmware/acpi hierarchy is not populated with any 
> >useful data yet 
> >> so there should be no concern of breakign userspace interface (I am 
> >> mot talking about removing /proc/acpi part at the moment).
> >
> >Funny you should mention it.
> >
> >My current opinion is that /sys/firmware/acpi should probably 
> >not exist at all, and that there should be a single 
> >/sys/devices including both ACPI-enumerated and pci-enumerated 
> >devices.  Adam and I have been ganging up on Patrick on this, 
> >but we don't have him convinced yet...
> >
> For my hotplug work I am currently banking on
> /sys/firmware/acpi/namespace/ACPI
>  to find the all the children devices and from there to find the
> corresponding logical
>  devices. Once I find the all child devices then the scripts would turn
> online or offline of all the child devices.
> 
> If we are proposing to eliminate /sys/firmware/acpi can anyone pass a
> pointer to the above discussion.
>

We would not eliminate it, it would naturally move to something like

/sys/bus/acpi/devices/acpi_root/... and  /sys/devices/acpi_root/...

Or something along these lines.

-- 
Dmitry


-------------------------------------------------------
SF.Net email is sponsored by Shop4tech.com-Lowest price on Blank Media
100pk Sonic DVD-R 4x for only $29 -100pk Sonic DVD+R for only $33
Save 50% off Retail on Ink & Toner - Free Shipping and Free Gift.
http://www.shop4tech.com/z/Inkjet_Cartridges/9_108_r285

RE: [PATCH 1/4] properly create kobjects in acpi/scan.c

[Keshavamurthy, Anil S]

 
>> If we are proposing to eliminate /sys/firmware/acpi can 
>anyone pass a 
>> pointer to the above discussion.
>>
>
>We would not eliminate it, it would naturally move to something like
>
>/sys/bus/acpi/devices/acpi_root/... and  /sys/devices/acpi_root/...
>
>Or something along these lines.
>
Thanks so much.

-Anil Keshavamurthy


-------------------------------------------------------
SF.Net email is sponsored by Shop4tech.com-Lowest price on Blank Media
100pk Sonic DVD-R 4x for only $29 -100pk Sonic DVD+R for only $33
Save 50% off Retail on Ink & Toner - Free Shipping and Free Gift.
http://www.shop4tech.com/z/Inkjet_Cartridges/9_108_r285

[PATCH 1/4] properly create kobjects in acpi/scan.c

[Dmitry Torokhov]

Hi,

I have a bunch of patches that I would like you to take a look at:

01-acpi-scan-kobject.patch
	- properly create/register kobjects - take reference to the parent
	  since kobject_unregister will try dropping it potentially
	  (if ACPI was ever to release created devices) causing parent be
	  destroyed early) and use kobject_set_name to properly set name.

02-acpi-event-code.patch
	- move event (userspace signalling) code from acpi/bus.c to
          acpi/event.c as too many implementation details were exported,
	  plus some additional fixes to the event handling code.

03-acpi-use-rwsem.patch
	- kill brandead usage of acpi_device_lock & list_for_eacj_safe.
	  list_for_each_safe is only safe in the sense that current
	  element can be safely removed by the same thread that is using
	  list_for_each_safe. List still has to be protected from other
	  threads for entire duration of list traversal.
	  Use acpi_subsys.rwsem instead of acpi_device_lock to protect
	  lists.

Btw, why don't we just convert ACPI to be standard driver core bus with
standard drivers and devices? I could cook up something... The
/sys/firmware/acpi hierarchy is not populated with any useful data yet
so there should be no concern of breakign userspace interface (I am mot
talking about removing /proc/acpi part at the moment).

04-acpi-multiple-readers.patch
	- allow multiple readrs access /proc/acpi/event, every event is
	  delivered to all readers. Also limit number of pending events
	  to 64 (per reader) so if reader is stuck ACPI does not consume
	  all memory.

	  Can be user by various daemons, I could see cpufreqd listening
	  for battery insertion/removal events and debugging is much
	  easier too.

The patches are against last night pull from Linus repo.
	
-- 
Dmitry


===================================================================


ChangeSet@1.1842, 2004-08-24 01:09:40-05:00, dtor_core-yWtbtysYrB+LZ21kGMrzwg@public.gmane.org
  ACPI: Properly create kobj when registering acpi_device:
        - use kobject_set_name to initialize name;
        - take teference to parent's kobj as kobject_unregister
          will drop it
        - do not call kobject_init and kobject_add, just use
          kobject_register as everyone else does.
  
  Signed-off-by: Dmitry Torokhov <dtor-JGs/UdohzUI@public.gmane.org>


 scan.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)


===================================================================



diff -Nru a/drivers/acpi/scan.c b/drivers/acpi/scan.c
--- a/drivers/acpi/scan.c	2004-08-24 01:16:25 -05:00
+++ b/drivers/acpi/scan.c	2004-08-24 01:16:25 -05:00
@@ -66,13 +66,11 @@
 		list_add_tail(&device->g_list,&acpi_device_list);
 	spin_unlock(&acpi_device_lock);
 
-	kobject_init(&device->kobj);
-	strlcpy(device->kobj.name,device->pnp.bus_id,KOBJ_NAME_LEN);
+	kobject_set_name(&device->kobj, device->pnp.bus_id);
 	if (parent)
-		device->kobj.parent = &parent->kobj;
-	device->kobj.ktype = &ktype_acpi_ns;
+		device->kobj.parent = kobject_get(&parent->kobj);
 	device->kobj.kset = &acpi_namespace_kset;
-	kobject_add(&device->kobj);
+	kobject_register(&device->kobj);
 }
 
 static int


-------------------------------------------------------
SF.Net email is sponsored by Shop4tech.com-Lowest price on Blank Media
100pk Sonic DVD-R 4x for only $29 -100pk Sonic DVD+R for only $33
Save 50% off Retail on Ink & Toner - Free Shipping and Free Gift.
http://www.shop4tech.com/z/Inkjet_Cartridges/9_108_r285

Re: [PATCH 1/4] properly create kobjects in acpi/scan.c

[Hiroshi Miura]

Hi,

At Tue, 24 Aug 2004 01:34:15 -0500,
Dmitry Torokhov wrote:
> 04-acpi-multiple-readers.patch
> 	- allow multiple readrs access /proc/acpi/event, every event is
> 	  delivered to all readers. Also limit number of pending events
> 	  to 64 (per reader) so if reader is stuck ACPI does not consume
> 	  all memory.
> 
> 	  Can be user by various daemons, I could see cpufreqd listening
> 	  for battery insertion/removal events and debugging is much
> 	  easier too.

'acpid' has soket interface for other daemon/utillities.

from man acpid
       In  addition  to  rule  files, acpid also accepts connections on a UNIX
       domain socket (/var/run/acpid.socket by default).  Any application  may
       connect  to  this  socket.  Once connected, acpid will send the text of
       all ACPI events to the client.  The client has  the  responsibility  of
       filtering  for messages about which it cares.  acpid will not close the
       client socket except in the case of a SIGHUP or acpid exiting.

I think it is enough.

If allow multiple readers, acpid and other tool need some lock facility to avoid 
unwanted multiple event handling.

-- 
Hiroshi Miura  --- http://www.da-cha.org/  --- miura-yiisDzvROlQdnm+yROfE0A@public.gmane.org
NTTDATA Corp. OpenSource Software Center. --- miurahr-3MafRgGXt7BL9jVzuh4AOg@public.gmane.org 
NTTDATA Intellilink Corp. OpenSource Engineering Dev. -- miurahr-w0OK63jvRlAuJ+9fw/WgBHgSJqDPrsil@public.gmane.org
Key fingerprint = 9117 9407 5684 FBF1 4063  15B4 401D D077 04AB 8617


-------------------------------------------------------
SF.Net email is sponsored by Shop4tech.com-Lowest price on Blank Media
100pk Sonic DVD-R 4x for only $29 -100pk Sonic DVD+R for only $33
Save 50% off Retail on Ink & Toner - Free Shipping and Free Gift.
http://www.shop4tech.com/z/Inkjet_Cartridges/9_108_r285

Re: [PATCH 1/4] properly create kobjects in acpi/scan.c

[Mattia Dongili]

On Wed, Aug 25, 2004 at 12:23:04AM +0900, Hiroshi Miura wrote:
> Hi,
> 
> At Tue, 24 Aug 2004 01:34:15 -0500,
> Dmitry Torokhov wrote:
> > 04-acpi-multiple-readers.patch
> > 	- allow multiple readrs access /proc/acpi/event, every event is
> > 	  delivered to all readers. Also limit number of pending events
> > 	  to 64 (per reader) so if reader is stuck ACPI does not consume
> > 	  all memory.
> > 
> > 	  Can be user by various daemons, I could see cpufreqd listening
> > 	  for battery insertion/removal events and debugging is much
> > 	  easier too.
> 
> 'acpid' has soket interface for other daemon/utillities.
> 
> from man acpid
>        In  addition  to  rule  files, acpid also accepts connections on a UNIX
>        domain socket (/var/run/acpid.socket by default).  Any application  may
[...]
> I think it is enough.

but relying on the availability of acpid is not a good approach. And why
can acpid hijack /proc/acpi/event?
Allowing multiple readers is much better for every daemon that needs to
get acpi events.

just my 0.2 :)
-- 
mattia
:wq!


-------------------------------------------------------
SF.Net email is sponsored by Shop4tech.com-Lowest price on Blank Media
100pk Sonic DVD-R 4x for only $29 -100pk Sonic DVD+R for only $33
Save 50% off Retail on Ink & Toner - Free Shipping and Free Gift.
http://www.shop4tech.com/z/Inkjet_Cartridges/9_108_r285

Re: [PATCH 1/4] properly create kobjects in acpi/scan.c

[Dmitry Torokhov]

On Tuesday 24 August 2004 10:23 am, Hiroshi Miura wrote:
> Hi,
> 
> At Tue, 24 Aug 2004 01:34:15 -0500,
> Dmitry Torokhov wrote:
> > 04-acpi-multiple-readers.patch
> > 	- allow multiple readrs access /proc/acpi/event, every event is
> > 	  delivered to all readers. Also limit number of pending events
> > 	  to 64 (per reader) so if reader is stuck ACPI does not consume
> > 	  all memory.
> > 
> > 	  Can be user by various daemons, I could see cpufreqd listening
> > 	  for battery insertion/removal events and debugging is much
> > 	  easier too.
> 
> 'acpid' has soket interface for other daemon/utillities.
> 
> from man acpid
>        In  addition  to  rule  files, acpid also accepts connections on a UNIX
>        domain socket (/var/run/acpid.socket by default).  Any application  may
>        connect  to  this  socket.  Once connected, acpid will send the text of
>        all ACPI events to the client.  The client has  the  responsibility  of
>        filtering  for messages about which it cares.  acpid will not close the
>        client socket except in the case of a SIGHUP or acpid exiting.
> 
> I think it is enough.

I do not think that present implementation of acpid has to be a prerequisite
for all setups. Plus, tunneling events through acpid introduces unwanted (IMHO)
dependencies: acpid has to be started first, restarting it can mess up other
processes reading from it's socket, etc, etc...

Given how cheap multiple readers implementation is and the fact that exclusive
only access only causes troubles (think psaux in older kernels) I think it
should go in.
 
> If allow multiple readers, acpid and other tool need some lock facility to avoid 
> unwanted multiple event handling.
> 

Well, since acpid manifests that it will pass _all_ events to its readers the
same issue exists right now.

-- 
Dmitry


-------------------------------------------------------
SF.Net email is sponsored by Shop4tech.com-Lowest price on Blank Media
100pk Sonic DVD-R 4x for only $29 -100pk Sonic DVD+R for only $33
Save 50% off Retail on Ink & Toner - Free Shipping and Free Gift.
http://www.shop4tech.com/z/Inkjet_Cartridges/9_108_r285

Re: [PATCH 1/4] properly create kobjects in acpi/scan.c

[Len Brown]

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

On Tue, 2004-08-24 at 02:34, Dmitry Torokhov wrote:
> Hi,
> 
> I have a bunch of patches that I would like you to take a look at:
> 
> 01-acpi-scan-kobject.patch
> 	- properly create/register kobjects - take reference to the parent
> 	  since kobject_unregister will try dropping it potentially
> 	  (if ACPI was ever to release created devices) causing parent be
> 	  destroyed early) and use kobject_set_name to properly set name.

I'll take a look.

> 02-acpi-event-code.patch
> 	- move event (userspace signalling) code from acpi/bus.c to
>           acpi/event.c as too many implementation details were exported,
> 	  plus some additional fixes to the event handling code.

I'll take a look.

> 03-acpi-use-rwsem.patch
> 	- kill brandead usage of acpi_device_lock & list_for_eacj_safe.
> 	  list_for_each_safe is only safe in the sense that current
> 	  element can be safely removed by the same thread that is using
> 	  list_for_each_safe. List still has to be protected from other
> 	  threads for entire duration of list traversal.
> 	  Use acpi_subsys.rwsem instead of acpi_device_lock to protect
> 	  lists.

sounds like a bug.

> Btw, why don't we just convert ACPI to be standard driver core bus with
> standard drivers and devices? I could cook up something... The
> /sys/firmware/acpi hierarchy is not populated with any useful data yet
> so there should be no concern of breakign userspace interface (I am mot
> talking about removing /proc/acpi part at the moment).

Funny you should mention it.

My current opinion is that /sys/firmware/acpi should probably
not exist at all, and that there should be a single
/sys/devices including both ACPI-enumerated and pci-enumerated
devices.  Adam and I have been ganging up on Patrick on this,
but we don't have him convinced yet...

> 04-acpi-multiple-readers.patch
> 	- allow multiple readrs access /proc/acpi/event, every event is
> 	  delivered to all readers. Also limit number of pending events
> 	  to 64 (per reader) so if reader is stuck ACPI does not consume
> 	  all memory.
> 
> 	  Can be user by various daemons, I could see cpufreqd listening
> 	  for battery insertion/removal events and debugging is much
> 	  easier too.

others have been frustrated by this also, so I tend to agree that
we should probably address it.  Somebody can still contact acpid if they
want to...  At some point, of course, we're going to have to move this
out of /proc...

> The patches are against last night pull from Linus repo.

Note that if you did individual file comments, then I can preserve your
comments if you export with bjorn's bkexport script (attached).

thanks,
-Len




[-- Attachment #2: bkimport --]
[-- Type: text/plain, Size: 608 bytes --]

#!/bin/sh

# Import a bitkeeper patch, preserving author and per-file comments

_import() {
    PATCH=$1

    FROM=`grep '^#### AUTHOR' $PATCH`
    export BK_USER=`echo $FROM | sed -e 's/^#### AUTHOR //' -e 's/@.*//'`
    export BK_HOST=`echo $FROM | sed -e 's/.*@//'`

    echo Importing $PATCH from $BK_USER@$BK_HOST

    #bk import -tpatch -qR $PATCH .
    bk import -tpatch -vR $PATCH .
    if [ $? -ne 0 ]; then
	echo $PATCH: import failed
	exit 0
    fi
    cat $PATCH |\
	awk "/#### COMMENT START/ {p=1; next}; /#### COMMENT END/ {exit}; p==1" |\
	bk comments -
}

for P in $@; do
    _import $P
done

[-- Attachment #3: bkexport --]
[-- Type: text/plain, Size: 524 bytes --]

#!/bin/sh

# Export a bitkeeper patch, adding author and per-file comments

if [ $# -gt 1 ]; then
	CSET=`bk r2c $*`
	CSET="-r$CSET"
else
	CSET=$1
fi

echo -n "#### AUTHOR "
bk cset $CSET |\
    awk -F\| "{printf \"bk prs -r%s -h -d '\$if(:DPN:=ChangeSet){:USER:@:HOST:\\n}' %s\n\", \$2, \$1}" |\
    sh
echo "#### COMMENT START"
bk cset $CSET |\
    awk -F\| "{printf \"bk prs -r%s -h -d '### Comments for :DPN:\\n\$each(:C:){(:C:)\\n}' %s\n\", \$2, \$1}" |\
    sh
echo "#### COMMENT END"
echo

bk export -tpatch -du $CSET