Date: Sat, 3 May 2014 23:48:44 -0300 From: Luiz Otavio O Souza <loos.br@gmail.com> To: Bruce Evans <brde@optusnet.com.au> Cc: Luiz Otavio O Souza <loos@freebsd.org>, svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r265191 - head/sys/dev/gpio Message-ID: <CAJ8CS7q31wYWTJ5a-yAfd8hZm=wbHD8Nj8WN7gfMupxcpoyC9g@mail.gmail.com> In-Reply-To: <20140502180115.I1337@besplex.bde.org> References: <201405011409.s41E9mun075016@svn.freebsd.org> <20140502180115.I1337@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, May 2, 2014 at 5:34 AM, Bruce Evans wrote:
> On Thu, 1 May 2014, Luiz Otavio O Souza wrote:
>
>> Log:
>> Remove unnecessary headers. Sort out the headers. Add a missing header
>> on
>> ofw_gpiobus.c (it was working because of sys/libkern.h).
>
>
> Do you use /usr/src/tools/tools/kerninclude/kerninclude.sh to find the
> unused includes? There are many false positives and negatives which are
> hard to find without a lot of testing. kerninclude.sh automates some of
> the testing. I think it is rarely used and has rotted. A full universe
> build is probably required, but kerninclude.sh is i386-centric and only
> tests LINT, GENERIC and GENERIC98 (GENERIC98 last existed in the source
> tree in FreeBSD-3, but the script creates it by copying GENERIC and
> building with pc98 options).
>
> I prefer my unusedinc script. It is easier to run 1 on file at a time,
> but does less hacking to reduce the false positives and negatives.
I wasn't aware of the existence of kerninclude.sh. I usually go with
clearly unused headers (not as aggressive as with the automated
tools). Much of the code i touch has a lot of copy and paste
leftovers. A few examples can be found in sys/etherswitch/arswitch/*
which includes iic headers when it isn't needed, but i avoid to make
these changes until i have time to test it.
Your script seems to be less complex to use and, indeed, has helped to
find all the unused includes in these files.
As for test, i usually run my changes through a universe build anyway.
Most of the last minute changes (which won't pass by universe test)
tends to bring issues.
>
>
>> Modified: head/sys/dev/gpio/gpiobus.c
>>
>> ==============================================================================
>> --- head/sys/dev/gpio/gpiobus.c Thu May 1 14:08:19 2014 (r265190)
>> +++ head/sys/dev/gpio/gpiobus.c Thu May 1 14:09:47 2014 (r265191)
>> @@ -28,21 +28,16 @@
>> __FBSDID("$FreeBSD$");
>>
>> #include <sys/param.h>
>> -#include <sys/systm.h>
>
>
> This unsorts the includes. <sys/systm.h> must be included after
> <sys/param.h>, and should be always included, since other headers may
> depend on it (for things like KASSERT() in inline functions). Some
> broken headers include it nested. This makes it difficult to tell
> whether it is used. Some other headers that don't do this may
> compile accidentally because they are included after the polluted
> ones.
Fixed.
>
>> +#include <sys/bus.h>
>> +#include <sys/gpio.h>
>> +#include <sys/kernel.h>
>> #include <sys/malloc.h>
>> #include <sys/module.h>
>> -#include <sys/kernel.h>
>> -#include <sys/queue.h>
>
>
> <sys/queue.h> is probably used. It is included nested in many headers,
> and this is not considered pollution, unlike for <sys/systm.h>, but it
> makes it very hard to determine if <sys/queue.h> is included as needed
> in other headers and .c files.
>
> kerninclude.sh attempts to determine if headers like <sys/queue.h> are
> needed by doing things like replacing it by an empty header in some
> contexts. I suspect this doesn't handle the full complications.
> Ideally, the include of <sys/queue.h> in a .c file should be explicit
> iff the .c files uses any queue macro.
No queue macros are in use in these files.
>
>> -#include <sys/sysctl.h>
>> +#include <sys/systm.h>
>> #include <sys/types.h>
>
>
> <sys/types.h> is certainly not needed. It is standard pollution in
> <sys/param.h>, and it is a style bug to not depend on that. It is
> also usually a style bug (in kernel code) to include <sys/types.h>
> and not include <sys.param.h>. Many things depend on the standard
> pollution, or might be changed to depend on it.
Fixed, this is even documented on style(9)... My bad...
>
>> Modified: head/sys/dev/gpio/ofw_gpiobus.c
>>
>> ==============================================================================
>> --- head/sys/dev/gpio/ofw_gpiobus.c Thu May 1 14:08:19 2014
>> (r265190)
>> +++ head/sys/dev/gpio/ofw_gpiobus.c Thu May 1 14:09:47 2014
>> (r265191)
>> @@ -33,17 +33,13 @@ __FBSDID("$FreeBSD$");
>> #include <sys/bus.h>
>> #include <sys/gpio.h>
>> #include <sys/kernel.h>
>> -#include <sys/libkern.h>
>
>
> Correct. <sys/libkern.h> is standard pollution in <sys/systm.h>, and it
> is a style bug to include it directly.
>
>
>> -#include <sys/lock.h>
>> #include <sys/module.h>
>> -#include <sys/mutex.h>
>> +#include <sys/systm.h>
>
>
> Not using mutexes may indicate missing locking. I think this works because
> almost everything in new-bus is Giant-locked and Giant locking hides its
> own details very well.
Mutexes are in use here, but sys/lock.h and sys/mutex.h comes from
gpiobusvar.h (which i may need to validate again).
[...]
Thanks for the review, it is greatly appreciated.
Regards,
Luiz
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ8CS7q31wYWTJ5a-yAfd8hZm=wbHD8Nj8WN7gfMupxcpoyC9g>