synchronization question about /sys/dev/vkbd/vkbd.c

[Maksim Yevmenkin]

Norbert,

> When looking at /sys/dev/vkbd/vkbd.c I found
> one thing, that I do not understand.
> 
> There are three places, where a flag TASK is used:
> 
> 1. in vkbd_dev_close():
> 
>   while(state->ks_flag & TASK) VKBD_SLEEP (...);
> 
> 2. in vkbd_dev_write()
> 
>   VKBD_LOCK ();
>   ...
>   if (!(state->ks_flags & TASK) && task_enqueue(...))
>     state->ks_flags |= TASK;
>   ...
>   VKBD_UNLOCK ();
> 
> 3. in vkbd_dev_intr()
> 
>   ...
>   /* call intr */
>   ...
>   VKBD_LOCK();
>   state->ks_flags &= ~TASK;
>   wakeup(...);
>   VKBD_UNLOCK();
> 
> As I understand:
> vkbd_dev_write() writes data into its queue
> and wants vkbd_dev_intr() to process the queue.

vkbd_dev_intr() is a "interrupt handler". the real keyboard would 
generate interrupt when keys are pressed/released. vkbd(4) does not have 
real keyboard. instead, as soon as vkbd_dev_write() puts scancodes into 
the queue it schedules vkbd_dev_intr() task (to emulate keyboard 
interrupt). the TASK flag is used to indicate the fact that "intrrupt" 
is pending and vkbd(4) does not need to schedule one.

> My question is:
> Is it not possible, that vkbd_dev_intr() could be
> interrupted at any position before the VKBD_LOCK()
> and then vkbd_dev_write() called?

in theory it is possible.

> If yes, how should vkbd_dev_write() know, that it should
> call task_enqueue(), as TASK is still set?

well, i guess it is possible to miss interrupt in this case. also, the 
scancodes are not lost, they will be processed on next write.

i suspect that the vkbd_dev_intr() should be interrupted exactly in between

(*kbdsw[kbd->kb_index]->intr)(kbd, NULL);

and

VKBD_LOCK(state);

this is because most/all of intr() keyboard methods have something like

while (check_char) {
	read_char()
	...
}

> Why not always call task_enqueue() unconditionally here
> and leave TASK only to synchronize the close call?

yes, that could be done. it is also possible to have a callout going few 
times a second to check if there is a scancodes in the queue and 
schedule vkbd_dev_intr(). funny that atkbd(4) and ukbd(4) have just this.

thanks,
max