svn commit: r281281 - head/sys/dev/nvme

[Bruce Evans]

On Wed, 8 Apr 2015, Bjoern A. Zeeb wrote:

Please trim quotes.

>> On 08 Apr 2015, at 21:49 , Jim Harris <jimharris at FreeBSD.org> wrote:

[... quotes trimmed]

>> Log:
>>  nvme: create separate DMA tag for non-payload DMA buffers
>
> I think this one break i386 PAE and XEN kernels at least:
>
> /scratch/tmp/bz/head.svn/sys/modules/nvme/../../dev/nvme/nvme_qpair.c:502:6: error: implicit conversion from 'unsigned long long' to 'bus_size_t' (aka 'unsigned int') changes value from 18446744073709551615 to 4294967295 [-Werr
> or,-Wconstant-conversion]
>            BUS_SPACE_MAXADDR, 1, BUS_SPACE_MAXSIZE, 0,
>            ^~~~~~~~~~~~~~~~~
> ./x86/bus.h:121:27: note: expanded from macro 'BUS_SPACE_MAXADDR'
> #define BUS_SPACE_MAXADDR       0xFFFFFFFFFFFFFFFFULL
>                                ^~~~~~~~~~~~~~~~~~~~~

Also, the use of the long long abomination is a type error on amd64.
The same definition is used for both i386 PAE and amd64, but on amd64
bus_addr_t is uint64_t which is u_long.

Also, the macro name doesn't match the type name.  Only the macro name
has '_SPACE' or '_space' in it.  Other type names do have '_space' in
them, so the shorter spelling of bus_addr_t is apparently intentional.
I don't know if it is because bus_addr_t applies to more than bus
spaces or just to not be so verbose.

Even on i386 PAE, the abomination doesn't need to be spelled explicitly
with 'ULL'.  That was only needed for C90 compilers that warned about
constant literals too large for u_long.  In C99, 0xFFFFFFFFFFFFFFFF
is just a <abomination deleted> constant.

x86 bus.h is otherwise bad.  It still has half-baked support for K&R --
it has mounds of code to declare prototypes for static inline functions.
But K&R doesn't support inline, and without -funit-at-a-time inline
functions have to be defined before they are used or they don't work,
so there was never any need for prototypes.

The abomination is also used for BUS_SPACE_MAXADDR on mips64 and
powerpc64.  This gives the same type error as on amd64, without the
excuse of the PAE complication.

mips seems to have another bug from another complication.  Both
mips64 and mips with CPU_CNMIPS have a 64-bit bus_addr_t, but
only mips64 has a 64-bit BUS_SPACE_MAXADDR.

BUS_SPACE_UNRESTRICTED has a type error/fragility on all arches.
It is (~0).  It is spelled that way to reduce ifdefs.  It gives
a signed type that promotes to an unsigned type with the correct
number of bits when used in combination with bus_addr_t and other
unsigned types.  The others might not have 64 bits even on 64-bit
arches, so the ifdefs would be even messier than for BUS_SPACE_MAXADDR.
But this is fragile.  Perhaps the spelling should be
BUS_ADDR_UNRESTRICTED = BUS_ADDR_MAX where the latter is a correctly-
spelled BUS_SPACE_MAXADDR.

Some drivers still use the worse spelling of ~0ul.  This tends to work
for the same reason that ~0 works.  But ~0u wouldn't work.
BUS_SPACE_UNRESTRICTED is rarely used in device drivers.
BUS_SPACE_MAXADDR is used a lot.

> --- nvme_qpair.o ---
> *** [nvme_qpair.o] Error code 1
>
> /scratch/tmp/bz/head.svn/sys/modules/nvme/../../dev/nvme/nvme_qpair.c:502:6: error: implicit conversion from 'unsigned long long' to 'bus_size_t' (aka 'unsigned int') changes value from 18446744073709551615 to 4294967295 [-Werr
> or,-Wconstant-conversion]
>            BUS_SPACE_MAXADDR, 1, BUS_SPACE_MAXSIZE, 0,
>            ^~~~~~~~~~~~~~~~~
> ./x86/bus.h:121:27: note: expanded from macro 'BUS_SPACE_MAXADDR'
> #define BUS_SPACE_MAXADDR       0xFFFFFFFFFFFFFFFFULL
>                                ^~~~~~~~~~~~~~~~~~~~~
> 1 error generated.
> --- nvme_qpair.o ---

This has further type errors which are detected accidentally by PAE.
Apparently, 2 of the BUS_SPACE args are sizes, but one of these is
spelled BUS_SPACE_MAXADDR.  This is detected because BUS_SPACE_MAXADDR
is too large for a size on PAE.

The compiler misspells 0xFFFFFFFFFFFFFFFFULL as 18446744073709551615 in
the error message.

LINT should have an option or default to use type much weirder than PAE
so as to detect more of these type mismatches non-accidentally.
Unfortuntunately, this seems to require large ifdefs for the sizes too.
You would make bus_addr_t uint8_t with and BUS_ADDR_MAX = 0xFF or
weirder.  Then the error of spelling BUS_ADDR_MAX as ~0u would be
detected.  OTOH, the spelling of BUS_SPACE_UNRESTRICTED as ~0 would
probably not be detected.  ~0 is -1 on 2's complement systems, and
compilers don't warn about converting small negative values to an
unsigned type since the conversion is reversible.

Lower limits tend to be spelled as 0.  That works without much
fragility.  It only needs prototypes in scope.

Bruce