Re: (almost working) patch to make opencrypto use mutex/condvar

[Andrew Doran <ad%netbsd.org@localhost>]

On Sat, Feb 02, 2008 at 07:26:43AM +0100, Pawel Jakub Dawidek wrote:

> On Fri, Feb 01, 2008 at 04:45:13PM -0500, Thor Lancelot Simon wrote:
> > @@ -621,10 +635,13 @@
> >  
> >     error = crypto_kdispatch(krp);
> >     if (error == 0)
> > -           error = tsleep(krp, PSOCK, "crydev", 0);
> > -   if (error)
> > +           mutex_spin_enter(&crypto_mtx);
> > +           cv_wait(&krp->krp_cv, &crypto_mtx);     /* XXX cv_wait_sig? */
> > +           mutex_spin_exit(&crypto_mtx);
> > +#if 0
> > +   if (error)                      /* see XXX above -- wait intr? */
> >             goto fail;
> > -
> > +#endif
> 
> You changed:
> 
>       if (error == 0)
>               error = tsleep(krp, PSOCK, "crydev", 0);
> 
> to this:
> 
>       if (error == 0)
>               mutex_spin_enter(&crypto_mtx);
>               cv_wait(&krp->krp_cv, &crypto_mtx);     /* XXX cv_wait_sig? */
>               mutex_spin_exit(&crypto_mtx);
> 
> and it should be of course:
> 
>       if (error == 0) {
>               mutex_spin_enter(&crypto_mtx);
>               cv_wait(&krp->krp_cv, &crypto_mtx);     /* XXX cv_wait_sig? */
>               mutex_spin_exit(&crypto_mtx);
>       }

That looks suspicious. What if the event being waited for occurs before
mutex_spin_enter(&crypto_mtx)? Without the mutex being used to interlock
it's just there to placate cv_wait().

Thanks,
Andew