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 - cube%cubidou.net@localhost - cube%NetBSD.org@localhost
"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.
Attachment:
pgp_VgmQNbymB.pgp
Description: PGP signature