On Tue, Jul 17, 2007 at 07:29:56PM -0400, Jared D. McNeill wrote:

> First cut of my new device power management code is ready for review. 
> Feedback would be great.
> 
>   http://www.invisible.ca/~jmcneill/netbsd/jmcneill-devpm-22.patch

Thanks for working on this, it's something that we have been really missing
out on for a long time now.

A couple of nits:

1) +	callout_init(&dev->dv_dd.dd_idle, CALLOUT_MPSAFE);

From what I remember seeing when I looked, the callout doesn't do locking so
it's not MP safe. Until the soft interrupt changes from the vmlocking branch
go in it's actually quite difficult to do that since there is an ordering
constraint between the kernel_lock and all spinlocks in the kernel.

2) +MALLOC_DEFINE(M_DEVPM, "devpm", "device power management memory");

If you're not going to allocate or free memory from an interrupt handler,
then kmem_alloc/kmem_free are a better choice.

I have some more general comments, and I'm going to display my ignorance of
power management here.

In what order do devices get notified of events? Does the bus driver get
notified first or its children first? Naively "leaf drivers first, nexus
drivers after that" seems to be the right order.

What kind of overlap is there with the shutdown hooks?

I've given the whole hooks framework a bit of thought over time and I really
don't like it. From the perspective of providing a stable driver API and
making loadable drivers work well, it's crummy. It encodes information in
the driver that the driver shouldn't know about: calls to establish the
hooks. And, I think that any shutdown/power hook established outside a
device driver is evidence of a missing driver. :-)

What I was thinking of is to have the power management bits be something
that every device in the system has (so they are part of device_t), and have
an optional device entry point in cfattach_t that handles these kinds of
requests - some kind of basic message passing interface, so that we can
extend or version it easily enough later.

Where that may also help is when we clean up the locking protocol around
autoconf (e.g. preventing devices from appearing or disappearing when
something's talking with them).

What do you think?

Thanks,
Andrew