cvs commit: src/sys/dev/fdc fdc.c fdcvar.h

[Joerg Wunsch]

Thanks for the interesting discussion.  I've learned a bit by that.

> > > If a thread is suspended and the module is unloaded, what would
> > > be the failure case?

> > Leaving unclaimed KVM around that can never be recovered.

> It would be really sad if we _designed_ FreeBSD to leak threads.
> Windows stopped doing that back with Win98.  We're better than that.

I wouldn't even have dreamed of leaking resources. ;-)

Btw., the main reason for wanting to be able to kldunload the fdc
module is to ease development work.  It would be very interesting to
suspend/resume a thread when the actual implementation changed between
the invocations...

As M. Warner Losh wrote:

>  John Baldwin <jhb at freebsd.org> writes:

> : Note that relying on a wakeup from your own code is not safe if you
> : expect fdc to be a module since you could wakeup the thread doing the
> : kldunload (and thus detach) and then be interrupted for an interrupt and
> : it could unmap the memory backing that function before you get a chance
> : to run again resulting in a panic. [...]

> Wouldn't simply unregistering your interrupt before killing the worker
> thread handle this case?  [...]

> The floppy driver, however, doesn't appear to do this.  It kills the
> thread and then releases the interrupt.  It should do something more
> like:

> +	/* Tear down the interrupt before killing the thread */
> +	if (fdc->fdc_intr)
> +		bus_teardown_intr(dev, fdc->res_irq, fdc->fdc_intr);
> +	fdc->fdc_intr = NULL;
> +
>  	/* kill worker thread */
> -	fdc->flags |= FDC_KTHREAD_EXIT;
>  	mtx_lock(&fdc->fdc_mtx);
> +	fdc->flags |= FDC_KTHREAD_EXIT;

Sounds reasonable.  I'll do it that way.  I simply didn't think of the
possible interrupt issue.  OTOH, the thread will only exit if all bp's
are done, which somehow implies that no further interrupts should
happen.

As M. Warner Losh wrote:

> : Why not use kthread_suspend/kthread_suspend_check instead?

> because this is safer.  The thread exit code will wake up all sleepers
> on the thread pointer when the thread is totally dead.  This ensures
> that the thread is out of the driver and it is safe to proceed.

*That* was the information I've been looking for.  I already wondered
how pccbb (which you've been referring me to) actually woke up the
parent.

I've just added this piece of information to kthread(9) to spare
others the puzzling over it.

> However, the fdc->flags |= should be under the lock, because
> otherwise you are racing the thread's touching of flags.

phk did already most of this.  IMHO, it should also be protected
inside the worker thread, but since I accidentally forgot to call
kthread_exit() anyway, I'll commit these changes in one rush then.

-- 
cheers, J"org               .-.-.   --... ...--   -.. .  DL8DTL

http://www.sax.de/~joerg/                        NIC: JW11-RIPE
Never trust an operating system you don't have sources for. ;-)