Date: Thu, 9 Apr 2015 14:48:49 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: "Bjoern A. Zeeb" <bz@freebsd.org> Cc: svn-src-head@freebsd.org, Jim Harris <jimharris@freebsd.org>, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r281281 - head/sys/dev/nvme Message-ID: <20150409134744.L1040@besplex.bde.org> In-Reply-To: <6E785386-3746-48A7-87B5-D33CDD999767@FreeBSD.org> References: <201504082149.t38LnjBZ058856@svn.freebsd.org> <6E785386-3746-48A7-87B5-D33CDD999767@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 8 Apr 2015, Bjoern A. Zeeb wrote: Please trim quotes. >> On 08 Apr 2015, at 21:49 , Jim Harris <jimharris@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150409134744.L1040>