Re: [patch 04/19] Add a framework to manage clock event devices.

[Arjan van de Ven <arjan@infradead.org>]

On Thu, 2006-11-09 at 23:38 +0000, Thomas Gleixner wrote:
> + * struct clock_event_device - clock event descriptor
> + *
> + * @name:		ptr to clock event name
> + * @capabilities:	capabilities of the event chip
> + * @max_delta_ns:	maximum delta value in ns
> + * @min_delta_ns:	minimum delta value in ns
> + * @mult:		nanosecond to cycles multiplier
> + * @shift:		nanoseconds to cycles divisor (power of two)
> + * @set_next_event:	set next event
> + * @set_mode:		set mode function
> + * @suspend:		suspend function (optional)
> + * @resume:		resume function (optional)
> + * @evthandler:		Assigned by the framework to be called by the low
> + *			level handler of the event source
> + */

it would be nice if the datastructure was "pure"; eg entirely owned by
the source (and could be made const), however the only way I can see
that done is by having a private duplicate datastructure in each clock
driver... which is way overkill for one single function pointer ;(


well you could do

struct clock_event_device 
{
	const struct clock_ops *ops;
	void *instance;
	evthndlr_t *evthandler;
}

that way if you have, say, 3 hpet channels you can use the same
"ops" (or maybe "props") structure for all three, but still register the
per channel state as well..


you maybe also want to have a "costs" member, so that you can pick the
cheapest available timer..


> +struct clock_event_device {
> +	const char	*name;
> +	unsigned int	capabilities;
> +	unsigned long	max_delta_ns;
> +	unsigned long	min_delta_ns;
> +	unsigned long	mult;
> +	int		shift;
> +	void		(*set_next_event)(unsigned long evt,
> +					  struct clock_event_device *);
> +	void		(*set_mode)(enum clock_event_mode mode,
> +				    struct clock_event_device *);
> +	void		(*event_handler)(struct pt_regs *regs);

is pt_regs really really needed here? We got rid of it in most places
(and made it a per tast struct thing), I wonder if it can be made to go
away here too.


> +/*
> + * Start up an event device
> + */
> +static void startup_event(struct clock_event_device *evt, unsigned int caps)
> +{
> +	int mode;
> +
> +	if (caps == CLOCK_CAP_NEXTEVT)

isn't caps a bitfield ? if so, shouldn't this be a & ?


> + */
> +int clockevents_set_next_event(ktime_t expires, int force)
> +{
> +	struct local_events *devices = &__get_cpu_var(local_eventdevices);
> +	struct clock_event_device *nextevt = devices->nextevt;
> +	int64_t delta = ktime_to_ns(ktime_sub(expires, ktime_get()));
> +
> +	if (delta <= 0 && !force) {
> +		devices->expires_next.tv64 = KTIME_MAX;
> +		return -ETIME;
> +	}

hmmm so if I set a timer 10 nsec in the future, and then I get an
interrupt, I suddenly get an infinite time timer? Sounds more like a
case of "please just run it right away"


> + * Resume the cpu local clock events
> + */
> +static void clockevents_resume_local_events(void *arg)
> +{
> +	struct local_events *devices = &__get_cpu_var(local_eventdevices);
> +	int i;
> +
> +	for (i = 0; i < devices->installed; i++) {
> +		if (devices->events[i].real_caps)
> +			startup_event(devices->events[i].event,
> +				      devices->events[i].real_caps);
> +	}
> +	touch_softlockup_watchdog();
> +}

what is this watchdog touching for?

> +static int clockevents_cpu_notify(struct notifier_block *self,
> +				  unsigned long action, void *hcpu)
> +{
> +	switch(action) {
> +	case CPU_UP_PREPARE:
> +		break;

don't you want to start the per cpu timer in such a case?




-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via http://www.linuxfirmwarekit.org

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/