On Thu, Jun 21, 2007 at 06:54:56AM +0200, Juan RP wrote:
> On Sat, 16 Jun 2007 03:47:43 +0200
> Juan RP <juan@xtrarom.org> wrote:
> 
> > On Thu, 14 Jun 2007 17:16:42 +0200
> > Juan RP <juan@xtrarom.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
> http://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