FYI: ENVSYS 2 ready

Hi,

I want to inform you that my branch is 95% finished, so
let's explain
what does it provide:

- The code has been proplib(3)ified.
- Detachable sensors now really work, when a device is
unregistered with
  sysmon_envsys_unregister(), it's removed from the linked
list and its
  dictionary is removed from the global dictionary.
- Each device uses an array of dictionaries, one per
sensor.
- envstat(8) rewritten from scratch with support to specify
your
  critical limits, descriptions, rfact in /etc/envsys.conf.
- /etc/rc.d/envsys. A script to start/stop monitoring
functions and/or
  changing other aspects of envstat(8) -m.
- Support to set critical max/min/capacity limits on
sensors, directly
  using the new API features or via envstat(8).
- Support to change sensor's description on the fly, via
envstat.
- Support to change rfact on the fly in voltage sensors (if
enabled on
  the driver).
- sysmon_power/powerd(8) has been extended to support the
new events
  used by envsys2. The following scripts are available:

/etc/powerd/scripts/sensor_battery (for battery sensors)
/etc/powerd/scripts/sensor_drive (for drive sensors,
mfi(4))
/etc/powerd/scripts/sensor_fan
/etc/powerd/scripts/sensor_power (Watts/Ampere)
/etc/powerd/scripts/sensor_resistency (Ohms)
/etc/powerd/scripts/sensor_temperature
/etc/powerd/scripts/sensor_voltage

- There's a linked list for the sysmon envsys events, when
the first
  one is added, a workqueue(9) is created and a callout(9)
runs every
  10 seconds looking if the condition is triggered and sends
a
  critical/warning event. If the state after coming from
  critical/warning is normal, a "normal" event is
sent to userland to
  notify powerd(8) that all it's ok again.
- acpibat(4) has been adapted to send critical/critunder and
warnunder
  events when battery capacity is critical, warncap or
lowcap
  respectively.
- acpitz(4) has been modified to send critical/critover
events, when
  the thermal zone is critical or hot.

All drivers were adapted to the new branch, and 60% of them
were tested by some developers/users (Manuel Bouyer, Nicolas
Joly,
Brett Lymn, Mihai Chelaru, Joerg Sonnenberger, etc).

Also I fixed the n^2 gtredata bug on many drivers, all this
stuff is
explained in the README file. I'm only missing to write the
documentation (sysmon_envsys(9) and sysmon_power(9)).

You can see all the code here: http://www.xtrarom.org/
repos/

I'm going to commit it ASAP, so if you are not happy with
something
in the code or in the API, explain what's wrong and why and
I'll answer
you.

Thanks for your cooperation.

-- 
Juan Romero Pardines	- The NetBSD Project
http://plog.xtrarom.org/
	- NetBSD/pkgsrc news in Spanish

On Thu, 14 Jun 2007 16:55:59 +0200
Juan RP <juanxtrarom.org> wrote:

> Hi,
> 
> I want to inform you that my branch is 95% finished, so
let's explain
> what does it provide:
> 
> - The code has been proplib(3)ified.
> - Detachable sensors now really work, when a device is
unregistered
> with sysmon_envsys_unregister(), it's removed from the
linked list
> and its dictionary is removed from the global
dictionary.
> - Each device uses an array of dictionaries, one per
sensor.
> - envstat(8) rewritten from scratch with support to
specify your
>   critical limits, descriptions, rfact in
/etc/envsys.conf.
> - /etc/rc.d/envsys. A script to start/stop monitoring
functions and/or
>   changing other aspects of envstat(8) -m.
> - Support to set critical max/min/capacity limits on
sensors, directly
>   using the new API features or via envstat(8).
> - Support to change sensor's description on the fly,
via envstat.
> - Support to change rfact on the fly in voltage sensors
(if enabled on
>   the driver).
> - sysmon_power/powerd(8) has been extended to support
the new events
>   used by envsys2. The following scripts are
available:
> 
> /etc/powerd/scripts/sensor_battery (for battery
sensors)
> /etc/powerd/scripts/sensor_drive (for drive sensors,
mfi(4))
> /etc/powerd/scripts/sensor_fan
> /etc/powerd/scripts/sensor_power (Watts/Ampere)
> /etc/powerd/scripts/sensor_resistency (Ohms)
> /etc/powerd/scripts/sensor_temperature
> /etc/powerd/scripts/sensor_voltage
> 
> - There's a linked list for the sysmon envsys events,
when the first
>   one is added, a workqueue(9) is created and a
callout(9) runs every
>   10 seconds looking if the condition is triggered and
sends a
>   critical/warning event. If the state after coming
from
>   critical/warning is normal, a "normal"
event is sent to userland to
>   notify powerd(8) that all it's ok again.
> - acpibat(4) has been adapted to send
critical/critunder and warnunder
>   events when battery capacity is critical, warncap or
lowcap
>   respectively.
> - acpitz(4) has been modified to send critical/critover
events, when
>   the thermal zone is critical or hot.
> 
> All drivers were adapted to the new branch, and 60% of
them
> were tested by some developers/users (Manuel Bouyer,
Nicolas Joly,
> Brett Lymn, Mihai Chelaru, Joerg Sonnenberger, etc).
> 
> Also I fixed the n^2 gtredata bug on many drivers, all
this stuff is
> explained in the README file. I'm only missing to write
the
> documentation (sysmon_envsys(9) and sysmon_power(9)).
> 
> You can see all the code here: http://www.xtrarom.org/
repos/

More things I wanted to say:

- Support for the old ENVSYS_GTREDATA and ENVSYS_GTREINFO
ioctls
  is implemented, so old envstat(8) and other apps using the
old
  framework work perfectly.
- If there are no items on the sysmon envsys events linked
list, the
  workqueue(9) is destroyed and the callout(9) stopped.
- Old powerd(8) won't work anymore because
POWER_EVENT_MSG_SIZE
  is now 128 bytes, where previously it used 32 bytes.
There's no easy
  way to support the old powerd.

-- 
Juan Romero Pardines	- The NetBSD Project
http://plog.xtrarom.org/
	- NetBSD/pkgsrc news in Spanish

On Thu, Jun 14, 2007 at 04:55:59PM +0200, Juan RP wrote:
> 
> /etc/powerd/scripts/sensor_resistency (Ohms)
> 

Just a niggly nit, that should be:

/etc/powerd/scripts/sensor_resistance (Ohms)

-- 
Brett Lymn

> You can see all the code here: http://www.xtrarom.org/
repos/

can you provide a diff?

YAMAMOTO Takashi

On Fri, 15 Jun 2007 11:28:19 +0930
Brett Lymn <blymnbaesystems.com.au> wrote:

> On Thu, Jun 14, 2007 at 04:55:59PM +0200, Juan RP
wrote:
> > 
> > /etc/powerd/scripts/sensor_resistency (Ohms)
> > 
> 
> Just a niggly nit, that should be:
> 
> /etc/powerd/scripts/sensor_resistance (Ohms)

Thanks, I'll fix.

-- 
Juan Romero Pardines	- The NetBSD Project
http://plog.xtrarom.org/
	- NetBSD/pkgsrc news in Spanish

On Fri, 15 Jun 2007 12:23:53 +0900 (JST)
yamtmwd.biglobe.ne.jp (YAMAMOTO Takashi) wrote:

> > You can see all the code here: http://www.xtrarom.org/
repos/
> 
> can you provide a diff?

http
://www.netbsd.org/~xtraeme/full_envsys2.diff

-- 
Juan Romero Pardines	- The NetBSD Project
http://plog.xtrarom.org	-
NetBSD/pkgsrc news in Spanish

On Thu, 14 Jun 2007 17:16:42 +0200
Juan RP <juanxtrarom.org> wrote:

> - Old powerd(8) won't work anymore because
POWER_EVENT_MSG_SIZE
>   is now 128 bytes, where previously it used 32 bytes.
There's no easy
>   way to support the old powerd.

This is not valid anymore, I just added support for a
"dictionary based
communication channel" between sysmon_power.c and
powerd(8).
Old powerd (8) works as before via COMPAT_40, but it's only
able to
handle pswitch events.

Just FYI, here's new powerd(8) in debug mode:

$ sudo powerd -d 
dispatch_dev_power: 1 events available
dispatch_dev_power: event type 0
<?xml version="1.0"
encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple Computer//DTD PLIST
1.0//EN"
"http://www.apple.com/DTDs/PropertyList-1.0.dtd">
 <plist version="1.0">
<dict>
        <key>driver-name</key>
        <string>acpi0</string>
        <key>power_type</key>
        <string>pswitch</string>
        <key>powerd-event-name</key>
        <string>pressed</string>
        <key>powerd-script-name</key>
        <string>power_button</string>
</dict>
</plist>
running script: /etc/powerd/scripts/power_button acpi0
pressed
dispatch_dev_power: 1 events available
dispatch_dev_power: event type 0
<?xml version="1.0"
encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple Computer//DTD PLIST
1.0//EN"
"http://www.apple.com/DTDs/PropertyList-1.0.dtd">
 <plist version="1.0">
<dict>
        <key>driver-name</key>
        <string>acpiacad0</string>
        <key>power_type</key>
        <string>pswitch</string>
        <key>powerd-event-name</key>
        <string>released</string>
        <key>powerd-script-name</key>
        <string>acadapter</string>
</dict>
</plist>
running script: /etc/powerd/scripts/acadapter acpiacad0
released

Have fun!

-- 
Juan Romero Pardines	- The NetBSD Project
http://plog.xtrarom.org/
	- NetBSD/pkgsrc news in Spanish

Hi Juan,

On Thu, Jun 14, 2007 at 04:55:59PM +0200, Juan RP wrote:
[...]
> You can see all the code here: http://www.xtrarom.org/
repos/
> 
> I'm going to commit it ASAP, so if you are not happy
with something
> in the code or in the API, explain what's wrong and why
and I'll answer
> you.

This smells very good.  I don't know how quickly you intend
to commit,
but I wanted to look at the code, and between work and my
stuff at home
claimed by a power surge, I might lack time.

Meanwhile, can you provide diffs for the files you modify? 
That would
definitely help review (and not just mine).

Good work and thanks,

-- 
Quentin Garnier - cubecubidou.net - cubeNetBSD.org
"You could have made it, spitting out benchmarks
Owe it to yourself not to fail"
Amplifico, Spitting Out Benchmarks, Hometakes Vol. 2,
2005.

On Sat, 16 Jun 2007 03:47:43 +0200
Juan RP <juanxtrarom.org> wrote:

> On Thu, 14 Jun 2007 17:16:42 +0200
> Juan RP <juanxtrarom.org> wrote:
> 
> > - Old powerd(8) won't work anymore because
POWER_EVENT_MSG_SIZE
> >   is now 128 bytes, where previously it used 32
bytes. There's no easy
> >   way to support the old powerd.
> 
> This is not valid anymore, I just added support for a
"dictionary based
> communication channel" between sysmon_power.c and
powerd(8).
> Old powerd (8) works as before via COMPAT_40, but it's
only able to
> handle pswitch events.

Please review the latest patches, if there are no objections
or useful
comments, I'll commit it in 10 days (31st June).

The patches are separated in three components for easier
reading:

http:
//www.netbsd.org/~xtraeme/envsys2_api.diff
h
ttp://www.netbsd.org/~xtraeme/envsys2_drivers.diff

http://www.netbsd.org/~xtraeme/envsys2_userland.diff

You can see the manpages here:

http://www.xtra
rom.org/~juan/envsys2/

-- 
Juan Romero Pardines	- The NetBSD Project
http://plog.xtrarom.org	-
NetBSD/pkgsrc news in Spanish

On Thu, 21 Jun 2007 06:54:56 +0200
Juan RP <juanxtrarom.org> wrote:

> Please review the latest patches, if there are no
objections or useful
> comments, I'll commit it in 10 days (31st June).

There's is not 31st June, so make it 1st July.

> The patches are separated in three components for
easier reading:
> 
> http:
//www.netbsd.org/~xtraeme/envsys2_api.diff
> h
ttp://www.netbsd.org/~xtraeme/envsys2_drivers.diff
> 
http://www.netbsd.org/~xtraeme/envsys2_userland.diff
> 
> You can see the manpages here:
> 
> http://www.xtra
rom.org/~juan/envsys2/

-- 
Juan Romero Pardines	- The NetBSD Project
http://plog.xtrarom.org	-
NetBSD/pkgsrc news in Spanish

I don't like struct tables are put in headers.

Masao

On Thu, 21 Jun 2007 16:02:44 +0900
"Masao Uebayashi" <uebayasigmail.com> wrote:

> I don't like struct tables are put in headers.

What do you suggest then? I need those available for at
least three
components: sysmon_envsys.c, sysmon_envsys_events.c and
envstat.c.

-- 
Juan Romero Pardines	- The NetBSD Project
http://plog.xtrarom.org	-
NetBSD/pkgsrc news in Spanish

On Thu, Jun 21, 2007 at 09:08:19 +0200, Juan RP wrote:

> On Thu, 21 Jun 2007 16:02:44 +0900
> "Masao Uebayashi" <uebayasigmail.com> wrote:
> 
> > I don't like struct tables are put in headers.
> 
> What do you suggest then? I need those available for at
least three
> components: sysmon_envsys.c, sysmon_envsys_events.c and
envstat.c.

Yes, tables in headers is totally wrong.  Also, think about
l10n on
messages for envstat(8), for which such static tables are
not enough.

SY, Uwe
-- 
uwestderr.spb.ru                       |       Zu Grunde
kommen
http://snark.ptc.spbu.
ru/~uwe/          |       Ist zu Grunde gehen

On Thu, 21 Jun 2007 13:55:54 +0400
"Valeriy E. Ushakov" <uwestderr.spb.ru> wrote:

> On Thu, Jun 21, 2007 at 09:08:19 +0200, Juan RP wrote:
> 
> > On Thu, 21 Jun 2007 16:02:44 +0900
> > "Masao Uebayashi" <uebayasigmail.com> wrote:
> > 
> > > I don't like struct tables are put in
headers.
> > 
> > What do you suggest then? I need those available
for at least three
> > components: sysmon_envsys.c,
sysmon_envsys_events.c and envstat.c.
> 
> Yes, tables in headers is totally wrong.  Also, think
about l10n on
> messages for envstat(8), for which such static tables
are not enough.

I'm fixing it... I'm just testing the changes now.

-- 
Juan Romero Pardines	- The NetBSD Project
http://plog.xtrarom.org	-
NetBSD/pkgsrc news in Spanish

On Thu, 21 Jun 2007 16:02:44 +0900
"Masao Uebayashi" <uebayasigmail.com> wrote:

> I don't like struct tables are put in headers.

Ok, I removed the static tables from the headers... they are
now only
used on sysmon_envsys.c and sysmon_power.c. envstat(8)
doesn't need to
use them anymore.

The patches are:

http
://www.netbsd.org/~xtraeme/envsys2_api2.diff
h
ttp://www.netbsd.org/~xtraeme/envsys2_drivers.diff
http://www.netbsd.org/~xtraeme/envsys2_userland2.diff

-- 
Juan Romero Pardines	- The NetBSD Project
http://plog.xtrarom.org	-
NetBSD/pkgsrc news in Spanish

On Thu, Jun 21, 2007 at 06:54:56AM +0200, Juan RP wrote:
> On Sat, 16 Jun 2007 03:47:43 +0200
> Juan RP <juanxtrarom.org> wrote:
> 
> > On Thu, 14 Jun 2007 17:16:42 +0200
> > Juan RP <juanxtrarom.org> wrote:
> > 
> > > - Old powerd(8) won't work anymore because
POWER_EVENT_MSG_SIZE
> > >   is now 128 bytes, where previously it used
32 bytes. There's no easy
> > >   way to support the old powerd.
> > 
> > This is not valid anymore, I just added support
for a "dictionary based
> > communication channel" between sysmon_power.c
and powerd(8).
> > Old powerd (8) works as before via COMPAT_40, but
it's only able to
> > handle pswitch events.
> 
> Please review the latest patches, if there are no
objections or useful
> comments, I'll commit it in 10 days (31st June).
> 
> The patches are separated in three components for
easier reading:
> 
> http:
//www.netbsd.org/~xtraeme/envsys2_api.diff
> h
ttp://www.netbsd.org/~xtraeme/envsys2_drivers.diff
> 
http://www.netbsd.org/~xtraeme/envsys2_userland.diff

1. sysmon_envsys_find()

	mutex_enter(&sme_mtx);
	LIST_FOREACH(sme, &sysmon_envsys_list, sme_list) {
		if (strcmp(sme->sme_name, name) == 0)
			break;
	}
	mutex_exit(&sme_mtx);
	return sme;

How do you know the item is still there after you drop
sme_mtx? Can it be
taken off the list as soon as you unlock?

2. In sme_make_dictionary(), why acquire and release the
lock so many times?
Wouldn't it be clearer to acquire it fewer times?

3. sme_event_unregister():

	if (LIST_EMPTY(&sme_events_list)) {
		mutex_exit(&sme_event_mtx);
		callout_stop(&seeco);
		workqueue_destroy(seewq);

		mutex_enter(&sme_event_mtx);
		sme_events_initialized = false;

When you unlock to destroy the work queue / callout, it's
possible for
another thread to do sme_events_init() at the same time,
right?

The problem with holding a lock around those is that later
on when interrupt
handlers have thread context, you may want to acquire the
lock from an
interrupt handler or callout. Those cannot be delayed for
long. Functions
like workqueue_create() may want to sleep long term for
memory. One solution
may be to have two locks. One short term lock for data
(sme_event_mtx) and
one longer term lock for sme_events_initialized
(sme_event_init_mtx) that
you hold while setting up or destroying the resources.

4. sme_events_check()

Since it's a callout you can't take sme_event_mtx there.. It
might be useful
to add some commented out code to that effect, as later it
will be possible
to take the lock, eg:

sme_events_check(void *arg)
{
	sme_event_t *see;

	/* mutex_enter(&sme_event_mtx); XXX notyet */
	LIST_FOREACH(see, &sme_events_list, see_list) {

Andrew

On Thu, 21 Jun 2007 14:53:20 +0100
Andrew Doran <adnetbsd.org> wrote:

> 1. sysmon_envsys_find()
> 
> 	mutex_enter(&sme_mtx);
> 	LIST_FOREACH(sme, &sysmon_envsys_list, sme_list)
{
> 		if (strcmp(sme->sme_name, name) == 0)
> 			break;
> 	}
> 	mutex_exit(&sme_mtx);
> 	return sme;
> 
> How do you know the item is still there after you drop
sme_mtx? Can it be
> taken off the list as soon as you unlock?

I check in the parts that use sysmon_envsys_find() if
returned is NULL,
isn't it enough? or are you talking about something else?

> 2. In sme_make_dictionary(), why acquire and release
the lock so many
> times? Wouldn't it be clearer to acquire it fewer
times?

I was using the approach that you said, but rmind
suggested to avoid
doing large operations with a mutex held...

> 3. sme_event_unregister():
> 
> 	if (LIST_EMPTY(&sme_events_list)) {
> 		mutex_exit(&sme_event_mtx);
> 		callout_stop(&seeco);
> 		workqueue_destroy(seewq);
> 
> 		mutex_enter(&sme_event_mtx);
> 		sme_events_initialized = false;
> 
> When you unlock to destroy the work queue / callout,
it's possible for
> another thread to do sme_events_init() at the same
time, right?

Hmm why? sme_events_init() is protected by the mutex
sme_events_init.

> The problem with holding a lock around those is that
later on when
> interrupt handlers have thread context, you may want to
acquire the lock
> from an interrupt handler or callout. Those cannot be
delayed for long.
> Functions like workqueue_create() may want to sleep
long term for memory.
> One solution may be to have two locks. One short term
lock for data
> (sme_event_mtx) and one longer term lock for
sme_events_initialized
> (sme_event_init_mtx) that you hold while setting up or
destroying the
> resources.

Also, rmind said that I must not allocate/deallocate resources
with
an adaptive mutex, is it true? or are you suggesting to
protect these
operations with another mutex?

> 4. sme_events_check()
> 
> Since it's a callout you can't take sme_event_mtx
there.. It might be
> useful to add some commented out code to that effect,
as later it will be
> possible to take the lock, eg:
> 
> sme_events_check(void *arg)
> {
> 	sme_event_t *see;
> 
> 	/* mutex_enter(&sme_event_mtx); XXX notyet */
> 	LIST_FOREACH(see, &sme_events_list, see_list) {

Hmm, ok.

-- 
Juan Romero Pardines	- The NetBSD Project
http://plog.xtrarom.org	-
NetBSD/pkgsrc news in Spanish

On Thu, 21 Jun 2007 14:53:20 +0100
Andrew Doran <adnetbsd.org> wrote:

> 2. In sme_make_dictionary(), why acquire and release
the lock so many
> times? Wouldn't it be clearer to acquire it fewer
times?
> 
> 3. sme_event_unregister():
> 
> 	if (LIST_EMPTY(&sme_events_list)) {
> 		mutex_exit(&sme_event_mtx);
> 		callout_stop(&seeco);
> 		workqueue_destroy(seewq);
> 
> 		mutex_enter(&sme_event_mtx);
> 		sme_events_initialized = false;
> 
> When you unlock to destroy the work queue / callout,
it's possible for
> another thread to do sme_events_init() at the same
time, right?
> 
> The problem with holding a lock around those is that
later on when
> interrupt handlers have thread context, you may want to
acquire the lock
> from an interrupt handler or callout. Those cannot be
delayed for long.
> Functions like workqueue_create() may want to sleep
long term for memory.
> One solution may be to have two locks. One short term
lock for data
> (sme_event_mtx) and one longer term lock for
sme_events_initialized
> (sme_event_init_mtx) that you hold while setting up or
destroying the
> resources.

Can you please check: http
://www.netbsd.org/~xtraeme/envsys2_api3.diff

I added another mutex to protect the operations with
callout/workqueue
as you suggested and I removed many mutex_enter/mutex_exit
with a few
ones in sme_make_dictionary().

Thanks!
-- 
Juan Romero Pardines	- The NetBSD Project
http://plog.xtrarom.org	-
NetBSD/pkgsrc news in Spanish

On Thu, Jun 21, 2007 at 04:02:41PM +0200, Juan RP wrote:
> On Thu, 21 Jun 2007 14:53:20 +0100
> Andrew Doran <adnetbsd.org> wrote:
> 
> > 1. sysmon_envsys_find()
> > 
> > 	mutex_enter(&sme_mtx);
> > 	LIST_FOREACH(sme, &sysmon_envsys_list,
sme_list) {
> > 		if (strcmp(sme->sme_name, name) == 0)
> > 			break;
> > 	}
> > 	mutex_exit(&sme_mtx);
> > 	return sme;
> > 
> > How do you know the item is still there after you
drop sme_mtx? Can it be
> > taken off the list as soon as you unlock?
> 
> I check in the parts that use sysmon_envsys_find() if
returned is NULL,
> isn't it enough? or are you talking about something
else?

I think the concern is that sysmon_envsys_find can find
something, but 
since you don't do reference counting, as soon as you return
it as found, 
another thread can remove it from the list and presumably
destroy it.

The mutex does a great job of protecting the list, but
nothing protects 
the entries after you take a reference.

Take care,

Bill

On Thu, 21 Jun 2007 11:57:39 -0700
Bill Stouder-Studenmund <wrstudennetbsd.org> wrote:

> I think the concern is that sysmon_envsys_find can find
something, but 
> since you don't do reference counting, as soon as you
return it as found, 
> another thread can remove it from the list and
presumably destroy it.
> 
> The mutex does a great job of protecting the list, but
nothing protects 
> the entries after you take a reference.

Thanks for the great information. It's fixed now...
sysmon_envsys_release()
was there for a reason 

I added another mutex and a condition variable to protect
those operations
(sysmon_envsys_find() and sysmon_envsys_unregister()).

Latest patches at:

http
://www.netbsd.org/~xtraeme/envsys2_api4.diff
h
ttp://www.netbsd.org/~xtraeme/envsys2_drivers.diff
http://www.netbsd.org/~xtraeme/envsys2_userland2.diff

-- 
Juan Romero Pardines	- The NetBSD Project
http://plog.xtrarom.org	-
NetBSD/pkgsrc news in Spanish