Re: kernel condvars: how to use?

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

> Date: Thu, 7 Dec 2017 22:26:54 -0500 (EST)
> From: Mouse <mouse%Rodents-Montreal.ORG@localhost>
> 
> DIAGNOSTIC and DEBUG are both on already.  None of the LOCK* options
> are, though - see Paul Goyette's response, below.

My usual debug-all-the-things kernel configuration is:

makeoptions 	DBG=-"-g"
options 	DEBUG
options 	DIAGNOSTIC
options 	LOCKDEBUG

> ftp.rodents-montreal.org:/mouse/misc/lpt/ holds the code.  base/ has
> the code I started with (which is stock 5.2, for these files); new/ has
> my current version.  lptreg.h is identical; lptvar.h and lpt.c have
> changes.  diffs is output from "diff -u -r base new".  (All these are
> relative to sys/dev/ic/.)

Where do you set LPT_RF_WAITING?

Some other general comments:

Are you sure that's the code you're using?  lpt_raw_read seems to
refer to a variable s in splx(s) without defining it -- seems to have
just `spllpt()' where it should have `s = spllpt()'.  Of course, all
that is unnecessary with a lock at the right IPL, but it makes me
suspicious that I'm not looking at the code that reproduces your bug.

It's unclear to me why you have splhigh in lptopen.  splhigh should be
reserved for very special purposes that are clearly justified.  If you
need to defer the first tick until after the rest of the lpt is
initialized, just use callout_setfunc first, and then after it's
initialized at the end of lptopen do callout_schedule.

For lptclose, note that callout_stop does not wait for the callout to
stop if it was in the middle of executing, and does not destroy it.
You must use callout_halt to wait, and then use callout_destroy to
destroy it.

In this case, everything will run giant-locked, but if you're writing
new code you should avoid relying on that, and definitely avoid
volatile voodoo.  For instance, in lpt_raw_read_tick, I advise that
you unconditionally acquire the lock and use it for all access to the
new fields you added: rawbuf, rflags, rbfill, rbptr, laststat.  (If
not, then whoever wants to remove the giant lock will have to figure
out the right places to put in memory ordering barriers, and that's
not a fun surprise to give someone!)

Can you write down all the states you expect this thing to be in, and
the state transitions that each action -- lpt_raw_read_tick,
lpt_raw_read -- can take, and in what states lpt_raw_read must wait
for a tick to transition to another state before it can complete?

The condition variable represents exactly that set of state
transitions of interest under that mutex, and it's helpful to identify
what it is in order to assess whether all paths that effect those
state transitions also notify the condition variable.

> ...I used IPL_VM, because splvm() is the same as spllpt(), and because
> the manpage says that results in a spin mutex, which I thought was what
> I needed.  Is there some way to tell what IPL the hardware interrupt
> for the device comes in on?

You should set the IPL to the same one that the interrupt handler runs
at.  In this case, it's a callout, so it's IPL_SOFTCLOCK, as the
callout(9) man page says.

(Under the hood, in this case, there actually is _no_ splsoftclock()
issued by mutex_enter, but that's not important to you -- what is
important is that you pass IPL_SOFTCLOCK to mutex_init so that
mutex_enter works in thread or interrupt context to exclude one
another.)

> Or, in my case, hang?  I don't know who holds it; I'm not sure how to
> find out.  I should probably go peek under the hood.

With LOCKDEBUG, if you know the lock address you can use `show lock'
in ddb.