Re: Module and device configuration locking [was Re: Modules loading modules?]

[Mindaugas Rasiukevicius <rmind%netbsd.org@localhost>]

Hello,

Paul Goyette <paul%whooppee.com@localhost> wrote:
> 
> Well, it was a slow day at the office today, so I found some time to 
> work on this!  :)
> 
> Attached is the latest version of this change.  For simplicity, I have 
> broken the patches up into five separate groups:
> 
> <...>

Few comments on the patch:

> -#define      __NetBSD_Version__      599003800       /* NetBSD 5.99.38 */
> +#define      __NetBSD_Version__      599003900       /* NetBSD 5.99.39 */

Why bumping?

> + * of of kernel functionality, such as device configuration and loading

One "of" too much.

> +void
> +kernconfig_lock(void)
> +{
> +     lwp_t   *l;
> +
> +     /*
> +      * OK to check this unlocked, since it could only be set to
> +      * curlwp by the current thread itself, and not by an interrupt
> +      * or any other LWP.
> +      */

Please add KASSERT(!cpu_intr_p());

> +     l = curlwp;
> +     if (kernconfig_lwp == l) {
> +             kernconfig_recurse++;
> +             KASSERT(kernconfig_recurse != 0);

Such (++ then != 0) assert is a bit useless.  How about:

KASSERT(kernconfig_recurse > 1);

> +     } else {
> +             mutex_enter(&kernconfig_mutex);
> +             kernconfig_lwp = l;
> +             kernconfig_recurse = 1;
> +     }
> +}

Somebody will confuse l with 1. :)

> +     if (--kernconfig_recurse == 0) {
> +             kernconfig_lwp = 0;
> +             mutex_exit(&kernconfig_mutex);

kernconfig_lwp = NULL;

> +bool
> +kernconfig_is_held(void)
> +{
> +
> +     return (mutex_owned(&kernconfig_mutex));

No need for brackets around function.

> +     KASSERT(kernconfig_is_held);

But missing brackets in few cases.

Otherwise seems OK to me.  Thanks.

-- 
Mindaugas