On Sun, Jul 18, 2010 at 12:35:31PM -0700, Paul Goyette wrote: > (Resent, this time with attachment!) > > On Sun, 18 Jul 2010, Quentin Garnier wrote: > > >On Sun, Jul 18, 2010 at 07:14:57AM -0700, Paul Goyette wrote: > >>Currently, pseudo-devices are silently attached to the device tree > >>without giving the driver any access to the associated device_t > >>structure. As a result, the pseudo-device driver is unable to > >>access much of the pmf framework. In particular, the pseudo-device > >>cannot register a suspend/resume/shutdown handler, nor can it > >>register to receive pmf event notifications. > >> > >>It seems to me that it would be reasonably useful for pseudo-devices > >>to be capabile of participating in pmf. One particular example I've > >>run into recently is the swwdog pseudo-device. Other watchdog > >>drivers can prevent a suspend if their timer is armed, but since > >>swwdog is only a pseudo-device it cannot register a pmf handler. > > > >Convert it to a defpseudodev then. > > I've done this, and it works! Diffs attached. Any objections to > committing them?
Not really objections, just a few comments. > +struct swwdog_softc *sc_swwdog = NULL; That can't possibly be needed (and, if it was, you'd have to mutexify it wouldn't you?). > + > +extern struct cfdriver swwdog_cd; Include "ioconf.h" I think. > +swwdogattach(int n) > { > - int i; > + int err; > + cfdata_t cf; > + > + if (n > 1) > + aprint_verbose("%s: ignoring count of %d\n", > + swwdog_cd.cd_name, n); > + > + err = config_cfattach_attach(swwdog_cd.cd_name, &swwdog_ca); > + if (err) { > + aprint_error("%s: couldn't register cfattach: %d\n", > + swwdog_cd.cd_name, err); > + config_cfdriver_detach(&swwdog_cd); > + return; > + } > + > + cf = kmem_alloc(sizeof(struct cfdata), KM_NOSLEEP); > + if (cf == NULL) > + aprint_error("%s: couldn't allocate cfdata\n", > + swwdog_cd.cd_name); > + else { > + cf->cf_name = swwdog_cd.cd_name; > + cf->cf_atname = swwdog_cd.cd_name; > + cf->cf_unit = 0; > + cf->cf_fstate = FSTATE_STAR; > > - for (i = 0; i < NSWWDOG; i++) { > - struct swwdog_softc *sc = &sc_wdog[i]; > + (void)config_attach_pseudo(cf); Ok, so you should be aware here that you're changing the logic of that function, and you're only allowing for the creation of one swwdog(4). That feature of the old code--implemented with needs-count no less, brrr--is highly debatable, and the manual page actually says it's pointless. So yeah, go ahead and kill it (you can keep the __unused on int count by the way), but change the manual page. Also, you don't have to allocate a struct cfdata, you can use a static one (indepedently of the fact that you only use it once, by the way, all the other pseudo-devices could be changed in that respect). > +static int > +swwdog_match(device_t parent, cfdata_t data, void *aux) > +{ > + /* Match unless we already have a swwdog */ > + if (sc_swwdog == NULL) > + return 1; > + > + return 0; > +} What could possibly trigger another config_attach_pseudo? I know that config(9) is special, but not magic :-) > + if (!pmf_device_register(self, NULL, NULL)) > + aprint_error_dev(self, "couldn't establish power " > + "handler\n"); So what was your initial point about swwdog(4) and pmf(9), anyway? While you're there, I guess you could make swwdog_reboot a sysctl node. -- Quentin Garnier - c...@cubidou.net - c...@netbsd.org "See the look on my face from staying too long in one place [...] every time the morning breaks I know I'm closer to falling" KT Tunstall, Saving My Face, Drastic Fantastic, 2007.
pgpfGEuqltjV7.pgp
Description: PGP signature