svn commit: r265191 - head/sys/dev/gpio

[Luiz Otavio O Souza]

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