Date: Sun, 30 Aug 2015 15:02:10 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Justin Hibbits <jrh29@alumni.cwru.edu> Cc: Adrian Chadd <adrian.chadd@gmail.com>, Marcel Moolenaar <marcel@xcllnt.net>, "freebsd-arch@freebsd.org" <freebsd-arch@freebsd.org> Subject: Re: Devices with 36-bit paddr on 32-bit system Message-ID: <20150830130612.L890@besplex.bde.org> In-Reply-To: <CAHSQbTAGD=4A20XZL09YXbEm9hdf5K2_QCRPFOAjrXHF4eg9sQ@mail.gmail.com> References: <CAHSQbTDsvB32%2BLyzHJO78VwUwAfUTMOUQp13BMCUpapSMT0fbg@mail.gmail.com> <ED4B5B25-D7A7-440C-9452-4C79B0800D2E@xcllnt.net> <1568331.OrSoeYfXsf@ralph.baldwin.cx> <CAJ-VmomduZBYT6=e7HUm2V1m0taM0MAMXxMojYV8wvgEKyUEyA@mail.gmail.com> <CAHSQbTAGD=4A20XZL09YXbEm9hdf5K2_QCRPFOAjrXHF4eg9sQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 29 Aug 2015, Justin Hibbits wrote:
> Hey, I already had that thought, too, you just encouraged me to take
> it :) Anyway, here's an initial patch. I've *only* tested it on one
> PowerPC board (the one needing this change), none of my other devices,
> and no other architectures. Thoughts?
It has lots of style bugs and type errors.
% Index: sys/dev/ata/ata-pci.c
% ===================================================================
% --- sys/dev/ata/ata-pci.c (revision 287189)
% +++ sys/dev/ata/ata-pci.c (working copy)
% @@ -217,7 +217,7 @@
%
% struct resource *
% ata_pci_alloc_resource(device_t dev, device_t child, int type, int *rid,
% - u_long start, u_long end, u_long count, u_int flags)
% + bus_addr_t start, bus_addr_t end, u_long count, u_int flags)
The first style bug is in the first changed line (line made longer than 80
columns by blind substitution).
% Index: sys/dev/ata/ata-pci.h
% ===================================================================
% --- sys/dev/ata/ata-pci.h (revision 287189)
% +++ sys/dev/ata/ata-pci.h (working copy)
% @@ -535,7 +535,7 @@
% int ata_pci_print_child(device_t dev, device_t child);
% int ata_pci_child_location_str(device_t dev, device_t child, char *buf,
% size_t buflen);
% -struct resource * ata_pci_alloc_resource(device_t dev, device_t child, int type, int *rid, u_long start, u_long end, u_long count, u_int flags);
% +struct resource * ata_pci_alloc_resource(device_t dev, device_t child, int type, int *rid, bus_addr_t start, bus_addr_t end, u_long count, u_int flags);
% int ata_pci_release_resource(device_t dev, device_t child, int type, int rid, struct resource *r);
% int ata_pci_setup_intr(device_t dev, device_t child, struct resource *irq, int flags, driver_filter_t *filter, driver_intr_t *function, void *argument, void **cookiep);
% int ata_pci_teardown_intr(device_t dev, device_t child, struct resource *irq, void *cookie);
These were already misformatted.
No comment on further style bugs from blind substitution.
% Index: sys/dev/ata/chipsets/ata-promise.c
% ===================================================================
% --- sys/dev/ata/chipsets/ata-promise.c (revision 287189)
% +++ sys/dev/ata/chipsets/ata-promise.c (working copy)
% @@ -191,18 +191,19 @@
% !BUS_READ_IVAR(device_get_parent(GRANDPARENT(dev)),
% GRANDPARENT(dev), PCI_IVAR_DEVID, &devid) &&
% ((devid == ATA_DEC_21150) || (devid == ATA_DEC_21150_1))) {
% - static long start = 0, end = 0;
% + static bus_addr_t start = 0;
% + static u_long count = 0;
Renaming the variable is an unrelated fix. The closure of this fix is
quite large:
- bus_get_resource() is undocumented except for a bogus reference to
bus_get_resource(9) in bus_set_resource(9) , so the meaning of its
last arg takes more work than necessary to check. The source file
bus_get_resource.9 doesn't exist, and the installed file
bus_get_resource.9.gz cannot be a link to bus_set_resource.9.gz
since the top-level bus man pages are partially correctly organized
with only 1 function per file.
- the meaning of bus_get_resource()'s args are hard to see from its
prototype since its prototype has no paramater names. <sys/bus.h> uses
a nonense mixture of no parameter names, parameter names without
underscores, and parameter names with underscores.
%
% if (pci_get_slot(dev) == 1) {
% - bus_get_resource(dev, SYS_RES_IRQ, 0, &start, &end);
% + bus_get_resource(dev, SYS_RES_IRQ, 0, &start, &count);
% strcat(buffer, " (channel 0+1)");
% }
% - else if (pci_get_slot(dev) == 2 && start && end) {
% - bus_set_resource(dev, SYS_RES_IRQ, 0, start, end);
% + else if (pci_get_slot(dev) == 2 && start && count) {
% + bus_set_resource(dev, SYS_RES_IRQ, 0, start, count);
% strcat(buffer, " (channel 2+3)");
% }
% else {
% - start = end = 0;
% + start = count = 0;
This assumes that the types are similar enough for the conversion to
work and not give a warning. If the types are integral, then the
conversion always works and compilers shouldn't warn if it is a
downcast (since 0 is representable in both types). But it isn't
clear that the types are integral. pc98 uses handles, but bus_addr_t
is still an integer and the handles are used to map it to the actual
address. Perhaps large addresses should be handled similarly.
% }
% }
% sprintf(buffer, "%s %s controller", buffer, ata_mode2str(idx->max_dma));
% Index: sys/dev/fdt/simplebus.c
% ===================================================================
% --- sys/dev/fdt/simplebus.c (revision 287189)
% +++ sys/dev/fdt/simplebus.c (working copy)
% ...
% @@ -365,7 +365,8 @@
% if (j == sc->nranges && sc->nranges != 0) {
% if (bootverbose)
% device_printf(bus, "Could not map resource "
% - "%#lx-%#lx\n", start, end);
% + "%#llx-%#llx\n", (uint64_t)start,
% + (uint64_t)end);
%
% return (NULL);
% }
This uses the long long abomination, and isn't even correct since type
type isn't even (unsigned) long long. The type starts as u_long, which
was not unsigned long long on any arch. It is cast to uint64_t. This
breaks it if it is larger than uint64_t. This makes it compatible with
unsigned long long on 32-bit arches, but has no effect on 32-bit arches
-- there the type remains as u_long and the above fails to compile.
No comment on further uses of the abomination.
% Index: sys/dev/gpio/gpiobus.c
% ===================================================================
% --- sys/dev/gpio/gpiobus.c (revision 287189)
% +++ sys/dev/gpio/gpiobus.c (working copy)
% @@ -178,7 +178,7 @@
% sc->sc_intr_rman.rm_type = RMAN_ARRAY;
% sc->sc_intr_rman.rm_descr = "GPIO Interrupts";
% if (rman_init(&sc->sc_intr_rman) != 0 ||
% - rman_manage_region(&sc->sc_intr_rman, 0, ~0) != 0)
% + rman_manage_region(&sc->sc_intr_rman, 0, ~(bus_addr_t)0) != 0)
I think ~0 gave the correct value except in the 1's complement case.
Stranglely, ~0ul is more fragile than ~0.
Elsewhere you spell this better as RM_MAX_END.
% panic("%s: failed to set up rman.", __func__);
%
% if (GPIO_PIN_MAX(sc->sc_dev, &sc->sc_npins) != 0)
% ...
% @@ -516,7 +516,7 @@
%
% if (type != SYS_RES_IRQ)
% return (NULL);
% - isdefault = (start == 0UL && end == ~0UL && count == 1);
% + isdefault = (start == 0UL && end == ~(bus_addr_t)0UL && count == 1);
Not using RM_MIN_START and RM_MAX_END gives even worse spelling than above.
Both of the UL's are unnecessary, and the second one is bogus since the
cast gives the correct type and it is not always UL.
% rle = NULL;
% if (isdefault) {
% rl = BUS_GET_RESOURCE_LIST(bus, child);
% Index: sys/dev/ofw/ofwbus.c
% ===================================================================
% --- sys/dev/ofw/ofwbus.c (revision 287189)
% +++ sys/dev/ofw/ofwbus.c (working copy)
% @@ -156,7 +156,7 @@
% sc->sc_mem_rman.rm_descr = "Device Memory";
% if (rman_init(&sc->sc_intr_rman) != 0 ||
% rman_init(&sc->sc_mem_rman) != 0 ||
% - rman_manage_region(&sc->sc_intr_rman, 0, ~0) != 0 ||
% + rman_manage_region(&sc->sc_intr_rman, 0, BUS_SPACE_MAXADDR) != 0 ||
Looks like a logic error. This is an rman function, so it should use an
rman limit (now spelled RMAN_MAX_END), not a bus space limit.
% rman_manage_region(&sc->sc_mem_rman, 0, BUS_SPACE_MAXADDR) != 0)
The old code had a mixture of ~0 and BUS_SPACE_MAXADDR. This almost made
sense. The first thing being managed is an interrupt id. Interrupts ids
are normally small nonnegative integers. But they are still represented
as u_longs or bus addresses. ~0 works for signed integers of any size
and also for unsigned integers of any size except in the 1's complement
case, and seems to have been used to reflect the smaller range required.
% panic("%s: failed to set up rmans.", __func__);
%
% ...
% @@ -201,7 +201,7 @@
% }
% start = rle->start;
% count = ulmax(count, rle->count);
% - end = ulmax(rle->end, start + count - 1);
% + end = qmax(rle->end, start + count - 1);
This doesn't use the long long abomination, but assumes that quads are
large enough. This is very fragile. No one knows what quads are, but
they are currently int64_t. uquads are uint64_t, but uqmax() doesn't
exist. So the above only works for 63-bit addresses. It seems to be
very broken already for 64-bit bus_addr_t, since that gives the 64-bit
address RMAN_MAX_END.
No comment on further uses of quads.
% }
%
% switch (type) {
% ...
% Index: sys/dev/pci/pci_pci.c
% ===================================================================
% --- sys/dev/pci/pci_pci.c (revision 287189)
% +++ sys/dev/pci/pci_pci.c (working copy)
% @@ -387,7 +388,7 @@
% char buf[64];
% int error, rid;
%
% - if (max_address != (u_long)max_address)
% + if (max_address != (bus_addr_t)max_address)
% max_address = ~0ul;
pci uses pci_addr_t for addresses including max_address. pci_addr_t
happens to be uint64_t, so it works better with a larger rman type/
Assume that the rman type is not larger. Then the above checks that
the rman type is large enough to hold the current max_address, and if
not it reduces to the _old_ rman limit. It should reduce to the current
rman limit (RMAN_END_MAX).
If the rman type is larger, then this code has no effect. Assigning
RMAN_END_MAX to max_address would overflow, but is not reached since
the rman type is large enough to hold any max_address.
% w->rman.rm_start = 0;
% w->rman.rm_end = max_address;
% ...
% Index: sys/kern/subr_rman.c
% ===================================================================
% --- sys/kern/subr_rman.c (revision 287189)
% +++ sys/kern/subr_rman.c (working copy)
% @@ -90,8 +90,8 @@
% TAILQ_ENTRY(resource_i) r_link;
% LIST_ENTRY(resource_i) r_sharelink;
% LIST_HEAD(, resource_i) *r_sharehead;
% - u_long r_start; /* index of the first entry in this resource */
% - u_long r_end; /* index of the last entry (inclusive) */
% + bus_addr_t r_start; /* index of the first entry in this resource */
% + bus_addr_t r_end; /* index of the last entry (inclusive) */
Blind substitution also breaks indentation of comments.
% u_int r_flags;
% void *r_virtual; /* virtual address of this resource */
% struct device *r_dev; /* device which has allocated this resource */
% @@ -135,7 +135,7 @@
% }
%
% if (rm->rm_start == 0 && rm->rm_end == 0)
RMAN_MIN_START is still spelled as 0 in many places. Since it is not very
magic, this spelling is better. Let prototypes comvert plain 0 to the
actual type.
% ...
% @@ -174,7 +174,7 @@
%
% /* Skip entries before us. */
% TAILQ_FOREACH(s, &rm->rm_list, r_link) {
% - if (s->r_end == ULONG_MAX)
% + if (s->r_end == BUS_SPACE_MAXADDR)
Why not RMAN_MAX_END?
I think some cases of RMAN_MAX_END are actually magic meaning "not really
the end, but a magic (default?) value". A different RMAN_FOO could be
used for this.
% Index: sys/powerpc/powerpc/nexus.c
% ===================================================================
% --- sys/powerpc/powerpc/nexus.c (revision 287189)
% +++ sys/powerpc/powerpc/nexus.c (working copy)
% @@ -189,13 +189,13 @@
% {
%
% if (type == SYS_RES_MEMORY) {
% - vm_offset_t start;
% + vm_paddr_t start;
I think more places uses this instead of bus_addr_t or uint64_t.
% void *p;
%
% - start = (vm_offset_t) rman_get_start(r);
% + start = rman_get_start(r);
The conversion is still logically non-null.
% ...
% Index: sys/sys/bus.h
% ===================================================================
% --- sys/sys/bus.h (revision 287189)
% +++ sys/sys/bus.h (working copy)
% @@ -29,6 +29,7 @@
% #ifndef _SYS_BUS_H_
% #define _SYS_BUS_H_
%
% +#include <machine/_bus.h>
% #include <machine/_limits.h>
% #include <sys/_bus_dma.h>
% #include <sys/ioccom.h>
% @@ -292,8 +293,8 @@
% int rid; /**< @brief resource identifier */
% int flags; /**< @brief resource flags */
% struct resource *res; /**< @brief the real resource when allocated */
% - u_long start; /**< @brief start of resource range */
% - u_long end; /**< @brief end of resource range */
% + bus_addr_t start; /**< @brief start of resource range */
% + bus_addr_t end; /**< @brief end of resource range */
I think rman functions should use an rman type and not hard-code bus_addr_t.
Related bus functions should then use this type. Style bugs from blind
substitution can be reduced by using a less verbose name.
% u_long count; /**< @brief count within range */
% };
% STAILQ_HEAD(resource_list, resource_list_entry);
% ...
% @@ -474,7 +475,8 @@
% static __inline struct resource *
% bus_alloc_resource_any(device_t dev, int type, int *rid, u_int flags)
% {
% - return (bus_alloc_resource(dev, type, rid, 0ul, ~0ul, 1, flags));
% + return (bus_alloc_resource(dev, type, rid, (bus_addr_t)0ul,
% + ~(bus_addr_t)0ul, 1, flags));
"ul" is now bogus, as above for UL except with a worse spelling.
(Apparently, RLIM_FOO is not available here, so you have to expand it
inline.)
The first cast to bus_addr_t also has no affect.
% }
%
% /*
% Index: sys/sys/rman.h
% ===================================================================
% --- sys/sys/rman.h (revision 287189)
% +++ sys/sys/rman.h (working copy)
% @@ -54,6 +54,9 @@
% #define RF_ALIGNMENT_LOG2(x) ((x) << RF_ALIGNMENT_SHIFT)
% #define RF_ALIGNMENT(x) (((x) & RF_ALIGNMENT_MASK) >> RF_ALIGNMENT_SHIFT)
Example of normal style (except for the long line).
%
% +#define RM_MIN_START ((bus_addr_t)0)
% +#define RM_MAX_END (~(bus_addr_t)0)
Style bugs (space instead of table after define).
There are no bogus ULs here. The first cast to bus_addr_t is probably
unecessary here too.
% @@ -108,8 +111,8 @@
% struct resource_head rm_list;
% struct mtx *rm_mtx; /* mutex used to protect rm_list */
% TAILQ_ENTRY(rman) rm_link; /* link in list of all rmans */
% - u_long rm_start; /* index of globally first entry */
% - u_long rm_end; /* index of globally last entry */
% + bus_addr_t rm_start; /* index of globally first entry */
% + bus_addr_t rm_end; /* index of globally last entry */
% enum rman_type rm_type; /* what type of resource this is */
% const char *rm_descr; /* text descripion of this resource */
% };
This has mounds of new and old style bugs:
new:
unformatting by blind substitution
old:
- still uses old style of a tab instead of a space after struct, enum,
and even in the middle of "const char"
- avoids misindented comment for rm_list by not having one
- has minimally misindented comment for rm_link after first using
excessive indentation for the member name (then a space instead of
a tab before the comment)
- has minimally misindented comment for rm_type (after first using
excessive indentation after enum, use only a space for the member
name and the comment).
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150830130612.L890>