Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 20 May 2019 21:05:05 +1000 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        Conrad Meyer <cem@freebsd.org>
Cc:        src-committers@freebsd.org, svn-src-all@freebsd.org,  svn-src-head@freebsd.org
Subject:   Re: svn commit: r347984 - in head/sys: amd64/vmm/io arm/allwinner arm/allwinner/a10 arm/allwinner/clkng arm/arm arm/broadcom/bcm2835 arm/freescale/imx arm/mv arm/mv/armada arm/nvidia arm/nvidia/tegra12...
Message-ID:  <20190520202101.L1014@besplex.bde.org>
In-Reply-To: <201905200038.x4K0cNoZ019920@repo.freebsd.org>
References:  <201905200038.x4K0cNoZ019920@repo.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 20 May 2019, Conrad Meyer wrote:

> Log:
>  Extract eventfilter declarations to sys/_eventfilter.h

sys/_eventhandler.h has 2 identical 72-line copies of the correct version.
The second copy of the reinclusion guard works as a guard against the
doubling too.

>
>  This allows replacing "sys/eventfilter.h" includes with "sys/_eventfilter.h"
>  in other header files (e.g., sys/{bus,conf,cpu}.h) and reduces header
>  pollution substantially.
>
>  EVENTHANDLER_DECLARE and EVENTHANDLER_LIST_DECLAREs were moved out of .c
>  files into appropriate headers (e.g., sys/proc.h, powernv/opal.h).

Thanks, but this gives even more pollution.  Almost everything includes
sys/proc.h, so its already large pollution affects almost everything.
The previous pollution in it didn't include sys/eventhandler.h, except
possibly via a nested include.

> Modified: head/sys/amd64/vmm/io/iommu.c
> ==============================================================================
> --- head/sys/amd64/vmm/io/iommu.c	Sun May 19 23:56:04 2019	(r347983)
> +++ head/sys/amd64/vmm/io/iommu.c	Mon May 20 00:38:23 2019	(r347984)
> @@ -32,10 +32,10 @@
> __FBSDID("$FreeBSD$");
>
> #include <sys/param.h>
> -#include <sys/types.h>
> -#include <sys/systm.h>

Error.  sys/systm.h must be included after sys/param.h, since it contains
macros like KASSERT() which may be used in inline functions in any kernel
header except sys/param.h and possibly sys/types.h.

> Modified: head/sys/arm/allwinner/a10/a10_intc.c
> ==============================================================================
> --- head/sys/arm/allwinner/a10/a10_intc.c	Sun May 19 23:56:04 2019	(r347983)
> +++ head/sys/arm/allwinner/a10/a10_intc.c	Mon May 20 00:38:23 2019	(r347984)
> @@ -30,11 +30,12 @@ __FBSDID("$FreeBSD$");
>
> #include "opt_platform.h"
>
> -#include <sys/types.h>
> +#include <sys/param.h>

Including sys/types.h and not including sys/param.h was another error.
sys/param.h supplies lots of standard pollution that may be depended
on by any other kernel header.  It is impractical to even test if
sys/types.h works with all combinations of ifdefs.

> #include <sys/bus.h>
> #include <sys/cpuset.h>
> #include <sys/kernel.h>
> #include <sys/ktr.h>
> +#include <sys/lock.h>
> #include <sys/module.h>
> #include <sys/mutex.h>
> #include <sys/param.h>

sys/param.h is now included twice.  It is another header that must be
included in non-alphabetical order.  The second include may have
compensated for not including it first.

>
> Modified: head/sys/arm/allwinner/a10_dmac.c
> ==============================================================================
> --- head/sys/arm/allwinner/a10_dmac.c	Sun May 19 23:56:04 2019	(r347983)
> +++ head/sys/arm/allwinner/a10_dmac.c	Mon May 20 00:38:23 2019	(r347984)
> @@ -38,7 +38,9 @@ __FBSDID("$FreeBSD$");
> #include <sys/rman.h>
> #include <sys/condvar.h>
> #include <sys/kernel.h>
> +#include <sys/lock.h>
> #include <sys/module.h>
> +#include <sys/mutex.h>
>
> #include <machine/bus.h>
>
> Modified: head/sys/arm/allwinner/a31_dmac.c
> ==============================================================================
> --- head/sys/arm/allwinner/a31_dmac.c	Sun May 19 23:56:04 2019	(r347983)
> +++ head/sys/arm/allwinner/a31_dmac.c	Mon May 20 00:38:23 2019	(r347984)
> @@ -38,7 +38,9 @@ __FBSDID("$FreeBSD$");
> #include <sys/rman.h>
> #include <sys/condvar.h>
> #include <sys/kernel.h>
> +#include <sys/lock.h>
> #include <sys/module.h>
> +#include <sys/mutex.h>
> #include <sys/endian.h>
>
> #include <machine/bus.h>

I gave up trying to fix missing includes of sys/mutex.h and its
prerequisite sys/lock.h 15+ years ago before disk code added lots more
of them.  sys/buf.h was polluted by adding an include of sys/mutex.h,
and most disk drivers dependended on that.  The only other includes
of sys/mutex.h in a <sys> header were in sys/eventhandler.h and
sys/vnode.h, and the latter was fixed in my version.  This pollution
has spread to sys/buf_ring.h, sys/fail.h, sys/jail.h, sys/mman.h,
sys/msgbuf.h, sys/rmlock.h, sys/timeet.h and sys/tty.h.

The smaller headers here are trivial to fix, and even sys/vnode.h
wasn't hard to fix 15 years ago before the pollution spread.  sys/mman.h
seems to have only 1 mutex declaration, of a struct mtx, so using the
correct header sys/_mutex.h works.  Only inlines using mutexes cause
pollution that is hard to avoid.  sys/vnode.h still has only 1 inline
function.

Bruce



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20190520202101.L1014>